diff --git a/.changeset/action-body-write-set-lint.md b/.changeset/action-body-write-set-lint.md index 8c2b839713..711c799422 100644 --- a/.changeset/action-body-write-set-lint.md +++ b/.changeset/action-body-write-set-lint.md @@ -10,8 +10,10 @@ that carries one. An action body is the same artefact: the same `HookBodySchema` union, parsed by the same `HookBodySchema.safeParse` in `actionBodyRunnerFactory`, run in the same QuickJS sandbox. So it fails the same way — `ctx.api.object('crm_deal').update({ stag: 'won' })` inside an -action succeeds, returns success to the caller, and the unknown column simply -never lands. Half the surface was still blind. +action reaches the driver unfiltered, and the outcome splits by driver: on SQL +the stray column fails the whole call with a driver-level error far from the +authoring site, and on a schemaless driver the stray key is persisted. Half +the surface was still blind. **New rule — `action-body-write-unknown-field` (advisory).** Wired into `REFERENCE_INTEGRITY_RULES`, so `os validate`, `os lint` and `os compile` all diff --git a/.changeset/body-write-lint-message-driver-truth.md b/.changeset/body-write-lint-message-driver-truth.md new file mode 100644 index 0000000000..2e47555c5c --- /dev/null +++ b/.changeset/body-write-lint-message-driver-truth.md @@ -0,0 +1,41 @@ +--- +"@objectstack/lint": patch +"@objectstack/spec": patch +--- + +fix(lint): the write-set diagnostics describe what the runtime actually does (#4271) + +`hook-body-write-unknown-field` and `action-body-write-unknown-field` told +authors the undeclared column "silently never lands in the stored record". +Measured on `main`, that is wrong in **both** directions. Nothing between the +body and the driver filters the key — `applyMutationsToInput` is a plain +`Object.assign`, and `validateRecord` walks declared fields on insert and +`continue`s past a key with no field def on update — so the driver decides: + +- **SQL** — the stray column enters the statement and the **whole write + fails** with a driver-level error (`table deal has no column named stagee`). + Nothing is stored, so the correctly-spelled fields of that row are lost too, + and the error names a column far from the body that wrote it. +- **Schemaless** (memory, MongoDB — both spread the payload without consulting + the declared field set) — the stray key **is** persisted, as an undeclared + column nothing downstream reads. + +A lint that misdescribes the failure it is warning about teaches the wrong +debugging instinct: an author told the value silently vanishes will not connect +the driver error they actually see to the typo that caused it, and on a +schemaless driver will not go looking for the stray key that is really there. +All three messages now state the split, matching the "What still happens at +runtime" description #4355 gave `content/docs/automation/hook-bodies.mdx`. + +Both outcomes are pinned by a new integration test — +`runtime/src/sandbox/undeclared-field-write-driver-split.integration.test.ts`. +Its insert cases run the full chain (real QuickJS sandbox, real hook body, real +engine, real driver against a real SQLite table), so "reaches the driver +unfiltered" is proved rather than asserted: if anything on that path ever +learns to filter, the SQL half stops throwing and the test goes red. The rule +headers, the `ScriptBodySchema` / `ActionSchema.body` notes and the two +still-unreleased #4271 changesets are corrected to match. #4355 fixed the +prose docs; this is the same correction on the surfaces that ship in the +packages — the diagnostic an author actually reads, and a test that pins it. + +`@objectstack/spec`: doc comments only — no schema or generated-artifact change. diff --git a/.changeset/hook-body-write-set-lint.md b/.changeset/hook-body-write-set-lint.md index dc689c7ee2..37b9c6ad55 100644 --- a/.changeset/hook-body-write-set-lint.md +++ b/.changeset/hook-body-write-set-lint.md @@ -7,12 +7,23 @@ feat(lint): L2 hook-body writes to undeclared fields warn at author time (#4271) An L2 (`language:'js'`) hook body that writes a field the target object never declares — `ctx.input.amout = 0`, `ctx.api.object('deal').update({ stag: … })` -— runs clean in the QuickJS sandbox, reports success, and the unknown column -simply never lands in the stored record. No diagnostic anywhere: the #4001 -"silent no-op manufactures false completion" failure mode at the -runtime-expression layer. The read side (`hook.condition`) and the capability -surface were already statically checked; the write side was the one blind face, -and `hook-body.zod.ts` carried it as an **accepted gap**. +— runs clean in the QuickJS sandbox and reaches the driver **unfiltered**: +`applyMutationsToInput` is a plain `Object.assign`, and the write-path +validator walks declared fields on insert and skips a key it has no field def +for on update. What happens next depends on the driver, and neither half is +acceptable: + +- **SQL** — the stray column enters the statement and the **whole write fails** + with a driver-level error (`table deal has no column named stagee`). The + write is lost, and the error surfaces far from the mistake that caused it. +- **Schemaless** (memory, MongoDB) — the driver spreads the payload, so the + stray key **is** persisted: an undeclared column nothing downstream reads. + +No diagnostic anywhere, and nothing at the authoring site either way — the +#4001 "the mistake is invisible where it is made" family. The read side +(`hook.condition`) and the capability surface were already statically checked; +the write side was the one blind face, and `hook-body.zod.ts` carried it as an +**accepted gap**. **New rule — `hook-body-write-unknown-field` (advisory).** `@objectstack/lint` now parses each L2 body (TypeScript parser; parsed, never executed, never diff --git a/packages/lint/src/reference-integrity-suite.test.ts b/packages/lint/src/reference-integrity-suite.test.ts index eb579f6cfc..2428c302dc 100644 --- a/packages/lint/src/reference-integrity-suite.test.ts +++ b/packages/lint/src/reference-integrity-suite.test.ts @@ -63,8 +63,9 @@ describe('reference-integrity suite — every member actually runs', () => { // validateObjectReferences: a param pointing at an object nothing declares. { name: 'assign', label: 'Assign', params: [{ name: 'owner', reference: 'user' }] }, // validateActionBodyWrites, both of its rule ids from one body: - // - the L2 body persists a field crm_lead does not declare — the action - // returns success and the column never lands (#4271); + // - the L2 body writes a field crm_lead does not declare — on SQL that + // fails the whole call at the driver, on a schemaless driver it + // persists a stray key (#4271); // - and it assigns ctx.record, a snapshot the runtime never writes // back, without ever passing it anywhere (#4345). { @@ -143,7 +144,8 @@ describe('reference-integrity suite — every member actually runs', () => { skills: [{ name: 'metadata_authoring', surface: 'build', tools: ['forecast_revenue'] }], hooks: [ // validateHookBodyWrites: the L2 body writes a field crm_lead does not - // declare — runs clean in the sandbox, never lands in the record (#4271). + // declare — runs clean in the sandbox, then fails the whole write at a + // SQL driver / persists a stray key on a schemaless one (#4271). { name: 'score_lead', object: 'crm_lead', diff --git a/packages/lint/src/validate-action-body-writes.ts b/packages/lint/src/validate-action-body-writes.ts index e247954670..2b5f1538bc 100644 --- a/packages/lint/src/validate-action-body-writes.ts +++ b/packages/lint/src/validate-action-body-writes.ts @@ -6,11 +6,14 @@ // An action body is the same artefact as a hook body: the same // `HookBodySchema` union, parsed by the same `HookBodySchema.safeParse` in // `actionBodyRunnerFactory` (packages/runtime/src/sandbox/body-runner.ts), run -// in the same QuickJS sandbox. So it fails the same way — an action that -// persists a field the target object never declares runs clean, returns -// success to the caller, and the unknown column simply never lands. Same -// silent no-op, same #4001 family; the hook rule alone left half the surface -// uncovered. +// in the same QuickJS sandbox. So it fails the same way — an action body that +// writes a field the target object never declares reaches the driver +// unfiltered, and the outcome is DRIVER-DEPENDENT: on SQL the stray column +// fails the whole call with a driver-level error far from the authoring +// mistake, on a schemaless driver the stray key is persisted. Same #4271 +// split as the hook side (see that file's header for the measured chain, and +// `undeclared-field-write-driver-split.integration.test.ts` for the pin); the +// hook rule alone left half the surface uncovered. // // ─── What does NOT carry over ─────────────────────────────────────────────── // @@ -331,8 +334,9 @@ export function validateActionBodyWrites(stack: AnyRec): ActionBodyWriteFinding[ path: site.path, message: `body calls ctx.api.object('${w.object}').${w.method ?? 'update'}(…) writing '${w.field}', but ` + - `object '${w.object}' declares no such field. The action returns success while the unknown column ` + - `silently never lands (#4271).`, + `object '${w.object}' declares no such field. The write-path validator skips the unknown key — ` + + `on a SQL driver the whole action then fails with a driver-level error far from here; on a ` + + `schemaless driver (memory, MongoDB) the stray key is persisted (#4271).`, hint: fixHint(w.field, [...known]), }); } diff --git a/packages/lint/src/validate-hook-body-writes.ts b/packages/lint/src/validate-hook-body-writes.ts index 783e3693a0..7f47cd71c0 100644 --- a/packages/lint/src/validate-hook-body-writes.ts +++ b/packages/lint/src/validate-hook-body-writes.ts @@ -4,13 +4,28 @@ // // An L2 body that writes a field the target object never declares — // `ctx.input.amout = 0`, `ctx.api.object('deal').update({ stag: 'won' })` — -// runs clean in the QuickJS sandbox, reports success, and the unknown column -// simply never lands in the stored record. No diagnostic anywhere: the exact -// "silent no-op manufactures false completion" failure mode of #4001, at the -// runtime-expression layer. The read side (`hook.condition`, ADR-0032) and the -// capability surface are statically checked; until this rule, the write side -// was the one blind face (the gap `hook-body.zod.ts` used to carry as -// "accepted"). +// runs clean in the QuickJS sandbox and reaches the driver UNFILTERED: +// `applyMutationsToInput` (runtime/src/sandbox/body-runner.ts) is a plain +// `Object.assign`, and `validateRecord` walks declared fields on insert and +// `continue`s past a key with no field def on update. What happens after that +// is DRIVER-DEPENDENT, and neither half is acceptable: +// +// • SQL — the stray column enters the knex statement and the WHOLE write +// fails with a driver-level error (`table deal has no column named +// stagee`). The write is lost, and the error surfaces far from the +// authoring mistake that caused it. +// • Schemaless (memory, MongoDB) — the driver spreads the payload, so the +// stray key IS persisted: an undeclared column nothing downstream reads. +// +// Either way the mistake is invisible where it is MADE — the #4001 family, if +// not literally its silent-no-op shape. Both runtime outcomes are pinned by +// `runtime/src/sandbox/undeclared-field-write-driver-split.integration.test.ts` +// so this rule's wording cannot drift from what the runtime does; the same +// split is documented in `content/docs/automation/hook-bodies.mdx`. +// +// The read side (`hook.condition`, ADR-0032) and the capability surface are +// statically checked; until this rule, the write side was the one blind face +// (the gap `hook-body.zod.ts` used to carry as "accepted"). // // Scope — the literal write patterns in {@link HOOK_BODY_WRITE_PATTERNS}, and // nothing else. The body is PARSED (TypeScript parser, never executed, never @@ -586,8 +601,9 @@ export function validateHookBodyWrites(stack: AnyRec): HookBodyWriteFinding[] { path, message: `body writes '${w.field}' to its input, but ${objDesc} ${declares}. The sandboxed script runs ` + - `clean and the value is copied back onto the record payload — then the unknown column silently ` + - `never lands in the stored record (#4271).`, + `clean and the value is copied back onto the record payload unfiltered — on a SQL driver the ` + + `stray column then fails the WHOLE write with a driver-level error far from here; on a ` + + `schemaless driver (memory, MongoDB) it is persisted as an undeclared key (#4271).`, hint: fixHint(w.field, unionCandidates(targetSets)), }); } else { @@ -604,8 +620,9 @@ export function validateHookBodyWrites(stack: AnyRec): HookBodyWriteFinding[] { path, message: `body calls ctx.api.object('${w.object}').${w.method ?? 'update'}(…) writing '${w.field}', but ` + - `object '${w.object}' declares no such field. The call succeeds while the unknown column silently ` + - `never lands (#4271).`, + `object '${w.object}' declares no such field. The write-path validator skips the unknown key — ` + + `on a SQL driver the whole call then fails with a driver-level error far from here; on a ` + + `schemaless driver (memory, MongoDB) the stray key is persisted (#4271).`, hint: fixHint(w.field, [...known]), }); } diff --git a/packages/runtime/src/sandbox/undeclared-field-write-driver-split.integration.test.ts b/packages/runtime/src/sandbox/undeclared-field-write-driver-split.integration.test.ts new file mode 100644 index 0000000000..d2be4c6913 --- /dev/null +++ b/packages/runtime/src/sandbox/undeclared-field-write-driver-split.integration.test.ts @@ -0,0 +1,179 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * What ACTUALLY happens when a write names a field the object never declares + * (#4271) — pinned against both driver families, because the answer differs. + * + * This file exists to stop one sentence from drifting back. The + * `hook-body-write-unknown-field` / `action-body-write-unknown-field` lint + * messages used to tell authors the unknown column "silently never lands in + * the stored record". That is wrong in BOTH directions, and a lint that + * misdescribes the failure it is warning about teaches the wrong debugging + * instinct — an author told "it silently vanishes" will not connect the + * driver-level error they actually see to the typo that caused it. + * + * Nothing between the body and the driver filters the key: + * + * 1. `applyMutationsToInput` (./body-runner.ts) is a plain `Object.assign` — + * the sandbox's mutated input is copied onto the payload verbatim. + * 2. `validateRecord` (objectql/src/validation/record-validator.ts) walks + * DECLARED fields on insert, and on update does `if (!def) continue`. + * It neither rejects the unknown key nor strips it. + * 3. `engine.ts` hands the row to `driver.create` / the driver's update. + * + * So the driver decides, and the two families disagree: + * + * • SQL — the stray column reaches the statement and the WHOLE write fails. + * Nothing is stored, and the error names a column, not a field, far from + * the body that wrote it. + * • Schemaless (memory, and MongoDB on the same `...data` spread) — the key + * IS persisted, as an undeclared column nothing downstream reads. + * + * The lint messages, the `ScriptBodySchema` / `ActionSchema.body` notes and + * `content/docs/automation/hook-bodies.mdx` all describe this split; if any of + * them drifts back to "silently never lands", one half of this file fails. + * + * The insert cases run the FULL chain — real QuickJS sandbox, real hook body, + * real engine, real driver — so link 1 is proved rather than assumed: if + * `applyMutationsToInput` ever learned to filter, the SQL insert would stop + * throwing. The update cases go straight at the engine, because that is where + * `validateRecord`'s update branch lives and a `beforeUpdate` body would only + * add the flat-input envelope to the thing under test. + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { ObjectQL, bindHooksToEngine } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { InMemoryDriver } from '@objectstack/driver-memory'; +import { hookBodyRunnerFactory } from './body-runner.js'; +import { QuickJSScriptRunner } from './quickjs-runner.js'; + +/** `stagee` is the typo under test; `stage` is the field that exists. */ +const DEAL = { + name: 'deal', + fields: { + stage: { type: 'text', name: 'stage' }, + amount: { type: 'number', name: 'amount' }, + }, +}; + +/** The #4271 authoring mistake, in the shape the hook rule flags. */ +const TYPO_HOOK = { + name: 'deal_stage_typo', + object: 'deal', + events: ['beforeInsert'], + body: { language: 'js', source: `ctx.input.stagee = 'won';` }, +}; + +/** The same body with the field name spelled correctly — the control. */ +const CORRECT_HOOK = { + name: 'deal_stage_ok', + object: 'deal', + events: ['beforeInsert'], + body: { language: 'js', source: `ctx.input.stage = 'won';` }, +}; + +describe('#4271 an undeclared field written by an L2 body — the real runtime split', () => { + let engine: ObjectQL | null = null; + let dir: string | null = null; + + afterEach(async () => { + try { await engine?.destroy(); } catch { /* noop */ } + engine = null; + if (dir) { rmSync(dir, { recursive: true, force: true }); dir = null; } + }); + + async function bootSql(hook?: unknown) { + dir = mkdtempSync(join(tmpdir(), 'os-4271-')); + const driver = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: join(dir, 'data.sqlite') }, + useNullAsDefault: true, + }); + await driver.initObjects([DEAL]); // a REAL table, with only the declared columns + return boot(driver, hook); + } + + async function bootMemory(hook?: unknown) { + return boot(new InMemoryDriver(), hook); + } + + async function boot(driver: unknown, hook?: unknown) { + engine = new ObjectQL(); + engine.registerDriver(driver as any, true); + await engine.init(); + engine.registry.registerObject(DEAL as any); + engine.setDefaultBodyRunner( + hookBodyRunnerFactory(new QuickJSScriptRunner(), { ql: engine, appId: 'deal' }), + ); + if (hook) bindHooksToEngine(engine, [hook as any], { packageId: 'deal' }); + return engine; + } + + // ─── SQL: a loud failure that loses the whole write ──────────────────────── + + describe('SQL driver (better-sqlite3, real table)', () => { + it('fails the WHOLE insert at the driver — it is not a silent no-op', async () => { + const e = await bootSql(TYPO_HOOK); + // The body runs clean in the sandbox; the throw comes from the database. + await expect(e.insert('deal', { stage: 'open', amount: 10 })) + .rejects.toThrow(/stagee/); + }, 30000); + + it('stores NOTHING — the declared columns of that row are lost too', async () => { + const e = await bootSql(TYPO_HOOK); + await expect(e.insert('deal', { stage: 'open', amount: 10 })).rejects.toThrow(); + // The half that makes "silently never lands" actively misleading: an + // author told the column vanishes would expect a row with `amount: 10`. + expect(await e.find('deal', { where: {} } as any)).toHaveLength(0); + }, 30000); + + it('fails an UPDATE the same way, and leaves the row untouched', async () => { + const e = await bootSql(); + const row = await e.insert('deal', { stage: 'open', amount: 10 }); + // `validateRecord`'s update branch `continue`s past the unknown key + // rather than rejecting it, so the driver is what refuses the write. + await expect(e.update('deal', { id: row.id, stagee: 'won' } as any)) + .rejects.toThrow(/stagee/); + const after: any = (await e.find('deal', { where: { id: row.id } } as any))[0]; + expect(after.stage).toBe('open'); + expect(after).not.toHaveProperty('stagee'); + }, 30000); + + it('control: the same body with the field spelled right writes normally', async () => { + const e = await bootSql(CORRECT_HOOK); + const row = await e.insert('deal', { stage: 'open', amount: 10 }); + // Proves the failures above are about the undeclared column and not a + // broken fixture — and that `applyMutationsToInput` does reach the driver. + expect((await e.find('deal', { where: { id: row.id } } as any))[0].stage).toBe('won'); + }, 30000); + }); + + // ─── Schemaless: the stray key is persisted, not dropped ─────────────────── + + // `InMemoryDriver.create` spreads `...data`; `MongoDbDriver.create` spreads + // `...toStorageForms(object, rest)`. Neither consults the declared field set, + // so memory stands in for the whole schemaless family here. + describe('schemaless driver (memory)', () => { + it('accepts the insert and PERSISTS the undeclared key', async () => { + const e = await bootMemory(TYPO_HOOK); + const row = await e.insert('deal', { stage: 'open', amount: 10 }); + const stored: any = (await e.find('deal', { where: { id: row.id } } as any))[0]; + // The other direction the old wording got wrong: it lands. + expect(stored.stagee).toBe('won'); + expect(stored.stage).toBe('open'); + expect(stored.amount).toBe(10); + }, 30000); + + it('persists it on UPDATE too', async () => { + const e = await bootMemory(); + const row = await e.insert('deal', { stage: 'open', amount: 10 }); + await e.update('deal', { id: row.id, stagee: 'won' } as any); + const stored: any = (await e.find('deal', { where: { id: row.id } } as any))[0]; + expect(stored.stagee).toBe('won'); + }, 30000); + }); +}); diff --git a/packages/spec/src/data/hook-body.zod.ts b/packages/spec/src/data/hook-body.zod.ts index 15918e96d8..038da7b20a 100644 --- a/packages/spec/src/data/hook-body.zod.ts +++ b/packages/spec/src/data/hook-body.zod.ts @@ -138,13 +138,17 @@ export type ExpressionBody = z.infer; * `HOOK_BODY_WRITE_PATTERNS` ledger declares (`ctx.input.x = …`, * `Object.assign(ctx.input, { x })`, `ctx.api.object('y').update({ x })`) * are resolved against the target object's declared + system fields, and an - * unknown field warns with a did-you-mean. Statically unknowable writes — - * computed keys, spreads, aliased `ctx.input`, dynamic object names, - * wildcard-target hooks — remain opaque and are skipped silently, so the - * warning's absence is not proof of correctness. When the write set is fixed, - * still prefer a flow `update_record` node: `validateFlowNodeWrites` resolves - * its `config.fields` keys with no parser in between, so that surface gates - * (`flow-node-write-unknown-field`, `error`) where this one advises. + * unknown field warns with a did-you-mean. Nothing downstream catches what the + * lint misses: the payload reaches the driver unfiltered, so a SQL driver + * fails the WHOLE write with a driver-level error far from the authoring site, + * while a schemaless driver (memory, MongoDB) persists the stray key. + * Statically unknowable writes — computed keys, spreads, aliased `ctx.input`, + * dynamic object names, wildcard-target hooks — remain opaque and are skipped + * silently, so the warning's absence is not proof of correctness. When the + * write set is fixed, still prefer a flow `update_record` node: + * `validateFlowNodeWrites` resolves its `config.fields` keys with no parser in + * between, so that surface gates (`flow-node-write-unknown-field`, `error`) + * where this one advises. * * An **action** body carrying this same schema is checked by the sibling rule * `validateActionBodyWrites`, over the subset of that ledger which survives the diff --git a/packages/spec/src/ui/action.zod.ts b/packages/spec/src/ui/action.zod.ts index 9d67da3423..e8e5c370dd 100644 --- a/packages/spec/src/ui/action.zod.ts +++ b/packages/spec/src/ui/action.zod.ts @@ -477,8 +477,10 @@ const actionObject = () => z.object({ * * Both are checked at author time by `validateActionBodyWrites` in * `@objectstack/lint`: a literal `ctx.api.object('y').update({ x })` naming a - * field object `y` never declares warns with a did-you-mean, because the call - * succeeds while the unknown column silently never lands (#4271); and a + * field object `y` never declares warns with a did-you-mean, because nothing + * downstream catches it — on a SQL driver the stray column fails the whole + * call with a driver-level error far from the authoring site, and on a + * schemaless driver it is persisted as an undeclared key (#4271); and a * provably dead `ctx.record. = …` warns as discarded (#4345). * Advisory only, and blind to everything statically unknowable; see * `ScriptBodySchema` for the scope.