From 445c306e513919d85de084fa77c9201bfe74bc6d Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Fri, 31 Jul 2026 16:37:26 +0800 Subject: [PATCH] =?UTF-8?q?docs(hook-bodies):=20the=20write=20set=20is=20s?= =?UTF-8?q?tatically=20checked=20now=20=E2=80=94=20rewrite=20the=20section?= =?UTF-8?q?=20#4305/#4344=20made=20false=20(#4351)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `content/docs/automation/hook-bodies.mdx` claimed the write side had **no** static checking at all, and called it an "accepted gap with no planned closure". Since #4305 (`validateHookBodyWrites`) and #4344 (the action-side sibling) that is false: `hook-body-write-unknown-field` and `action-body-write-unknown-field` parse an L2 body, resolve its literal writes against the target object's declared fields, and warn with a did-you-mean. The mirror image of "declared ≠ enforced": here the capability shipped and the docs still said it had not. An author who drops a real guardrail because the docs said it does not exist pays the same price as one who trusts a guardrail that does not. - "Not statically checked: the write set" → "Write-set checking". Coverage described accurately: the three literal patterns (tabulated per surface, since an action's `ctx.input` is its params bag and only `api-crud-literal` carries over), advisory `warning` that never gates, and the shapes the parser must skip — computed keys, spreads, dynamic object names, wildcard `ctx.input`, partial multi-target hits, cross-package targets, aliased input — so the ABSENCE of a warning is not read as proof of correctness. - #3700 (structured `writes`, closed as not planned) stays as accurate history, but the "accepted gap" conclusion is retracted: the gap was closed from the other end, by parsing the write set out of the source. - Three now-stale cross-links in `hooks.mdx` follow the renamed anchor. The wildcard bullet gets sharper rather than deleted — a wildcard hook's `ctx.input` writes are exactly the shape the lint must skip. Two claims were corrected against measured behaviour rather than restated: - **Flow `update_record` has no unknown-field check.** The old advice ("prefer a flow node, they get the static checking hook bodies can't") is now backwards on this axis. Verified on main: for the same undeclared field, the hook body and action body each warn and the `update_record` node reports nothing — `validateReadonlyFlowWrites` walks its `fields` but skips unknown ones by design (`if (!meta) continue`). The recommendation is kept for what is still true (structured diffing, the gating `flow-update-readonly-field`) with the gap named. - **"Silently never lands" is wrong in both directions.** The lint's own message says the unknown column quietly vanishes. Measured: nothing strips it — `applyMutationsToInput` is a plain `Object.assign`, `validateRecord` walks declared fields only — so it reaches the driver. On SQL the whole write FAILS (`table deal has no column named stagee`, zero rows stored); on memory/ MongoDB the stray key IS persisted. The doc's runtime paragraph was right and is kept, now split by driver family. The lint's wording is tracked separately. Docs only — no behaviour change, empty changeset. Co-Authored-By: Claude Opus 5 --- .changeset/hook-bodies-write-set-docs.md | 4 +++ content/docs/automation/hook-bodies.mdx | 41 ++++++++++++++++++++---- content/docs/automation/hooks.mdx | 23 +++++++------ 3 files changed, 53 insertions(+), 15 deletions(-) create mode 100644 .changeset/hook-bodies-write-set-docs.md diff --git a/.changeset/hook-bodies-write-set-docs.md b/.changeset/hook-bodies-write-set-docs.md new file mode 100644 index 0000000000..3568487494 --- /dev/null +++ b/.changeset/hook-bodies-write-set-docs.md @@ -0,0 +1,4 @@ +--- +--- + +docs(hook-bodies): the write-set section said the write side had no static checking, which #4305/#4344 made false (#4351) — rewritten as an accurate coverage description (three literal patterns, advisory `hook-body-write-unknown-field` / `action-body-write-unknown-field`, and the shapes the parser must skip, so a missing warning is not read as proof of correctness), the "accepted gap with no planned closure" conclusion retracted (#3700 stays accurate history; the gap was closed from the other end, by parsing the write set), the runtime paragraph corrected against measured behaviour, and the three now-stale cross-links in `hooks.mdx` updated to the renamed anchor. Releases nothing. diff --git a/content/docs/automation/hook-bodies.mdx b/content/docs/automation/hook-bodies.mdx index 3902b7dc20..00514d611d 100644 --- a/content/docs/automation/hook-bodies.mdx +++ b/content/docs/automation/hook-bodies.mdx @@ -116,20 +116,49 @@ The CLI builder **rejects** any source that uses: 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.) -### Not statically checked: the write set +### Write-set checking Static validation around a hook is asymmetric, and it is worth knowing exactly where the line is: - **Checked — read side.** `hook.condition` is validated at build time against the target object's fields by the expression validator (`@objectstack/lint`), including array-valued `hook.object` targets. A condition referencing a nonexistent field fails the lint. - **Checked — capability side.** `body.capabilities` gates which `ctx` APIs the body may call at all; the sandbox throws on an undeclared call. -- **Not checked — write side.** Nothing validates *which fields* your body writes. `body.source` is an opaque JS string: the fields it assigns (`ctx.input.foo = …`) or writes cross-object (`ctx.api.object('x').update(id, { foo })`) are not represented anywhere in metadata, so no lint can verify they exist. This is an **accepted gap** with no planned closure — a structured `writes` declaration was considered and dropped ([#3700](https://github.com/objectstack-ai/objectstack/issues/3700), closed as not planned). +- **Checked — write side, advisory and literal-only.** Since [#4271](https://github.com/objectstack-ai/objectstack/issues/4271), `body.source` is **parsed** (never executed, never type-checked) and the field names it writes are resolved against the target object's declarations. An unknown field raises `hook-body-write-unknown-field` — a **warning** carrying a did-you-mean suggestion, which never blocks a build. Action bodies get the same check on their `ctx.api` writes (`action-body-write-unknown-field`). Both run under `os validate`, `os lint` and `os compile`. -Writing a field the target object doesn't have is not caught at runtime either — the write-path validator skips unknown keys. On a SQL driver the stray column reaches the database and the whole operation fails with a driver-level error, far from the authoring mistake; a schemaless driver (memory, MongoDB) silently persists the stray key. +Three literal write shapes are recognized, and only these: -Because the write set carries no static checking: +| Write shape | Hook body | Action body | +|---|---|---| +| `ctx.input. = …` / `ctx.input[''] ⟨op⟩= …` (including `+=`, `??=`, …) | checked | not checked — an action's `ctx.input` is its **params bag**, not a record | +| `Object.assign(ctx.input, { : … })` | checked | not checked — same surface | +| `ctx.api.object('').insert\|create\|update({ : … })`, `.updateById(id, { : … })` | checked | checked | + +**A missing warning is not a clean bill of health.** The rule bails *silently* on everything it cannot resolve statically, deliberately preferring a missed finding to a false one — a false positive kills an advisory lint, while a miss just leaves the gap open a little longer: + +- computed keys (`ctx.input[k] = …`), spreads, and non-literal payloads; +- dynamic object names (`ctx.api.object(name)`); +- `ctx.input` writes in a wildcard (`object: '*'`) hook — there is no single target to resolve against; +- multi-target hooks where the field exists on *some* target: a body may legitimately branch per object, so only a field missing on **every** named target is flagged; +- objects declared by another package; +- aliased input (`const doc = ctx.input; doc.x = 1`) — v1 does no data-flow analysis. + +System/audit columns and the flat-input envelope keys (`id`, `options`, `ast`, `data`) are never flagged. + +A structured `writes` declaration was considered and dropped ([#3700](https://github.com/objectstack-ai/objectstack/issues/3700), closed as not planned) — but the gap it left was closed from the other end, by parsing the write set out of the source. The practical consequence of that route: coverage is bounded by what a parser can see, not by what an author remembered to declare. + +#### What still happens at runtime + +An unknown field is **not** caught at runtime, and it does not fail quietly either. The write-path validator walks the object's *declared* fields, so an undeclared key is neither rejected nor stripped, and the sandbox's mutations are copied back onto the payload verbatim. What happens next is the driver's call: + +- **SQL drivers** put the stray column into the statement, so the **whole write fails** with a driver-level error (`table deal has no column named stagee`) — nothing is stored, and the error surfaces far from the authoring mistake. +- **Schemaless drivers** (memory, MongoDB) silently persist the stray key alongside the real ones. + +Neither outcome is the one you wanted, and the advisory warning is the earliest signal you get. + +Because the checking is advisory and literal-only: -- **Check your write targets by hand.** Every field the body assigns must exist on the target object(s) — for an array or `"*"` hook, on every target. -- **Prefer a flow `update_record` node when the write set is fixed.** Flow nodes declare their writes structurally in `fields`, so they get the static checking hook bodies can't. +- **Treat `hook-body-write-unknown-field` as a build failure by convention.** It does not gate, but the rule is tuned for near-zero false positives — in practice a warning is a real typo. +- **Check by hand what the parser cannot see.** Computed keys, spreads, aliased input and dynamic object names are invisible to the rule; for an array or `"*"` hook, every field must exist on every target. +- **Prefer a flow `update_record` node when the write set is fixed — but not for *this* check.** A flow node's writes are structured config: they diff field-by-field, render in the Console designer, and a write to a `readonly:true` field is a **gating error** (`flow-update-readonly-field`) that hooks have no counterpart for. What an `update_record` node does *not* get today is a field-existence check — writing a field the object never declares is currently reported by nothing at all. On that one axis an L2 body is now the better-checked surface. - **Exercise the hook against a real object before shipping** — on SQL drivers the mistake surfaces on the first write; schemaless drivers won't tell you. ### Signature conventions diff --git a/content/docs/automation/hooks.mdx b/content/docs/automation/hooks.mdx index e4a466ce1c..8446f53c85 100644 --- a/content/docs/automation/hooks.mdx +++ b/content/docs/automation/hooks.mdx @@ -33,12 +33,15 @@ cannot express. Two structural reasons to prefer the flow when either could work: -- **A flow's writes are validatable metadata; a hook body is opaque code.** An +- **A flow's writes are structured metadata; a hook body is code.** An `update_record` node's `fields` is structural config that `os validate` checks — readonly targets, template dialects, declared expression slots. A - hook body's write set is **not** statically checked (see - [Hook & Action Bodies](/docs/automation/hook-bodies#not-statically-checked-the-write-set)): - a misspelled field name runs green and writes nothing. + hook body's write set is checked too, but only for the literal patterns a + parser can recognise and only as an advisory warning (see + [Hook & Action Bodies](/docs/automation/hook-bodies#write-set-checking)). + Note the one axis where this reverses: writing a field the target object never + declares warns in a hook body, and is reported nowhere in an `update_record` + node. - **A flow reviews as data.** The node graph diffs field-by-field and renders in the Console designer with per-node run history; a hook body reviews as code only. @@ -84,9 +87,10 @@ a hook can be, so review it as such: belongs behind a `condition` (evaluated before the handler) rather than an early `return` inside the body. - **Reads and writes both widen.** A wildcard hook holding an L2 body with - `api.write` can write anywhere. Whether the fields it writes exist is - [not statically checked](/docs/automation/hook-bodies#not-statically-checked-the-write-set), - so a wildcard write set is the least verifiable shape in the system. + `api.write` can write anywhere. Its `ctx.input` writes are also the one shape + the [write-set lint](/docs/automation/hook-bodies#write-set-checking) has to + skip — with no single target object there is nothing to resolve field names + against — so a wildcard write set is the least verifiable shape in the system. Where a wildcard is the honest answer, say so in `description` — it is the one place a reviewer can find out why the broad target was chosen. @@ -179,8 +183,9 @@ ctx = { - Use `ctx.api.object('x')` for cross-object access so writes stay in scope - Handle errors gracefully - Verify every field your hook writes exists on the target object(s) — the - write set of a hook body is **not statically validated**, unlike `condition` - (see [Hook & Action Bodies](/docs/automation/hook-bodies#not-statically-checked-the-write-set)) + write-set lint catches the literal cases, but it is advisory, and computed + keys, spreads and aliased input are invisible to it (see + [Hook & Action Bodies](/docs/automation/hook-bodies#write-set-checking)) ❌ **DON'T:** - Query in loops