Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 54 additions & 0 deletions .changeset/hook-body-gate-reporting-honesty.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
---
"@objectstack/cli": patch
---

Make the hook-body build gates report only what they establish (#10678). Three
defects, one shape — a gate reporting something it never established. The
enforcement net was never the gap and is unchanged: no forbidden body ever
shipped as `body.source`, and every forbidden or free-identifier hook is still
refused under `--strict-body`, at the same exit codes as before.

**The default build no longer warn-and-bundles in silence.** A hook body
containing a forbidden pattern made `os build` exit 0 with no output at all: the
extraction failure was recorded in `bodyExtractionWarnings` and then printed
nowhere, so the only way to learn a handler had *not* become a metadata body was
to diff the artifact. The recorded warnings now reach a human — on stdout,
naming the hook and the pattern, with a pointer at `--strict-body` — and in
`--json` under a new `bodyExtractionWarnings` key. That key is separate from
`warnings` on purpose: `warnings` carries author-time rule advisories in the
shape `os validate --json` also reports, and these are a different record
(`{origin, reason}`). It is an empty array on a clean build, so a CI consumer can
read it unconditionally.

The build still exits 0 in this case. Making a forbidden pattern fatal by default
would change what `os build` accepts and is not part of this change.

**The `require()` refusal reason now fires on the real authoring path.** A
TypeScript config is loaded through `bundle-require` → esbuild, whose ESM interop
shim rewrites `require('node:os')` to `__require("node:os")` before `String(fn)`
runs — so the `require()`-specific reason could never match, and the refusal
arrived instead as the generic free-identifier message naming `__require`, an
identifier the author never typed. Both spellings now carry the one reason, which
also explains the rewrite. Accept behaviour is unchanged: the body was already
refused, already bundled, at the same exit code; only the wording moved.

**The `// @capabilities` directive is documented at its real reach.** It is read
off `String(fn)`, and esbuild strips `//` line comments before the handler is ever
a runtime function — so through `os build` it reaches the extractor from no
ordinary authoring shape. Measured on all four: `objectstack.config.ts`, `.js`,
`.mjs`, and a handler imported from a local `./handlers.js` all silently drop it
and ship the inferred capabilities alone. `hook-bodies.mdx` and the extractor
header now say so, and point at `body.capabilities` — data rather than a comment —
as the escape hatch that does survive. Whether the directive should gain a real
authorable surface or be retired is left open.

The extractor header claimed a forbidden pattern "makes the build **fail** …
no silent fallback"; docs described warn-and-bundle. The code agreed with the
docs, so the header was the outlier and has been rewritten to describe both
outcomes.

A new `os build`-level test (`hook-body-build-reach.e2e.test.ts`) spawns the real
CLI and pins all three behaviours against the artifact and the shell's exit code.
The existing extractor unit tests could not have caught any of this: they feed raw
JS function literals, which keep their comments and their `require(` spelling
because nothing transformed them.
39 changes: 37 additions & 2 deletions content/docs/automation/hook-bodies.mdx
Original file line numberDiff line numberDiff line change
Expand Up@@ -253,9 +253,11 @@ If you have a body that genuinely cannot be expressed in L1+L2 (typically: it ne
2. For each inline handler, take its source via `String(fn)` (the callable is already loaded by tsx/esbuild).
3. Run a regex allow-list over the stringified body (see "What the sandbox forbids" above).
4. **Pass:** emit `body: { language: 'js', source: <body>, capabilities: <inferred> }`.
5. **Forbidden token (default):** extraction fails, a `bodyExtractionWarning` is recorded, and the callable still ships via the back-compat handler-ref bundle — the build does **not** abort. Pass `objectstack compile --strict-body` to turn extraction warnings into a hard build failure (exit 1) with a per-callable diagnostic, e.g. `hook 'normalize_account': fetch() is not allowed in hook/action bodies — declare a Connector recipe instead.`
5. **Forbidden token (default):** extraction fails, a `bodyExtractionWarning` is recorded, and the callable still ships via the back-compat handler-ref bundle — the build does **not** abort. The warning is **printed** (and carried in `--json` under `bodyExtractionWarnings`), so a forbidden pattern is a visible warn-and-bundle rather than a silent success; before #10678 it was recorded and shown to nobody. Pass `objectstack compile --strict-body` to turn extraction warnings into a hard build failure (exit 1) with a per-callable diagnostic, e.g. `hook 'normalize_account': fetch() is not allowed in hook/action bodies — declare a Connector recipe instead.`

Capabilities are inferred by matching known patterns in the body source (e.g. `ctx.api.object(...).insert(...)` ⇒ `api.write`). A fuller AST-based analysis is planned for a later version. You can override with a directive comment when the inference is wrong.
Note that a CommonJS `require('node:os')` in a TypeScript config reaches the extractor as esbuild's `__require("node:os")`. Both spellings are refused under the same `require()` reason.

Capabilities are inferred by matching known patterns in the body source (e.g. `ctx.api.object(...).insert(...)` ⇒ `api.write`). A fuller AST-based analysis is planned for a later version. When the inference is wrong, supply `body` yourself with an explicit `capabilities` array — the `// @capabilities` directive comment [does not reach the build](#capability-inference).

## Migration

Expand DownExpand Up@@ -326,6 +328,39 @@ handler: async (ctx) => {
}
```

<Callout type="warn" title="This directive does not reach `objectstack build`">
The override is read off the handler's **stringified source** (`String(fn)`), so it
only works if the function the CLI holds still carries the comment. Through
`objectstack build`, it never does.

`loadConfig` runs your config through `bundle-require` → esbuild, and esbuild strips
`//` line comments before the handler is ever a runtime function. The directive is
gone by the time the extractor looks at it. Measured on all four authoring shapes:

| Authoring shape | `// @capabilities api.write log` reaches the extractor? |
|---|---|
| `objectstack.config.ts` | **No** — comment stripped by esbuild |
| `objectstack.config.js` | **No** — esbuild runs on `.js` too |
| `objectstack.config.mjs` | **No** — same path |
| handler imported from a local `./handlers.js` | **No** — esbuild bundles it as well |

In every case the build exits 0, prints nothing, and emits the **inferred**
capabilities only. A handler asking for `api.write log` whose body reads
`ctx.api.object('x').find({})` ships `"capabilities": ["api.read"]` — inference won,
silently, and the directive had no effect at all.

**So do not rely on this directive.** Write the body so the
[inference table](#capability-inference) above derives what you need, or supply
`body` yourself on the hook with an explicit `capabilities` array — that path is
data, not a comment, and survives the build.

Inference is unaffected: it matches the *code*, which esbuild keeps. Only the
comment-borne override is lost. Tracked in #10678, where the question of whether the
directive should get a real authorable surface (or be retired) is open — this page
documents the reach as measured, and an `objectstack build`-level test pins it so
this page and the extractor cannot drift apart again.
</Callout>

### Build pipeline at a glance

```
Expand Down
45 changes: 45 additions & 0 deletions packages/cli/src/commands/compile.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -148,6 +148,35 @@ export default class Compile extends Command {
}
}

// 2c. [#10678] SURFACE the warn-and-bundle. The default (non-`--strict-body`)
// build catches every extraction failure in `lowerCallables`, records it
// in `bodyExtractionWarnings`, ships the callable through the .mjs bundle
// and exits 0. That last part is correct and stays correct — flipping the
// default to a hard failure would change what `os build` ACCEPTS, which is
// not this change. What was wrong is that the recorded warnings reached
// nobody: a hook body containing `fetch()` produced a completely silent
// success, and the only way to learn the handler had NOT become a metadata
// body was to diff the artifact. The data already existed; it just never
// got printed. Advisory only — nothing below may touch the exit code.
//
// The reason strings carry a multi-line `--- offending body source ---`
// dump for `--strict-body`'s per-callable diagnostic; on the default path
// we print the first line and point at the flag for the rest, so the
// default build stays readable while staying honest.
if (lowering.bodyExtractionWarnings.length > 0 && !flags.json) {
const n = lowering.bodyExtractionWarnings.length;
console.log('');
printWarning(
`${n} handler${n === 1 ? '' : 's'} could not be lowered to a metadata body — ` +
`bundled via the legacy runtime module instead (build still succeeds)`,
);
for (const w of lowering.bodyExtractionWarnings.slice(0, 20)) {
console.log(` • ${w.origin}: ${String(w.reason).split('\n')[0]}`);
}
if (n > 20) console.log(chalk.dim(` … and ${n - 20} more`));
console.log(chalk.dim(' → run `os build --strict-body` for the full diagnostic, or to make this fatal'));
}

// 3. Validate the lowered (JSON-safe) stack against the Protocol.
if (!flags.json) printStep('Validating protocol compliance...');
const result = ObjectStackDefinitionSchema.safeParse(lowering.lowered);
Expand DownExpand Up@@ -435,6 +464,15 @@ export default class Compile extends Command {
// reports. This key used to carry the widget rule's warnings alone —
// one gate out of the twenty-odd that raise them.
warnings: ruleAdvisories,
// [#10678] Body-extraction failures that made a callable fall back to
// the legacy .mjs bundle. A SEPARATE key on purpose: `warnings` above
// is author-time RULE advisories (`{where,message,rule,path,hint}`)
// and is the shape `os validate --json` shares — folding a different
// record shape (`{origin,reason}`) into it would break that contract
// for every consumer that reads one shape from either command. Empty
// array when every callable lowered cleanly, so a CI consumer can read
// the key unconditionally.
bodyExtractionWarnings: lowering.bodyExtractionWarnings,
// Same key `os validate --json` uses, so a CI consumer reads one shape
// from either command rather than learning two.
conversions: conversionNotices,
Expand All@@ -451,6 +489,13 @@ export default class Compile extends Command {
if (ruleAdvisories.length > 0) {
printWarning(`${ruleAdvisories.length} author-time warning(s) — see above`);
}
if (lowering.bodyExtractionWarnings.length > 0) {
// [#10678] Repeat the tally in the summary: the detail printed before the
// parse, and a long build scrolls it away.
printWarning(
`${lowering.bodyExtractionWarnings.length} handler(s) bundled instead of lowered to a metadata body — see above`,
);
}
console.log('');
printMetadataStats(stats);
console.log('');
Expand Down
58 changes: 51 additions & 7 deletions packages/cli/src/utils/extract-hook-body.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -13,14 +13,43 @@
*
* For v1 we apply a deliberately simple **regex allow-list** over the
* extracted body — full TypeScript AST analysis is deferred to v2. Anything
* the regex rejects (top-level `import`, `require(`, `fetch(`, `process.*`,
* `globalThis.*`, `eval`, `new Function`) makes the build **fail**. There is
* no silent fallback to the L3 .mjs path because that path is being closed.
* the regex rejects (top-level `import`, `require(` / esbuild's `__require(`,
* `fetch(`, `process.*`, `globalThis.*`, `eval`, `new Function`) makes
* extraction **throw**.
*
* ⚠️ What that throw costs the BUILD depends on the flag, and the two outcomes
* are not the same one. This header used to claim only the second (#10678):
*
* default `os build` {@link lowerCallables} catches the throw, records it in
* `bodyExtractionWarnings`, and ships the callable through
* the back-compat `.mjs` bundle instead. No forbidden body
* is ever emitted as `body.source` — but the build exits
* **0**. `compile.ts` prints the recorded warnings, so
* warn-and-bundle is at least not silent; it was silent
* 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.
*
* 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
* until one of them is measured over a real `os build`.
*
* Capability inference: we scan the body for known `ctx.api.*`, `ctx.log.*`,
* `ctx.crypto.*` access patterns and add the matching capability tokens to
* `body.capabilities` automatically. Authors can still override by setting
* `// @capabilities api.read api.write` as the first line of the function.
* `body.capabilities` automatically.
*
* ⚠️ REACH of the `// @capabilities api.read api.write` override (#10678): it is
* read off `String(fn)`, so it survives only when the LOADED config still has
* the comment. A TypeScript `objectstack.config.ts` does not — `loadConfig`
* runs it through `bundle-require` -> esbuild, which strips `//` line comments
* before the handler is ever a runtime function — so through `os build` the
* directive reaches this code from **pre-bundled JS that preserved its comments
* only**. Every unit test below feeds a raw JS function literal and therefore
* cannot see that: they are why the override read as working for so long. The
* real reach is measured over a spawned `os build` in
* `test/hook-body-build-reach.e2e.test.ts` — change the reach, change that test.
*
* Self-containment (#1876): a handler that references a module-scope identifier
* (helper, import, top-level const) cannot be shipped body-only — the reference
Expand All@@ -32,7 +61,16 @@ import { detectFreeIdentifiers } from './detect-free-identifiers.js';

const FORBIDDEN_PATTERNS: Array<{ rx: RegExp; reason: string }> = [
{ rx: /\bimport\s*[\(\*\{]/, reason: 'dynamic `import()` and ES imports are not allowed in hook/action bodies — declare a Connector recipe instead' },
{ rx: /\brequire\s*\(/, reason: '`require()` is not allowed in hook/action bodies' },
// Both spellings, one reason (#10678). A TypeScript config is loaded through
// `bundle-require` -> esbuild, whose ESM interop shim rewrites a CommonJS
// `require('node:os')` into `__require("node:os")` BEFORE `String(fn)` ever
// runs. Matching only the source spelling made this reason UNREACHABLE from
// the real authoring path: the refusal still fired, but through the #1876
// free-identifier gate, naming `__require` — an identifier the author never
// typed and cannot act on. Accept behaviour is unchanged either way (the body
// was already refused); what changes is that the reason now names what was
// written. `\b(?:__)?` cannot widen to `myrequire(` — no word boundary there.
{ rx: /\b(?:__)?require\s*\(/, reason: '`require()` is not allowed in hook/action bodies (esbuild rewrites it to `__require()` when the config is TypeScript; both spellings are refused)' },
{ rx: /\bfetch\s*\(/, reason: '`fetch()` is not allowed in hook/action bodies — declare a Connector recipe instead' },
{ rx: /\bprocess\s*\./, reason: '`process` access is not allowed in hook/action bodies' },
{ rx: /\bglobalThis\s*\./, reason: '`globalThis` access is not allowed in hook/action bodies' },
Expand All@@ -59,7 +97,11 @@ const CAPABILITY_PATTERNS: Array<{ rx: RegExp; cap: 'api.read' | 'api.write' | '
export interface ExtractedBody {
/** Pure function-body source (without the surrounding `(ctx) => {...}`). */
source: string;
/** Inferred capability tokens — may be merged with explicit `// @capabilities` line. */
/**
* Inferred capability tokens — merged with an explicit `// @capabilities`
* line when one survives into `String(fn)`. See the REACH note in this
* file's header: through `os build` on a TS config, it does not.
*/
capabilities: Array<'api.read' | 'api.write' | 'crypto.uuid' | 'log'>;
/** True when source is a single expression (arrow with implicit return). */
isExpression: boolean;
Expand DownExpand Up@@ -115,6 +157,8 @@ export function extractHookBody(fn: (...a: unknown[]) => unknown, originLabel: s
}

// Honour an explicit override: `// @capabilities api.read api.write`.
// Reachable only when the caller handed us a function whose source still
// carries `//` comments — see the REACH note in this file's header (#10678).
const overrideMatch = block.source.match(/^[\s\n]*\/\/\s*@capabilities\s+([a-z.\s]+)/m);
if (overrideMatch) {
const tokens = overrideMatch[1].split(/\s+/).filter(Boolean);
Expand Down
28 changes: 28 additions & 0 deletions packages/cli/test/extract-hook-body.test.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -69,6 +69,34 @@ describe('extractHookBody', () => {
expect(() => extractHookBody(fn, 'hook bad')).toThrow(/fetch/);
});

// #10678 — BOTH spellings, one reason. A TS config never reaches the
// extractor spelling `require(`: `loadConfig` runs it through bundle-require
// -> esbuild, whose ESM interop shim rewrites it to `__require("node:os")`
// first. Matching only the source spelling left the promised require()-reason
// unreachable from the real authoring path — the refusal still fired, but as
// the generic #1876 free-identifier message naming an identifier the author
// never typed. These two cases are written with the call built at runtime so
// the test file itself is not rewritten by its own bundler.
it('rejects require() — the spelling the author writes', () => {
const fn = new Function('ctx', "const os = require('node:os'); return os;") as (...a: unknown[]) => unknown;
expect(() => extractHookBody(fn, 'hook bad')).toThrow(/`require\(\)` is not allowed/);
});

it('rejects __require() — the spelling esbuild leaves behind (#10678)', () => {
const fn = new Function('ctx', 'const os = __require("node:os"); return os;') as (...a: unknown[]) => unknown;
// The require()-specific reason, NOT the free-identifier fallback. If this
// ever reads "not in scope at runtime" again, the reason went unreachable.
expect(() => extractHookBody(fn, 'hook bad')).toThrow(/`require\(\)` is not allowed/);
expect(() => extractHookBody(fn, 'hook bad')).not.toThrow(/not in scope at runtime/);
});

it('does NOT widen to an identifier merely ending in `require` (#10678)', () => {
// `\b(?:__)?require\s*\(` has no word boundary inside `myrequire`, so the
// pattern cannot swallow an author's own helper. Guards the widening.
const fn = new Function('ctx', 'return ctx.myrequire ? 1 : 0;') as (...a: unknown[]) => unknown;
expect(() => extractHookBody(fn, 'hook ok')).not.toThrow();
});

it('rejects process access', () => {
const fn = (_ctx: any) => {
const env = (process as any).env.X;
Expand Down
Loading
Loading