diff --git a/.changeset/cli-hook-body-lowering-loud.md b/.changeset/cli-hook-body-lowering-loud.md new file mode 100644 index 0000000000..2216914f46 --- /dev/null +++ b/.changeset/cli-hook-body-lowering-loud.md @@ -0,0 +1,56 @@ +--- +'@objectstack/cli': minor +--- + +feat(cli): `os lint` refuses a hook body that silently stopped being metadata (#13651) + +An L2 hook handler is lowered to a metadata-only `body.source` and evaluated in +QuickJS with no module scope. When the handler reaches out of that scope, +`extractHookBody` refuses — and `lowerCallables` caught the refusal, recorded it, +and shipped the callable through the back-compat `.mjs` bundle instead. `os build` +exited 0. The hook kept working locally. What changed silently was the +**deployment shape**: the app had stopped being shippable as pure metadata. + +The refusal was never the missing piece — `extractHookBody` had already computed +the exact free-identifier list. What was missing is that nothing said no to the +recorded array. + +**The refusal now carries its classification — and what that publishes rides +on two CLI surfaces, not on new API exports.** Internally, +`HookBodyExtractionError` (with `HookBodyRefusalKind`) names which rule +refused — `free-identifiers` / `forbidden-token` / `unparseable` — and +`lowerCallables` records it beside the identifier list on each +`bodyExtractionWarnings` entry. Those types are module-internal: the package +`exports` map exposes only `.` and `./console`, and `src/index.ts` re-exports +none of them. What this release actually publishes is: + +- **`os lint`'s exit contract** — the new `hook-body/not-lowerable` rule is an + `error`, so `os lint` can now exit 1 where it previously exited 0. +- **`os build --json`** — each `bodyExtractionWarnings` entry now carries + `kind` and `freeIdentifiers` fields beside the unchanged `origin`/`reason`. + +**`os lint` is the first consumer, and it tells the classes apart.** They +used to share one catch, so they shared one fate: + +- **accidental** (`free-identifiers`) — the handler *is* expressible as a + metadata body; it merely names a module-scope const, helper or import. Now a + lint **`error`**, so a gate can fail on it. `os lint` exits 1. +- **structural** (`forbidden-token`) — `fetch`/`require`/`process`/… are + capabilities the sandbox does not have, so writing one *is* choosing a bundled + closure and the bundle is the designed answer. Reported as a **`warning`**, + never fatal. +- **instrument** (`unparseable`, or a failure of the extractor itself) — the + tool could not judge the body at all. Reported as a **`warning`** under its + own rule, with prose that names the instrument — never as if the author chose + a bundle, because an instrument failure is not a verdict about the author. + +An author who deliberately wants a bundled closure keeps two channels that +already existed and are still silent: give the hook an explicit `body`, or move +the function into the top-level `functions:` map and reference it by name. + +**What did NOT change: what `os build` accepts.** The catch in `lowerCallables` +stays, both classes still fall back to bundling, and the build still exits 0 — +verified over a real spawned `os build`. Flipping that default is a separate +contract decision. The new rule runs the *same* `extractHookBody` the build +runs, so the lint verdict cannot drift from what the build would do to the same +handler. diff --git a/packages/cli/src/commands/lint.ts b/packages/cli/src/commands/lint.ts index a6afcc900e..5690b98f61 100644 --- a/packages/cli/src/commands/lint.ts +++ b/packages/cli/src/commands/lint.ts @@ -11,6 +11,7 @@ import { lintDataModel, runAuthoringRules } from '@objectstack/lint'; import { resolveSduiManifest } from '../utils/sdui-manifest.js'; import { collectAndLintDocs } from '../utils/collect-docs.js'; import { scoreMetadata } from '../lint/score.js'; +import { checkHookBodyLowering } from '../lint/hook-body-lowering.js'; import { runMetadataEval } from '../lint/metadata-eval.js'; import { DEFAULT_METADATA_EVAL_CORPUS } from '../lint/corpus.js'; import { @@ -400,6 +401,19 @@ export function lintConfig(config: any, opts: LintConfigOptions = {}): LintIssue } } + // ── Hook/action bodies that cannot be lowered to metadata (#13651) ── + // `os build` catches every extraction refusal, warns, and bundles the closure + // at exit 0 — so an app can stop being shippable as pure metadata with nothing + // red anywhere. This rule is the "no" to that recorded array. It runs the SAME + // `extractHookBody` the build runs, so the two cannot disagree, and it splits + // the accidental class (an `error`, which a gate can fail on) from the + // structural one (a `warning`, because bundling is its designed answer). It + // does NOT move what `os build` accepts; see the rule module's header. + // + // Reads FUNCTION values, so it must run on the normalized input before any + // Zod parse — which is where `lintConfig` already sits. + issues.push(...checkHookBodyLowering(config as Record)); + // ── Data-model best practices (relationships / master-detail / roll-ups) ── // Cross-object rules that encode the conventions in ADR-0035 and the // objectstack-data/-ui skills. These double as the eval rubric (see score.ts). diff --git a/packages/cli/src/lint/hook-body-lowering.test.ts b/packages/cli/src/lint/hook-body-lowering.test.ts new file mode 100644 index 0000000000..5af4fdea76 --- /dev/null +++ b/packages/cli/src/lint/hook-body-lowering.test.ts @@ -0,0 +1,292 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #13651 — the silent downgrade becomes a lint verdict. + * + * The pins here are about the DISTINCTION, not just the noise: an accidental + * scope leak must be an `error` (so a gate can fail on it) while the structural + * refusal must stay a `warning` (so the legitimate fallback-to-bundling path is + * not punished). A test that only asserted "something was reported" would pass + * on the change this card explicitly forbids — deleting the catch. + */ + +import { describe, it, expect } from 'vitest'; +import { + checkHookBodyLowering, + NOT_LOWERABLE_RULE, + BUNDLED_FALLBACK_RULE, + UNPARSEABLE_BODY_RULE, + EXTRACTION_FAILED_RULE, +} from './hook-body-lowering.js'; +import { lowerCallables } from '../utils/lower-callables.js'; + +// Module scope — exactly what a lowered body cannot reach. +const SLA_MATRIX: Record = { high: 4, low: 48 }; + +const freeIdentifierHook = { + name: 'case_sla', + object: 'case', + events: ['beforeInsert'], + handler: (ctx: any) => { + ctx.input.sla_hours = SLA_MATRIX[ctx.input.priority]; + }, +}; + +const forbiddenTokenHook = { + name: 'enrich_lead', + object: 'lead', + events: ['beforeInsert'], + handler: async (ctx: any) => { + const res = await fetch('https://example.invalid/enrich'); + ctx.input.score = res.status; + }, +}; + +const selfContainedHook = { + name: 'normalize_name', + object: 'account', + events: ['beforeInsert'], + handler: (ctx: any) => { + ctx.input.name = String(ctx.input.name).trim(); + }, +}; + +// `unparseable`: `String(fn)` yields something `peelToBlockBody` cannot peel — +// an instrument LIMIT, not anything the author chose. +const unparseableHook = (() => { + const fn = (ctx: any) => { + ctx.input.x = 1; + }; + Object.defineProperty(fn, 'toString', { value: () => '???' }); + return { name: 'opaque', object: 'o', events: ['beforeInsert'], handler: fn }; +})(); + +// `unknown`: the extractor itself throws a bare Error (not a +// `HookBodyExtractionError`) — an instrument FAILURE. Same fixture shape the +// build-side pin in `hook-body-refusal-kind.test.ts` uses. +const explodingHook = (() => { + const fn = () => undefined; + Object.defineProperty(fn, 'toString', { + value: () => { + throw new Error('cannot stringify'); + }, + }); + return { name: 'exploding', object: 'o', events: ['beforeInsert'], handler: fn }; +})(); + +describe('checkHookBodyLowering', () => { + it('reports an accidental scope leak as an ERROR a gate can fail on', () => { + const issues = checkHookBodyLowering({ hooks: [freeIdentifierHook] }); + + 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'); + // Names the callable and the identifier that caused it — the diagnostic + // `extractHookBody` had already computed and the build dropped into a log. + expect(issues[0].message).toContain("hook 'case_sla'"); + expect(issues[0].message).toContain('SLA_MATRIX'); + // Says what actually changed. "no behavior change" is true of behaviour and + // false of deployment shape, which is the whole defect. + expect(issues[0].message).toContain('deployment shape'); + }); + + it('keeps the STRUCTURAL refusal a warning — the bundle is its designed answer', () => { + const issues = checkHookBodyLowering({ hooks: [forbiddenTokenHook] }); + + expect(issues).toHaveLength(1); + expect(issues[0].severity).toBe('warning'); + expect(issues[0].rule).toBe(BUNDLED_FALLBACK_RULE); + expect(issues[0].message).toContain("hook 'enrich_lead'"); + // Reported, but never fatal: `os lint` exits 1 on `error` only, so a + // legitimate `fetch()` handler still lints clean-enough to ship. + expect(issues.some((i) => i.severity === 'error')).toBe(false); + }); + + it('says nothing about a handler that really does ship as metadata', () => { + expect(checkHookBodyLowering({ hooks: [selfContainedHook] })).toEqual([]); + }); + + it('truncates the multi-line offending-source dump to one line', () => { + const [issue] = checkHookBodyLowering({ hooks: [forbiddenTokenHook] }); + expect(issue.message).not.toContain('--- offending body source ---'); + expect(issue.message.split('\n')).toHaveLength(1); + }); + + describe('an instrument failure is NOT an author verdict', () => { + // The whole PR exists because a silent downgrade was misattributed. Telling + // an author "you chose a bundled closure" when in fact the TOOL failed is + // the same wrong verdict wearing the fix's clothes — so `unparseable` and + // `unknown` must never land in the `bundled-fallback` arm, whose prose + // asserts "the body uses something the sandbox cannot provide". + + it('reports an unparseable body under its own rule, naming the instrument', () => { + const issues = checkHookBodyLowering({ hooks: [{ ...unparseableHook }] }); + + expect(issues).toHaveLength(1); + expect(issues[0].rule).toBe(UNPARSEABLE_BODY_RULE); + // Never fatal: an instrument limit must not move the exit contract. + expect(issues[0].severity).toBe('warning'); + expect(issues[0].message).toContain('not a verdict about the handler'); + // And never the author-verdict prose of the deliberate-bundle arm. + expect(issues[0].rule).not.toBe(BUNDLED_FALLBACK_RULE); + expect(issues[0].message).not.toContain('the body uses something the sandbox cannot provide'); + }); + + it('reports an extractor throw (kind `unknown`) under its own rule, not as a chosen bundle', () => { + const issues = checkHookBodyLowering({ hooks: [{ ...explodingHook }] }); + + expect(issues).toHaveLength(1); + expect(issues[0].rule).toBe(EXTRACTION_FAILED_RULE); + expect(issues[0].severity).toBe('warning'); + expect(issues[0].message).toContain('the extraction instrument itself failed'); + expect(issues[0].message).toContain('not a verdict about the handler'); + expect(issues[0].rule).not.toBe(BUNDLED_FALLBACK_RULE); + expect(issues[0].message).not.toContain('the body uses something the sandbox cannot provide'); + }); + + it('keeps the two instrument kinds distinct from each other, not just from the verdict arms', () => { + // `unknown` is deliberately not folded into `unparseable` (#13651): a + // broken instrument and a limited instrument are different events. + const [unparseable] = checkHookBodyLowering({ hooks: [{ ...unparseableHook }] }); + const [unknown] = checkHookBodyLowering({ hooks: [{ ...explodingHook }] }); + expect(unparseable.rule).not.toBe(unknown.rule); + }); + }); + + describe('the two ways an author declares "bundle this deliberately"', () => { + it('says nothing about a string handler (already a bundle reference)', () => { + const issues = checkHookBodyLowering({ + hooks: [{ name: 'h', object: 'o', events: ['beforeInsert'], handler: 'some_bundled_fn' }], + }); + expect(issues).toEqual([]); + }); + + it('says nothing when the author supplied an explicit `body`', () => { + // `lowerCallables` only extracts `if (!hook.body)`; this rule mirrors that + // skip, so the opt-out is the same one the build already honours. + const issues = checkHookBodyLowering({ + hooks: [{ ...freeIdentifierHook, body: { language: 'js', source: 'return 1;' } }], + }); + expect(issues).toEqual([]); + }); + + it('never judges a top-level `functions:` entry — that path is never lowered', () => { + const issues = checkHookBodyLowering({ + functions: { my_fn: (ctx: any) => ctx.input.x = SLA_MATRIX.high }, + }); + expect(issues).toEqual([]); + }); + }); + + describe('actions', () => { + it('judges an object action `target` and names its path', () => { + const issues = checkHookBodyLowering({ + objects: [ + { + name: 'case', + actions: [ + { name: 'escalate', target: (ctx: any) => { ctx.out = SLA_MATRIX.high; } }, + ], + }, + ], + }); + expect(issues).toHaveLength(1); + expect(issues[0].severity).toBe('error'); + expect(issues[0].path).toBe('objects[0].actions[0].target'); + expect(issues[0].message).toContain("action 'case_escalate'"); + }); + + it('judges a top-level action `target`', () => { + const issues = checkHookBodyLowering({ + actions: [{ name: 'sweep', target: (ctx: any) => { ctx.out = SLA_MATRIX.low; } }], + }); + expect(issues).toHaveLength(1); + expect(issues[0].path).toBe('actions[0].target'); + expect(issues[0].message).toContain("action 'global_sweep'"); + }); + }); + + /** + * The #3782 class: two surfaces disagreeing about what an author is told. + * This rule and the build both call `extractHookBody` on the same normalized + * input, so the agreement is by construction — this pins that it stays so, + * and would fail the moment someone re-implements the analysis here. + */ + it('judges exactly the callables the build records as extraction warnings', () => { + const config = { + hooks: [ + freeIdentifierHook, + forbiddenTokenHook, + selfContainedHook, + unparseableHook, + explodingHook, + { name: 'string_ref', object: 'o', events: ['beforeInsert'], handler: 'bundled' }, + { ...selfContainedHook, name: 'with_body', body: { language: 'js', source: 'return 1;' } }, + ], + objects: [ + { + name: 'case', + actions: [{ name: 'escalate', target: (ctx: any) => { ctx.out = SLA_MATRIX.high; } }], + }, + ], + }; + + // What the BUILD records (and then bundles anyway, at exit 0). + const lowering = lowerCallables(structuredClone_(config)); + const buildSaw = lowering.bodyExtractionWarnings + .map((w) => `${w.origin}|${w.kind}`) + .sort(); + + // What LINT reports, mapped back through the rule -> kind correspondence. + // Total over all four kinds on purpose: a kind with no row here is a kind + // whose divergent labeling this pin could never catch — which is exactly + // how the `unknown`-folded-into-"designed fallback" defect survived to + // review the first time. + const kindOfRule: Record = { + [NOT_LOWERABLE_RULE]: 'free-identifiers', + [BUNDLED_FALLBACK_RULE]: 'forbidden-token', + [UNPARSEABLE_BODY_RULE]: 'unparseable', + [EXTRACTION_FAILED_RULE]: 'unknown', + }; + const lintSaw = checkHookBodyLowering(structuredClone_(config)) + .map((i) => { + const origin = /^((?:hook|action) '[^']+')/.exec(i.message)?.[1]; + return `${origin}|${kindOfRule[i.rule]}`; + }) + .sort(); + + expect(lintSaw).toEqual(buildSaw); + // And the population is the real one, not an empty set agreeing with itself + // — all four kinds present, the two instrument kinds included. + expect(buildSaw).toEqual([ + "action 'case_escalate'|free-identifiers", + "hook 'case_sla'|free-identifiers", + "hook 'enrich_lead'|forbidden-token", + "hook 'exploding'|unknown", + "hook 'opaque'|unparseable", + ]); + }); +}); + +/** + * `structuredClone` cannot carry functions, and `lowerCallables` mutates only + * shallow clones of what it is handed — so the two passes above must each get a + * fresh object graph without losing the callables. A shallow-enough hand clone + * is exactly that. + */ +function structuredClone_>(v: T): T { + return { + ...v, + ...(Array.isArray(v.hooks) ? { hooks: v.hooks.map((h: any) => ({ ...h })) } : {}), + ...(Array.isArray(v.objects) + ? { + objects: v.objects.map((o: any) => ({ + ...o, + ...(Array.isArray(o.actions) ? { actions: o.actions.map((a: any) => ({ ...a })) } : {}), + })), + } + : {}), + ...(Array.isArray(v.actions) ? { actions: v.actions.map((a: any) => ({ ...a })) } : {}), + }; +} diff --git a/packages/cli/src/lint/hook-body-lowering.ts b/packages/cli/src/lint/hook-body-lowering.ts new file mode 100644 index 0000000000..009c396043 --- /dev/null +++ b/packages/cli/src/lint/hook-body-lowering.ts @@ -0,0 +1,250 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `os lint` rule: a registered handler that cannot be lowered to a + * metadata-only body (#13651). + * + * ## The defect this closes + * + * An L2 hook handler is lowered to a metadata-only `body.source` and evaluated + * in QuickJS with no module scope. When the handler reaches out of that scope, + * `extractHookBody` refuses — and `lowerCallables` catches the refusal, records + * it, and ships the callable through the back-compat `.mjs` bundle instead. + * `os build` exits 0. The hook keeps working locally. What changed silently is + * the DEPLOYMENT SHAPE: the app has stopped being shippable as pure metadata. + * + * The refusal was never the missing piece — `extractHookBody` had already + * computed the exact free-identifier list. What was missing is that nothing + * said NO to the recorded array. This rule is that "no". + * + * ## Why a lint `error` and not a build failure + * + * `os lint` exits 1 on an `error`, so a gate can fail on this — which is what + * the card asked for — WITHOUT changing what `os build` accepts. That + * separation is deliberate and load-bearing: + * + * - `os build`'s warn-and-bundle is a published behaviour that downstream + * apps this repo cannot measure depend on. Flipping its default to a hard + * failure is a contract act, and the in-tree note at `compile.ts` step 2c + * already records that position. + * - `os lint`'s own rubric, by contrast, is explicitly "a lint verdict, not a + * publish gate" (see `lintConfig`), which is exactly the tier this belongs + * in. The shared authoring-rule registry could not host it either way: its + * `gating` tier must be run by all three commands (so it WOULD move the + * build's accept set) and its `advisory` tier can never emit an `error`. + * + * ## What separates an author's mistake from a deliberate bundle + * + * Today both arrive through one catch, so they share one fate. They are not + * the same event, and the refusal kind already tells them apart: + * + * `free-identifiers` ACCIDENTAL. The handler IS expressible as a metadata + * body; it merely names a module-scope const/helper/ + * import. The author wrote something that reads + * self-contained, and the platform quietly re-shaped the + * deployment behind them — its own message even says "no + * behavior change", which is true of behaviour and false + * of shape. Remedy is local: inline the value. => `error`. + * `forbidden-token` STRUCTURAL. `fetch`/`require`/`process`/`eval`/… are + * capabilities the sandbox does not have, so the handler + * can NEVER be a metadata body. Writing one IS choosing a + * bundled closure, and the bundle is the designed answer + * (the refusal text says "declare a Connector recipe + * instead"). Reporting it is right; failing on it would + * punish the legitimate path. => `warning`. + * `unparseable` An instrument LIMIT: the extractor found no body it + * could judge. Not a verdict about the author — reported + * under its own rule, with prose that names the + * instrument, never as the author choosing a bundle. + * => `warning`. + * `unknown` An instrument FAILURE (a non-`HookBodyExtractionError` + * throw — the extractor itself broke). Same discipline, + * its own rule; kept distinct from `unparseable` on + * purpose. => `warning`. + * + * An author who deliberately wants a bundled closure for the ACCIDENTAL class + * is not cornered: two declarative channels already exist and are already + * silent here, and neither needs a new spec key. + * + * 1. Give the hook an explicit `body` — extraction is skipped entirely + * (`lowerCallables` only extracts `if (!hook.body)`). + * 2. Move the function into the top-level `functions:` map and reference it + * from the hook by NAME (`handler` accepts a string). That path registers + * the callable for bundling and never attempts extraction at all — which + * is measurably how three callables in this repo's own examples already + * ship, warning-free. + * + * So the inline-function form means "I intend this to be a hook body" and the + * named-`functions:` form means "I intend this to be bundled code". That + * distinction already existed in the authoring surface; nothing was reading it. + * + * ## Parity + * + * This rule calls the SAME `extractHookBody` that `lowerCallables` calls, on + * the same normalized input. It therefore cannot drift from what `os build` + * would do to the same handler — the #3782 class (two surfaces disagreeing + * about what an author is told) is closed by construction here, not by a + * second implementation kept in sync by hand. + */ + +import { extractHookBody, HookBodyExtractionError } from '../utils/extract-hook-body.js'; + +/** Mirrors `LintIssue` in `../commands/lint.ts` (structurally compatible). */ +export interface HookBodyLintIssue { + severity: 'error' | 'warning' | 'suggestion'; + rule: string; + message: string; + path: string; + fix?: string; +} + +/** The rule name a gate greps for when the handler could have been metadata. */ +export const NOT_LOWERABLE_RULE = 'hook-body/not-lowerable'; +/** The rule name for a refusal whose designed answer is the bundle. */ +export const BUNDLED_FALLBACK_RULE = 'hook-body/bundled-fallback'; +/** The rule name when the extractor found no body it could judge (`unparseable`). */ +export const UNPARSEABLE_BODY_RULE = 'hook-body/unparseable'; +/** The rule name when the extraction instrument itself threw (kind `unknown`). */ +export const EXTRACTION_FAILED_RULE = 'hook-body/extraction-failed'; + +const DELIBERATE_BUNDLE_REMEDY = + 'If a bundled closure is what you want, say so: give the hook an explicit `body`, ' + + 'or move the function into the top-level `functions:` map and reference it by name ' + + '(`handler: \'\'`) — neither is lowered to a metadata body.'; + +const isPlainObject = (v: unknown): v is Record => + typeof v === 'object' && v !== null && !Array.isArray(v); + +type AnyFn = (...args: unknown[]) => unknown; + +/** + * Run `extractHookBody` over one callable and turn a refusal into an issue. + * Returns `null` when the body extracts cleanly — i.e. the callable really does + * ship as metadata. + */ +function judge(fn: AnyFn, originLabel: string, path: string): HookBodyLintIssue | null { + try { + extractHookBody(fn, originLabel); + return null; + } catch (err: unknown) { + const kind = err instanceof HookBodyExtractionError ? err.kind : 'unknown'; + const free = err instanceof HookBodyExtractionError ? err.freeIdentifiers : []; + // Only the first line: the refusal text carries a multi-line offending-source + // dump for `--strict-body`'s per-callable diagnostic, which would swamp a + // lint report. `os build --strict-body` remains the place to read it whole. + const firstLine = String((err as Error)?.message ?? err).split('\n')[0]; + + if (kind === 'free-identifiers') { + return { + severity: 'error', + rule: NOT_LOWERABLE_RULE, + message: + `${originLabel} cannot be lowered to a metadata-only body: it references ` + + `${free.length === 1 ? 'the identifier' : 'identifiers'} ${free.join(', ')}, which ` + + `${free.length === 1 ? 'is' : 'are'} not in scope inside the sandbox. The handler is ` + + `BUNDLED instead, so this app is no longer shippable as pure metadata — a change of ` + + `deployment shape, not of behaviour. Inline the value(s) into the handler, or reach ` + + `them through \`ctx\`. ${DELIBERATE_BUNDLE_REMEDY}`, + path, + }; + } + + if (kind === 'unparseable' || kind === 'unknown') { + // An instrument LIMIT (`unparseable`: the extractor found no body it + // could judge) or an instrument FAILURE (`unknown`: the extractor itself + // threw something that is not a refusal). In both cases the tool is what + // fell short — the event says nothing about what the author chose, so it + // must not borrow the bundled-fallback prose below ("the body uses + // something the sandbox cannot provide"), which asserts a verdict about + // the handler. Kept distinct on purpose (#13651): an instrument failure + // must not read as a verdict about the author. + const [rule, instrument] = + kind === 'unparseable' + ? ([UNPARSEABLE_BODY_RULE, 'the extractor found no function body it could judge'] as const) + : ([EXTRACTION_FAILED_RULE, 'the extraction instrument itself failed'] as const); + return { + severity: 'warning', + rule, + message: + `${originLabel} could not be analysed: ${instrument}. ${firstLine} ` + + `This is a limit of the instrument, not a verdict about the handler — it does NOT mean ` + + `a bundled closure was chosen; whether this body could ship as pure metadata is ` + + `undetermined. \`os build\` still bundles it via the designed fallback.`, + path, + }; + } + + // `forbidden-token` — the only kind left, and the only one whose prose may + // assert the author's choice: the sandbox genuinely cannot provide what the + // body uses, so the bundle is the designed answer. + return { + severity: 'warning', + rule: BUNDLED_FALLBACK_RULE, + message: + `${originLabel} is bundled rather than shipped as a metadata-only body. ${firstLine} ` + + `This is the designed fallback — the body uses something the sandbox cannot provide — ` + + `but the app is not pure metadata while it is here.`, + path, + }; + } +} + +/** + * Every registered callable `lowerCallables` would attempt to extract, judged. + * + * Deliberately mirrors that function's walk (hooks, object actions, top-level + * actions) and its skip conditions (a string handler is already a bundle + * reference; an explicit `body` opts out of extraction). A callable this rule + * does not visit is one `os build` never attempts to lower either. + */ +export function checkHookBodyLowering(config: Record): HookBodyLintIssue[] { + const issues: HookBodyLintIssue[] = []; + + if (Array.isArray(config.hooks)) { + config.hooks.forEach((raw, i) => { + if (!isPlainObject(raw)) return; + if (typeof raw.handler !== 'function') return; // string ref = deliberate bundle + if (raw.body) return; // author supplied the body themselves + const name = + typeof raw.name === 'string' && raw.name.length > 0 ? raw.name : 'anon_hook'; + const issue = judge(raw.handler as AnyFn, `hook '${name}'`, `hooks[${i}].handler`); + if (issue) issues.push(issue); + }); + } + + const judgeActions = (actions: unknown[], ownerLabel: string, pathPrefix: string): void => { + actions.forEach((raw, i) => { + if (!isPlainObject(raw)) return; + if (typeof raw.target !== 'function') return; + if (raw.body) return; + const baseName = + typeof raw.name === 'string' && raw.name.length > 0 + ? `${ownerLabel}_${raw.name}` + : `${ownerLabel}_anon_action`; + const issue = judge( + raw.target as AnyFn, + `action '${baseName}'`, + `${pathPrefix}[${i}].target`, + ); + if (issue) issues.push(issue); + }); + }; + + if (Array.isArray(config.objects)) { + config.objects.forEach((rawObj, oi) => { + if (!isPlainObject(rawObj)) return; + if (!Array.isArray(rawObj.actions)) return; + judgeActions( + rawObj.actions, + String(rawObj.name ?? 'object'), + `objects[${oi}].actions`, + ); + }); + } + + if (Array.isArray(config.actions)) { + judgeActions(config.actions, 'global', 'actions'); + } + + return issues; +} diff --git a/packages/cli/src/utils/extract-hook-body.ts b/packages/cli/src/utils/extract-hook-body.ts index 68d5665359..5416d2304d 100644 --- a/packages/cli/src/utils/extract-hook-body.ts +++ b/packages/cli/src/utils/extract-hook-body.ts @@ -29,11 +29,18 @@ * until #10678, which is the whole defect that card names. * `--strict-body` the same recorded warnings become a hard failure (exit 1) * with a per-callable diagnostic, and nothing is bundled. + * `os lint` (#13651) reads the REFUSAL KIND, not the exit code, and + * gives the two classes different verdicts: an accidental + * scope leak (`free-identifiers`) is a lint `error`, so a + * gate can fail on it; a structural one (`forbidden-token`) + * stays a warning, because bundling is its designed answer. + * `os lint` calls THIS function, so its verdict cannot + * drift from what `os build` would do to the same handler. * * So the allow-list gates what may become `body.source`; it does not (yet) gate * what may build. Closing the L3 `.mjs` path is `--strict-body`'s job today and * Phase 3's later — not this list's. Docs `hook-bodies.mdx` describe the same - * two outcomes; when this header and that page disagree, they are both wrong + * outcomes; when this header and that page disagree, they are both wrong * until one of them is measured over a real `os build`. * * Capability inference: we scan the body for known `ctx.api.*`, `ctx.log.*`, @@ -68,6 +75,69 @@ import { detectFreeIdentifiers } from './detect-free-identifiers.js'; +/** + * WHY a refusal carries a machine-readable kind (#13651). + * + * Every refusal below already KNOWS which rule refused — the rule is what + * produced the sentence. Until this type existed, that knowledge was flattened + * into the message string at the `throw` and never recovered: `lowerCallables` + * caught the error, kept `err.message`, and every consumer downstream had a + * paragraph of English where it needed a category. So the two refusals that + * mean OPPOSITE things to an author shared one undifferentiated fate: + * + * `free-identifiers` the handler IS expressible as a metadata-only body. + * It references a module-scope helper/import/const, so + * the deployment shape silently changed from metadata to + * bundled closure — against what the author wrote. The + * remedy is local and mechanical (inline the value). + * `forbidden-token` the handler is NOT expressible as a metadata-only body + * at ALL. `fetch`/`require`/`process`/… are capabilities + * the QuickJS sandbox does not have, so writing one IS + * choosing a bundled closure. Falling back to the bundle + * is the designed answer, not a degradation to report as + * an error. + * `unparseable` the extractor could not find a body to peel. An + * instrument limit, not an author verdict. + * + * ⛔ The kind is NOT a license to change what `os build` accepts. Both classes + * still fall back to bundling and still exit 0 — see `lowerCallables`, whose + * catch is deliberately kept. What the kind buys is that a consumer can now + * treat the accidental class differently from the structural one; `os lint` is + * the first to do so. + */ +export type HookBodyRefusalKind = 'unparseable' | 'forbidden-token' | 'free-identifiers'; + +/** + * A refusal from {@link extractHookBody}, carrying the classification the + * refusing rule already had. + * + * The `message` is deliberately byte-identical to what this function threw + * before the class existed: `os build`'s warn-and-bundle line, `--strict-body`'s + * per-callable diagnostic and `content/docs/automation/hook-bodies.mdx` all + * quote those sentences, and a refusal that reads differently would be a + * documentation break wearing a refactor's clothes. The class ADDS structure + * beside the prose; it does not restate it. + */ +export class HookBodyExtractionError extends Error { + readonly kind: HookBodyRefusalKind; + readonly originLabel: string; + /** Names the handler referenced but does not bind — `free-identifiers` only. */ + readonly freeIdentifiers: readonly string[]; + + constructor( + kind: HookBodyRefusalKind, + originLabel: string, + message: string, + freeIdentifiers: readonly string[] = [], + ) { + super(message); + this.name = 'HookBodyExtractionError'; + this.kind = kind; + this.originLabel = originLabel; + this.freeIdentifiers = freeIdentifiers; + } +} + 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' }, // Both spellings, one reason (#10678). A TypeScript config is loaded through @@ -142,7 +212,9 @@ export function extractHookBody(fn: (...a: unknown[]) => unknown, originLabel: s // result is a pure block body suitable for `new Function('ctx', body)`. const block = peelToBlockBody(raw); if (!block) { - throw new Error( + throw new HookBodyExtractionError( + 'unparseable', + originLabel, `[hook-body-extract] could not parse the body of ${originLabel}; ` + `please rewrite the handler as a single arrow function or named function expression`, ); @@ -151,7 +223,9 @@ export function extractHookBody(fn: (...a: unknown[]) => unknown, originLabel: s // Reject any forbidden token before we ship the source as metadata. for (const { rx, reason } of FORBIDDEN_PATTERNS) { if (rx.test(block.source)) { - throw new Error( + throw new HookBodyExtractionError( + 'forbidden-token', + originLabel, `[hook-body-extract] ${originLabel}: ${reason}\n` + `--- offending body source ---\n${block.source.slice(0, 400)}${block.source.length > 400 ? '…' : ''}`, ); @@ -167,11 +241,14 @@ export function extractHookBody(fn: (...a: unknown[]) => unknown, originLabel: s // included) is analyzed so parameters are correctly in scope. const { free, unparsed } = detectFreeIdentifiers(raw); if (!unparsed && free.length > 0) { - throw new Error( + throw new HookBodyExtractionError( + 'free-identifiers', + originLabel, `[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\`).`, + free, ); } diff --git a/packages/cli/src/utils/hook-body-refusal-kind.test.ts b/packages/cli/src/utils/hook-body-refusal-kind.test.ts new file mode 100644 index 0000000000..e2d37543e9 --- /dev/null +++ b/packages/cli/src/utils/hook-body-refusal-kind.test.ts @@ -0,0 +1,116 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #13651 — the refusal carries its classification, and the messages did not move. + * + * Two halves, and the second matters as much as the first: `os build`'s + * warn-and-bundle line, `--strict-body`'s per-callable diagnostic and + * `content/docs/automation/hook-bodies.mdx` all quote these sentences, so a + * refactor that "only" reworded them would be a documentation break with a + * green test suite. + */ + +import { describe, it, expect } from 'vitest'; +import { extractHookBody, HookBodyExtractionError } from './extract-hook-body.js'; +import { lowerCallables } from './lower-callables.js'; + +const TERRITORY: Record = { US: 'na', DE: 'eu' }; + +describe('HookBodyExtractionError', () => { + it('classifies a module-scope reference as free-identifiers and names them', () => { + let caught: unknown; + try { + extractHookBody(((ctx: any) => { + ctx.input.territory = TERRITORY[ctx.input.country]; + }) as any, "hook 'x'"); + } catch (err) { + caught = err; + } + + expect(caught).toBeInstanceOf(HookBodyExtractionError); + const err = caught as HookBodyExtractionError; + expect(err.kind).toBe('free-identifiers'); + expect(err.originLabel).toBe("hook 'x'"); + expect(err.freeIdentifiers).toContain('TERRITORY'); + // Message unchanged — the docs quote this sentence. + expect(err.message).toContain('references identifier(s) not in scope at runtime'); + }); + + it('classifies a sandbox-impossible token as forbidden-token', () => { + let caught: unknown; + try { + extractHookBody((async (ctx: any) => { + ctx.out = await fetch('https://example.invalid'); + }) as any, "hook 'y'"); + } catch (err) { + caught = err; + } + + const err = caught as HookBodyExtractionError; + expect(err.kind).toBe('forbidden-token'); + expect(err.freeIdentifiers).toEqual([]); + expect(err.message).toContain('`fetch()` is not allowed in hook/action bodies'); + // The offending-source dump the `--strict-body` diagnostic prints. + expect(err.message).toContain('--- offending body source ---'); + }); +}); + +describe('lowerCallables carries the classification without changing what it does', () => { + const config = () => ({ + hooks: [ + { + name: 'territory', + object: 'account', + events: ['beforeInsert'], + handler: (ctx: any) => { + ctx.input.territory = TERRITORY[ctx.input.country]; + }, + }, + ], + }); + + it('records the kind and the identifiers beside the unchanged reason', () => { + const out = lowerCallables(config()); + + expect(out.bodyExtractionWarnings).toHaveLength(1); + const w = out.bodyExtractionWarnings[0]; + expect(w.origin).toBe("hook 'territory'"); + expect(w.kind).toBe('free-identifiers'); + expect(w.freeIdentifiers).toContain('TERRITORY'); + expect(w.reason).toContain('references identifier(s) not in scope at runtime'); + }); + + it('STILL bundles the callable and still emits no body — the fallback is intact', () => { + // The catch must survive this card. If a later change deletes it, the + // legitimate "author deliberately wants a bundled closure" path dies with + // the accidental one, and this assertion is what notices. + const out = lowerCallables(config()); + + expect(out.count).toBe(1); + expect(out.bodyExtracted).toBe(0); + expect(out.functions.territory).toBeTypeOf('function'); + expect((out.lowered.hooks as any[])[0].handler).toBe('territory'); + expect((out.lowered.hooks as any[])[0].body).toBeUndefined(); + }); + + it('reports kind "unknown" when the extractor itself throws a bare Error', () => { + // Not folded into `unparseable`: an instrument failure must stay + // distinguishable from a verdict about the author's handler. + const exploding = () => { + throw new Error('boom'); + }; + Object.defineProperty(exploding, 'toString', { + value: () => { + throw new Error('cannot stringify'); + }, + }); + + const out = lowerCallables({ + hooks: [{ name: 'boom', object: 'o', events: ['beforeInsert'], handler: exploding }], + }); + + expect(out.bodyExtractionWarnings).toHaveLength(1); + expect(out.bodyExtractionWarnings[0].kind).toBe('unknown'); + expect(out.bodyExtractionWarnings[0].freeIdentifiers).toEqual([]); + }); +}); diff --git a/packages/cli/src/utils/lower-callables.ts b/packages/cli/src/utils/lower-callables.ts index 8d96f8e043..933a484fb9 100644 --- a/packages/cli/src/utils/lower-callables.ts +++ b/packages/cli/src/utils/lower-callables.ts @@ -21,7 +21,32 @@ * emitting it. */ -import { extractHookBody } from './extract-hook-body.js'; +import { extractHookBody, HookBodyExtractionError, type HookBodyRefusalKind } from './extract-hook-body.js'; + +/** + * One recorded extraction refusal. + * + * `origin` and `reason` are unchanged — `os build`'s warn-and-bundle line, + * `--strict-body`'s diagnostic and the `--json` payload all read them. + * `kind` (#13651) is the classification the refusing rule already had and used + * to throw away at the catch below; `freeIdentifiers` is the list + * `detectFreeIdentifiers` had already computed. Both are ADDITIVE: nothing here + * changes which callables are bundled, or the exit code. + * + * `kind` is `'unknown'` only for a throw that is not a + * {@link HookBodyExtractionError} — i.e. a bug in the extractor rather than a + * verdict about the author's handler. It is deliberately NOT folded into + * `'unparseable'`: a consumer must be able to tell "the extractor refused this + * handler" from "the extractor itself fell over", and silently filing the + * second as the first is how an instrument failure gets read as an author + * verdict. + */ +export interface BodyExtractionWarning { + origin: string; + reason: string; + kind: HookBodyRefusalKind | 'unknown'; + freeIdentifiers: readonly string[]; +} export interface LoweringResult { /** A deep-cloned, JSON-safe copy of the stack with handlers replaced by strings. */ @@ -33,7 +58,7 @@ export interface LoweringResult { /** Number of handlers that successfully emitted a metadata-only `body`. */ bodyExtracted: number; /** Per-extraction failures (still emit handler ref + bundle, but warn). */ - bodyExtractionWarnings: Array<{ origin: string; reason: string }>; + bodyExtractionWarnings: BodyExtractionWarning[]; } type AnyFn = (...args: unknown[]) => unknown; @@ -57,7 +82,7 @@ function uniqueName(base: string, taken: Set): string { export function lowerCallables(input: Record): LoweringResult { const functions: Record = {}; const taken = new Set(); - const warnings: Array<{ origin: string; reason: string }> = []; + const warnings: BodyExtractionWarning[] = []; let bodyExtracted = 0; // Try to extract a metadata-only body from a callable. Returns null if the @@ -72,7 +97,17 @@ export function lowerCallables(input: Record): LoweringResult { bodyExtracted += 1; return { language: 'js', source: ext.source, capabilities: ext.capabilities }; } catch (err: any) { - warnings.push({ origin: originLabel, reason: err?.message ?? String(err) }); + // ⛔ The catch STAYS. Deleting it would make every refusal fatal and take + // the LEGITIMATE fallback-to-bundling path down with the accidental one — + // the two share this catch, which is exactly why the refusal now arrives + // classified. Telling them apart is the consumer's job (`os lint`), not + // this function's: lowering keeps bundling both, at exit 0, unchanged. + warnings.push({ + origin: originLabel, + reason: err?.message ?? String(err), + kind: err instanceof HookBodyExtractionError ? err.kind : 'unknown', + freeIdentifiers: err instanceof HookBodyExtractionError ? err.freeIdentifiers : [], + }); return null; } }