diff --git a/packages/cli/src/flow-node-undeclared-field-write.integration.test.ts b/packages/cli/src/flow-node-undeclared-field-write.integration.test.ts new file mode 100644 index 0000000000..479d34752b --- /dev/null +++ b/packages/cli/src/flow-node-undeclared-field-write.integration.test.ts @@ -0,0 +1,343 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#14241] What a flow CRUD node's `fields` write map ACTUALLY does when it + * names a field the target object never declares — pinned end to end, through + * the real AutomationEngine, the real builtin CRUD node executors, a real + * ObjectQL and a real driver. + * + * ## The sentence this file holds up + * + * `validate-flow-node-writes` (@objectstack/lint) is `severity: 'error'` — it + * GATES, where its two `ctx.api` siblings only advise — and its header states + * the runtime consequence as measured fact: + * + * • the declared-field door refuses the write — `INVALID_FIELD` / 400, + * "Unknown field 'stagee' on object 'deal'", identically on every + * datasource, before any statement is built; + * • the write is refused WHOLE: a correctly named field in the SAME payload + * does not land either; + * • on `create_record` the row is never created at all, so every later node + * expecting `{.id}` is working from a record that does not exist; + * • the node catches the refusal and folds it into a step failure + * (`create_record(deal) failed: …`), so the RUN fails — far from the + * authoring mistake, which is why an author-time rule is worth having. + * + * #13858 rewrote that prose after measuring it. The harness it measured with + * was a scratch and was deleted, so from that day the three corrected messages + * asserted a runtime behaviour that nothing pinned — the exact drift + * `packages/runtime/src/sandbox/undeclared-field-write-driver-split.integration.test.ts` + * exists to prevent for the two call shapes IT covers. This file is the flow + * node's half; the `ctx.api` half lives in that runtime file, beside the two + * shapes it already pinned. + * + * ## Why this shape is not already covered + * + * The flow executor calls the data engine directly (`data.insert` / + * `data.update` in service-automation's `builtin/crud-nodes.ts`), bypassing the + * metadata-protocol ingress, so the node's `fields` map arrives as an ORDINARY + * CALLER PAYLOAD. The refusal itself is therefore the pre-hook declared-field + * door (#8682 insert, #8738 update) — which the runtime file already pins on + * both driver families, including the schemaless family's "no shadow column". + * What is unpinned, and what this file adds, is everything the flow layer wraps + * around that refusal: whether the run fails or reports a clean success, what + * the step says, whether the correctly named siblings survive, and whether the + * row exists afterwards. + * + * ## Why there is no second driver arm here + * + * The lint prose says "identically on every datasource" — and the reason it can + * is structural, not statistical: NO DRIVER IS REACHED. So this file proves the + * structural fact directly (`writes` below counts every write verb the driver + * is asked to perform, and the refusal cases assert zero) rather than sampling + * two families and inferring it. A second family run could only ever agree with + * the first about a code path neither of them executes. + * + * ⚠️ That is also the only shape available here. The schemaless witness in this + * repo is `@objectstack/driver-memory`, whose every declaration is disposed of + * in `scripts/driver-memory-census.ledger.json` and gated by + * `pnpm check:driver-memory-census` (#6664, from #5704 / #5499). Admitting a + * new test consumer of a frozen driver is a maintainer ruling, not a test + * author's call — and the CLI's own ledger entry records that "the CLI imports + * the driver nowhere". The zero-write assertion is what makes that a + * non-sacrifice: the family-split question cannot arise below a door nothing + * gets past. + */ + +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 } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { AutomationEngine, registerCrudNodes } from '@objectstack/service-automation'; +import type { EngineQueryOptions } from '@objectstack/spec/data'; + +/** + * The read-backs, TYPED rather than cast — `check:query-options-erasure` + * counts an `as any` options bag in test code too, and these are ordinary + * `where` bags with no reason to be erased. + */ +const allRows: EngineQueryOptions = { where: {} }; +const rowById = (id: unknown): EngineQueryOptions => ({ where: { id } }); + +/** `stagee` is the typo under test; `stage` is the field that exists. */ +const DEAL = { + name: 'deal', + fields: { + name: { type: 'text', name: 'name' }, + stage: { type: 'text', name: 'stage' }, + amount: { type: 'number', name: 'amount' }, + }, +}; + +/** Silent logger — the engine and the node pack both take one. */ +function makeLogger(): any { + const l: any = { info() {}, warn() {}, error() {}, debug() {} }; + l.child = () => l; + return l; +} + +/** + * Every write verb the driver contract exposes, counted. + * + * This is the file's load-bearing instrument, not a convenience: "identically + * on every datasource" is a claim about a code path that is never entered, and + * the only honest way to pin a never-entered path is to watch the entrance. + */ +const WRITE_VERBS = ['create', 'update', 'upsert', 'delete', 'bulkCreate', 'bulkUpdate', 'updateMany', 'deleteMany'] as const; + +function countWrites(driver: any): { total: () => number; byVerb: Record } { + const byVerb: Record = {}; + for (const verb of WRITE_VERBS) { + const original = driver[verb]; + if (typeof original !== 'function') continue; + byVerb[verb] = 0; + driver[verb] = function patched(this: unknown, ...args: unknown[]) { + byVerb[verb] += 1; + return original.apply(driver, args); + }; + } + return { total: () => Object.values(byVerb).reduce((a, b) => a + b, 0), byVerb }; +} + +/** A `create_record` flow whose node writes `fields` into `deal`. */ +function createFlow(name: string, fields: Record) { + return { + name, label: name, type: 'autolaunched', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'c', type: 'create_record', label: 'Create', config: { objectName: 'deal', fields } }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'c' }, + { id: 'e2', source: 'c', target: 'end' }, + ], + } as any; +} + +/** An `update_record` flow naming one row by scalar id (no bulk intent). */ +function updateFlow(name: string, id: unknown, fields: Record) { + return { + name, label: name, type: 'autolaunched', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'u', type: 'update_record', label: 'Update', config: { objectName: 'deal', filter: { id }, fields } }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'u' }, + { id: 'e2', source: 'u', target: 'end' }, + ], + } as any; +} + +describe('#14241 a flow CRUD node writing an undeclared field', () => { + 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; } + }); + + /** A real ObjectQL on a real sqlite table with only the declared columns. */ + async function boot() { + dir = mkdtempSync(join(tmpdir(), 'os-14241-')); + const driver = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: join(dir, 'data.sqlite') }, + useNullAsDefault: true, + }); + await driver.initObjects([DEAL]); + engine = new ObjectQL(); + engine.registerDriver(driver as any, true); + await engine.init(); + engine.registry.registerObject(DEAL as any); + return { ql: engine, driver }; + } + + /** The real builtin CRUD nodes over that engine — no stub in the chain. */ + function automationOver(ql: ObjectQL) { + const logger = makeLogger(); + const automation = new AutomationEngine(logger); + registerCrudNodes(automation, { + logger, + getService: (n: string) => (n === 'data' ? ql : undefined), + } as any); + return automation; + } + + const stepOf = async (automation: AutomationEngine, flow: string, nodeId: string) => { + const runs = await automation.listRuns(flow); + return runs[0].steps.find((s: any) => s.nodeId === nodeId)!; + }; + + // ─── The envelope, at the seam the node hands the payload to ────────────── + + /** + * The node's `fields` map IS a caller payload, so the refusal it meets is + * the ADR-0112 envelope the three lint messages quote. The node folds that + * envelope into a string, and the engine then stamps the step it failed + * `NODE_FAILURE` (`create_record` re-surfaces a node-level `code` only for + * `DUPLICATE_RECORD`, `update_record` for nothing at all, and neither + * reaches `step.error.code` anyway). So this is the one place in the flow + * chain where the DOOR's `code` and `status` are still observable, and the + * step assertions below pin what is left of them. Asserted here rather than + * left to the step's prose: a message can be reworded, and a `toThrow()` + * would pass on any error at all — including the driver-level failure this + * door exists to make unreachable. + */ + it('the door answers INVALID_FIELD / 400 for the exact payload the node builds', async () => { + const { ql } = await boot(); + + const onInsert: any = await ql.insert('deal', { name: 'first', stagee: 'won' } as any) + .catch((x: unknown) => x); + const seed: any = await ql.insert('deal', { name: 'seed', stage: 'open', amount: 10 }); + const onUpdate: any = await ql.update('deal', { id: seed.id, stagee: 'won' } as any) + .catch((x: unknown) => x); + + for (const err of [onInsert, onUpdate]) { + expect(err?.code).toBe('INVALID_FIELD'); + expect(err?.status).toBe(400); + expect(err?.field).toBe('stagee'); + expect(err?.message).toBe("Unknown field 'stagee' on object 'deal'"); + } + }, 30000); + + // ─── create_record ──────────────────────────────────────────────────────── + + describe('create_record', () => { + it('fails the RUN, and the step names the refusal', async () => { + const { ql } = await boot(); + const automation = automationOver(ql); + automation.registerFlow('f_create_bad', createFlow('f_create_bad', { name: 'first', stagee: 'won' })); + + const res = await automation.execute('f_create_bad', { userId: 'u1' }); + + expect(res.success).toBe(false); + const step = await stepOf(automation, 'f_create_bad', 'c'); + expect(step.status).toBe('failure'); + // The step's error is an envelope of its own, and the WHOLE of it is + // pinned: the flow layer reclassifies every failing node to + // `NODE_FAILURE` (engine.ts, its single step-push site) and carries + // the door's message verbatim inside it. So `INVALID_FIELD` is NOT + // what a run reports — the message is the only channel that + // survives the fold, which is why it is asserted whole rather than + // by `toContain`. + expect(step.error).toEqual({ + code: 'NODE_FAILURE', + message: "create_record(deal) failed: Unknown field 'stagee' on object 'deal'", + }); + }, 30000); + + it('creates NO row — so a later {.id} has nothing to read', async () => { + const { ql, driver } = await boot(); + const writes = countWrites(driver); + const automation = automationOver(ql); + automation.registerFlow('f_create_none', createFlow('f_create_none', { name: 'first', stagee: 'won' })); + + await automation.execute('f_create_none', { userId: 'u1' }); + + expect(await ql.find('deal', allRows)).toHaveLength(0); + // "before any statement is built", measured rather than asserted in + // prose: the driver was never asked to write anything, which is why + // no datasource can answer this differently. + expect(writes.total()).toBe(0); + }, 30000); + + it('CONTROL — the same node spelled right creates the row', async () => { + const { ql } = await boot(); + const automation = automationOver(ql); + automation.registerFlow('f_create_ok', createFlow('f_create_ok', { name: 'first', stage: 'won' })); + + const res = await automation.execute('f_create_ok', { userId: 'u1' }); + + expect(res.success).toBe(true); + const rows: any[] = await ql.find('deal', allRows); + expect(rows).toHaveLength(1); + expect(rows[0].stage).toBe('won'); + }, 30000); + }); + + // ─── update_record ──────────────────────────────────────────────────────── + + describe('update_record', () => { + it('fails the RUN, and the step names the refusal', async () => { + const { ql } = await boot(); + const seed: any = await ql.insert('deal', { name: 'seed', stage: 'open', amount: 10 }); + const automation = automationOver(ql); + automation.registerFlow('f_update_bad', updateFlow('f_update_bad', seed.id, { stagee: 'won' })); + + const res = await automation.execute('f_update_bad', { userId: 'u1' }); + + expect(res.success).toBe(false); + const step = await stepOf(automation, 'f_update_bad', 'u'); + expect(step.status).toBe('failure'); + expect(step.error).toEqual({ + code: 'NODE_FAILURE', + message: "update_record(deal) failed: Unknown field 'stagee' on object 'deal'", + }); + }, 30000); + + it('refuses the write WHOLE — the correctly named field in the same map does not land either', async () => { + const { ql, driver } = await boot(); + const seed: any = await ql.insert('deal', { name: 'seed', stage: 'open', amount: 10 }); + const writes = countWrites(driver); + const automation = automationOver(ql); + automation.registerFlow( + 'f_update_whole', + updateFlow('f_update_whole', seed.id, { name: 'renamed', stagee: 'won' }), + ); + + await automation.execute('f_update_whole', { userId: 'u1' }); + + const after: any = (await ql.find('deal', rowById(seed.id)))[0]; + // `name` was spelled correctly and rode in the same payload. An + // author reading "the unknown key is skipped" would expect it to + // land; it does not. + expect(after.name).toBe('seed'); + expect(after.stage).toBe('open'); + // The assertion that separates a refusal from a silent write: a + // datasource with no schema to check against would keep the key. + expect(after).not.toHaveProperty('stagee'); + expect(writes.total()).toBe(0); + }, 30000); + + it('CONTROL — the same node spelled right updates the row', async () => { + const { ql } = await boot(); + const seed: any = await ql.insert('deal', { name: 'seed', stage: 'open', amount: 10 }); + const automation = automationOver(ql); + automation.registerFlow('f_update_ok', updateFlow('f_update_ok', seed.id, { name: 'renamed', stage: 'won' })); + + const res = await automation.execute('f_update_ok', { userId: 'u1' }); + + expect(res.success).toBe(true); + const after: any = (await ql.find('deal', rowById(seed.id)))[0]; + expect(after.name).toBe('renamed'); + expect(after.stage).toBe('won'); + }, 30000); + }); +}); 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 index 01619a6ff4..ab96141e49 100644 --- 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 @@ -159,8 +159,8 @@ 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'; +import { hookBodyRunnerFactory, actionBodyRunnerFactory } from './body-runner.js'; +import { QuickJSScriptRunner, SandboxError } from './quickjs-runner.js'; import type { EngineQueryOptions } from '@objectstack/spec/data'; import { captureExpectedReadRefusals, @@ -175,6 +175,9 @@ import { */ const rowById = (id: unknown): EngineQueryOptions => ({ where: { id } }); +/** [#14241] The unfiltered read, typed for the same reason `rowById` is. */ +const allRows: EngineQueryOptions = { where: {} }; + /** `stagee` is the typo under test; `stage` is the field that exists. */ const DEAL = { name: 'deal', @@ -458,4 +461,166 @@ describe('#4271 / #13657 an undeclared field written by an L2 body — one answe expect(stored).not.toHaveProperty('stagee'); }, 30000); }); + + // ─── [#14241] The THIRD shape: a write the body issues through `ctx.api` ─── + + /** + * [#14241] Why neither block above answers for a `ctx.api` write. + * + * `hook-body-write-unknown-field`, `action-body-write-unknown-field` and + * `flow-node-write-unknown-field` all tell authors the same thing about + * `ctx.api.object('').update({ stagee: … })`: it is REFUSED at run time — + * `INVALID_FIELD` / 400, identically on every driver, before any statement is + * built. #13858 measured that and rewrote the three messages to say it. The + * harness it measured with was a scratch and was deleted, which left three + * live author-facing sentences with nothing under them. + * + * The two blocks above are the corroboration those sentences were written + * from, and they are about DIFFERENT call shapes: + * + * • the L2 BODY block mutates `ctx.input`, which the engine folds into the + * CALLER's payload — refused by the POST-hook half of the door (#13657); + * • the CALLER block hands the engine a payload directly — refused by the + * PRE-hook half (#8682 / #8738). + * + * A `ctx.api` write is neither. It is a SECOND, nested engine call the body + * makes on its own behalf, with its own payload and its own trip through the + * door — and its verdict comes back through the VM BOUNDARY, which is exactly + * where a code and a status can be lost with every gate green (#3918 lost + * both directions of the payload once; #7867 lost `status` alone, and served + * the right diagnosis at the wrong HTTP status until it was restored). + * + * So what these cases pin is not "the engine refuses" — that is proved above — + * but that the refusal ARRIVES: intact, in its ADR-0112 envelope, at the + * caller who invoked the body. Asserting `code` AND `status` is the whole + * point; a bare `toThrow()` here would pass on the VM's flattened + * `: ` string, i.e. on the precise regression this exists to + * catch. + */ + + /** + * The hook body the lint rule's own example describes: a literal object name, + * a nested write, one key misspelled. `capabilities: ['api.write']` because + * the sandbox refuses the surface without it — a body that cannot reach + * `ctx.api` at all would make these cases green for the wrong reason. + */ + const apiTypoHook = (targetId: unknown) => ({ + name: 'deal_api_typo', + object: 'deal', + events: ['beforeInsert'], + body: { + language: 'js', + source: `await ctx.api.object('deal').update({ id: ${JSON.stringify(targetId)}, stagee: 'won' });`, + capabilities: ['api.write'], + }, + }); + + /** The same nested write, spelled right — the control for both faces. */ + const apiCorrectHook = (targetId: unknown) => ({ + name: 'deal_api_ok', + object: 'deal', + events: ['beforeInsert'], + body: { + language: 'js', + source: `await ctx.api.object('deal').update({ id: ${JSON.stringify(targetId)}, stage: 'won' });`, + capabilities: ['api.write'], + }, + }); + + /** The action face of the same write. `type: 'script'` or no handler binds. */ + const apiTypoAction = (targetId: unknown) => ({ + name: 'deal_api_typo_action', + object: 'deal', + type: 'script', + body: { + language: 'js', + source: + `await ctx.api.object('deal').update({ id: ${JSON.stringify(targetId)}, stagee: 'won' });` + + ` return 'unreachable';`, + capabilities: ['api.write'], + }, + }); + + describe("a `ctx.api` write from a HOOK body — refused identically on both families", () => { + it.each(FAMILIES)('%s: the refusal reaches the caller in its ADR-0112 envelope', async (_name, bootFamily) => { + const e = await bootFamily(); + // Seeded BEFORE the hook is bound, so the body's nested write names a row + // that exists: a refusal on a missing row would be RECORD_NOT_FOUND and + // would pin nothing about undeclared fields. + const seed = await e.insert('deal', { stage: 'open', amount: 10 }); + bindHooksToEngine(e, [apiTypoHook(seed.id) as any], { packageId: 'deal' }); + + const err: any = await e.insert('deal', { stage: 'new', amount: 20 }).catch((x: unknown) => x); + + expect(err?.code).toBe('INVALID_FIELD'); + expect(err?.status).toBe(400); + // The envelope crossed the VM boundary as a SandboxError — the hop that + // #3918/#7867 each broke once. `innerMessage` is the un-wrapped text a + // toast shows; `message` carries the sandbox's origin prefix. + expect(err).toBeInstanceOf(SandboxError); + expect(err?.innerMessage).toBe("Unknown field 'stagee' on object 'deal'"); + }, 30000); + + it.each(FAMILIES)('%s: nothing lands — not the nested write, not the write that triggered it', async (_name, bootFamily) => { + const e = await bootFamily(); + const seed = await e.insert('deal', { stage: 'open', amount: 10 }); + bindHooksToEngine(e, [apiTypoHook(seed.id) as any], { packageId: 'deal' }); + + await expect(e.insert('deal', { stage: 'new', amount: 20 })).rejects.toThrow(); + + // The half that distinguishes a REFUSAL from a silent write: on the + // schemaless family a persisted `stagee` would be an undeclared column + // nothing downstream reads and field-level security can never gate. + const rows: any[] = await e.find('deal', allRows); + expect(rows).toHaveLength(1); + expect(rows[0].stage).toBe('open'); + expect(rows[0]).not.toHaveProperty('stagee'); + }, 30000); + + it.each(FAMILIES)('%s: CONTROL — the same nested write spelled right lands', async (_name, bootFamily) => { + const e = await bootFamily(); + const seed = await e.insert('deal', { stage: 'open', amount: 10 }); + bindHooksToEngine(e, [apiCorrectHook(seed.id) as any], { packageId: 'deal' }); + + // Proves the refusals above are about the undeclared key and not about a + // `ctx.api` surface that never worked in this fixture at all. + await e.insert('deal', { stage: 'new', amount: 20 }); + + expect((await e.find('deal', rowById(seed.id)))[0].stage).toBe('won'); + }, 30000); + }); + + describe("a `ctx.api` write from an ACTION body — refused identically on both families", () => { + it.each(FAMILIES)('%s: the refusal reaches the caller in its ADR-0112 envelope', async (_name, bootFamily) => { + const e = await bootFamily(); + const seed = await e.insert('deal', { stage: 'open', amount: 10 }); + // The action face binds through its OWN factory rather than the engine's + // default body runner, which is the only structural difference from the + // hook face — same VM, same `ctx.api`, same door. + const handler = actionBodyRunnerFactory(new QuickJSScriptRunner(), { ql: e, appId: 'deal' })( + apiTypoAction(seed.id) as any, + ); + + const err: any = await handler!({ object: 'deal', params: {} }).catch((x: unknown) => x); + + expect(err?.code).toBe('INVALID_FIELD'); + expect(err?.status).toBe(400); + expect(err).toBeInstanceOf(SandboxError); + expect(err?.innerMessage).toBe("Unknown field 'stagee' on object 'deal'"); + }, 30000); + + it.each(FAMILIES)('%s: the target row is untouched, with no shadow column', async (_name, bootFamily) => { + const e = await bootFamily(); + const seed = await e.insert('deal', { stage: 'open', amount: 10 }); + const handler = actionBodyRunnerFactory(new QuickJSScriptRunner(), { ql: e, appId: 'deal' })( + apiTypoAction(seed.id) as any, + ); + + await expect(handler!({ object: 'deal', params: {} })).rejects.toThrow(); + + const after: any = (await e.find('deal', rowById(seed.id)))[0]; + expect(after.stage).toBe('open'); + expect(after).not.toHaveProperty('stagee'); + }, 30000); + }); });