diff --git a/.changeset/write-set-messages-drop-driver-split.md b/.changeset/write-set-messages-drop-driver-split.md new file mode 100644 index 0000000000..d91e0d7d90 --- /dev/null +++ b/.changeset/write-set-messages-drop-driver-split.md @@ -0,0 +1,53 @@ +--- +"@objectstack/lint": patch +--- + +fix(lint): the three write-set rule messages now state the refusal authors actually get, not a retired driver split (#13858) + +Message text only. Rule ids, severities, match sets and hints are untouched, and +no finding changes shape — but a lint's own header states why the prose is +governed: *"a lint that misdescribes the failure it is warning about teaches the +wrong debugging instinct"*. These three sentences did. + +`validate-hook-body-writes` (the `ctx.api` branch), `validate-action-body-writes` +and `validate-flow-node-writes` all told the author that an undeclared write has +a **driver-dependent** outcome: + +> 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 + +For the paths those three rules judge, that has not been true since the +declared-field door landed (#8682 insert, #8738 update). All three describe a +write whose payload is **caller-supplied**, not a mutation of an in-flight +`ctx.input`: `ctx.api` is a `ScopedContext` over the running engine, and a flow +node hands its `fields` map to the data engine directly. The door refuses a +caller-named undeclared key from the object's field map **before any statement is +built**, so no driver is reached and there is no split to observe. + +Measured before the prose was rewritten — all three paths, both driver families, +through a real QuickJS sandbox, a real `ObjectQL` engine, the real +`AutomationEngine` with the real builtin CRUD node executors, real +`@objectstack/driver-sql` (better-sqlite3) and real `@objectstack/driver-memory`: + +| path | driver-sql | driver-memory | +|---|---|---| +| hook body `ctx.api.object(x).update({…})` | `INVALID_FIELD` / 400 | `INVALID_FIELD` / 400 | +| action body `ctx.api.object(x).update({…})` | `INVALID_FIELD` / 400 | `INVALID_FIELD` / 400 | +| flow `create_record` / `update_record` `fields` | `INVALID_FIELD` / 400 | `INVALID_FIELD` / 400 | + +Every run answered `Unknown field 'stagee' on object 'deal'`; nothing was stored +on either family, and the schemaless family kept **no** shadow column — the half +the old message promised and the runtime no longer delivers. + +The three messages now name that refusal in the vocabulary the `ctx.input` +sibling landed with (`REFUSED at run time — INVALID_FIELD / 400, identically on +every driver`), say why the door and not a driver answers, and keep each path's +own blast radius: the hook refusal fails the operation that triggered the hook, +the action refusal fails the action, and the flow node's refusal is whole — the +correctly named fields in the same payload never land either, `create_record` +never creates the row, and the step fails the run. That last clause is why the +flow rule still gates at `error`; the severity is unchanged. + +`unprovisionedAnchorWriteConsequence()` in the same files is **untouched**: an +ADR-0015 external object's injected anchor *is* declared in the registered +schema, so it passes the door by construction and the remote database really is +what refuses it. That message was already correct. diff --git a/packages/lint/src/validate-action-body-writes.test.ts b/packages/lint/src/validate-action-body-writes.test.ts index b72dd906ed..9c3235307e 100644 --- a/packages/lint/src/validate-action-body-writes.test.ts +++ b/packages/lint/src/validate-action-body-writes.test.ts @@ -167,6 +167,35 @@ describe('validateActionBodyWrites — ctx.api writes', () => { expect(findings[0].hint).toContain("'discount_total'"); }); + // [#13858] The same rewrite as the hook sibling, from the same measurement: + // real QuickJS sandbox, a real L2 ACTION body run through + // `actionBodyRunnerFactory`, a real ObjectQL engine, real driver-sql + // (better-sqlite3) AND real driver-memory. Both families answered + // `INVALID_FIELD` / 400, "Unknown field 'stagee' on object 'deal'"; the + // target row was untouched and the memory family stored no shadow column. + // The old text promised a driver-level error on SQL and a persisted stray + // key on schemaless — neither happens on this path, and has not since + // #8682/#8738 put the declared-field door ahead of any statement. + it('states the measured refusal — INVALID_FIELD / 400 on every driver — and no driver split', () => { + const [finding] = validateActionBodyWrites( + stackWith("await ctx.api.object('crm_deal').update({ discont_total: 0 });"), + ); + + expect(finding.message).toContain('INVALID_FIELD / 400'); + expect(finding.message).toContain('identically on every driver'); + expect(finding.message).toContain('before any statement is built'); + // The reason the door — not a driver — is what answers. + expect(finding.message).toContain('ordinary CALLER write'); + // The action-side blast radius, the one word that differs from the hook + // sibling's sentence. Pinned so a future sweep cannot flatten the two. + expect(finding.message).toContain('fails the action'); + + expect(finding.message).not.toMatch(/driver-level error/); + expect(finding.message).not.toMatch(/schemaless/); + expect(finding.message).not.toMatch(/is persisted/); + expect(finding.message).not.toMatch(/write-path validator skips/); + }); + it('checks insert/create/update payloads (argument 0) and updateById at argument 1', () => { const findings = validateActionBodyWrites( stackWith( diff --git a/packages/lint/src/validate-action-body-writes.ts b/packages/lint/src/validate-action-body-writes.ts index bc372c3ae9..fad927f288 100644 --- a/packages/lint/src/validate-action-body-writes.ts +++ b/packages/lint/src/validate-action-body-writes.ts @@ -7,13 +7,18 @@ // `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 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. +// writes a field the target object never declares is refused at run time, far +// from the authoring mistake. [#13858] That refusal is NOT driver-dependent, +// and the message says so: this rule judges exactly one shape, +// `ctx.api.object('').insert|create|update|updateById(…)`, and +// `ctx.api` is a ScopedContext over the running engine, so the payload is +// CALLER-supplied and the declared-field door (#8682 insert, #8738 update) +// refuses it — `INVALID_FIELD` / 400, identically on driver-sql and +// driver-memory, before any statement is built. Measured on both families +// through the real sandbox and the real engine; the caller-payload half of +// that door is pinned in +// `undeclared-field-write-driver-split.integration.test.ts`. The hook rule +// alone left half the surface uncovered, which is why this file exists. // // ─── What does NOT carry over ─────────────────────────────────────────────── // @@ -429,9 +434,14 @@ 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 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).`, + // [#13858] Same door, same measurement as the hook sibling — ctx.api + // is a ScopedContext over the running engine, so this payload is + // CALLER-supplied and #8682/#8738 refuse it before any driver. + `object '${w.object}' declares no such field. ctx.api is a scoped handle on the running ` + + `engine, so the payload arrives as an ordinary CALLER write and the declared-field door ` + + `REFUSES it at run time — INVALID_FIELD / 400, identically on every driver (#4271), before ` + + `any statement is built. The write lands nothing, and the refusal escapes the body and ` + + `fails the action.`, hint: fixHint(w.field, [...known]), }); } diff --git a/packages/lint/src/validate-flow-node-writes.test.ts b/packages/lint/src/validate-flow-node-writes.test.ts index 81f3c1e094..049ed95eb8 100644 --- a/packages/lint/src/validate-flow-node-writes.test.ts +++ b/packages/lint/src/validate-flow-node-writes.test.ts @@ -181,6 +181,41 @@ describe('validateFlowNodeWrites', () => { expect(findings[0].hint).toMatch(/Did you mean (one of: )?'stage'/); }); + // [#13858] This rule GATES (severity `error`), so its message is what an + // author reads while their build is refused — the one place a wrong causal + // story costs the most. It used to say "on a SQL datasource the driver + // rejects the whole statement ('no such column') … on a schemaless one the + // stray key is persisted". Measured through the real AutomationEngine, the + // real builtin CRUD nodes, a real ObjectQL engine and BOTH families + // (driver-sql on better-sqlite3, driver-memory): neither happens. Both + // answered `INVALID_FIELD` / 400, "Unknown field 'stagee' on object 'deal'", + // the node folded that into `create_record(deal) failed: …`, the run failed, + // and nothing was stored on either family — no row on create, an untouched + // row and no shadow column on update. + it('states the measured refusal — INVALID_FIELD / 400 on every datasource — and no driver split', () => { + const [finding] = validateFlowNodeWrites({ + objects: [dealObject], + flows: [flowWith({ stagee: 'won' })], + }); + + expect(finding.message).toContain('INVALID_FIELD / 400'); + expect(finding.message).toContain('identically on every datasource'); + expect(finding.message).toContain('before any statement is built'); + // Why the door answers and not a datasource: the node hands `fields` + // straight to the data engine, so it is a caller payload. + expect(finding.message).toContain('ordinary caller payload'); + // The severity's own justification, unchanged by the rewrite and still + // stated: the refusal is WHOLE, so correctly named siblings are lost too. + expect(finding.message).toContain('never land either'); + expect(finding.message).toContain('the step fails the run'); + + // The retired driver split, both halves. + expect(finding.message).not.toMatch(/no such column/); + expect(finding.message).not.toMatch(/schemaless/); + expect(finding.message).not.toMatch(/is persisted/); + expect(finding.message).not.toMatch(/Nothing between the node and storage/); + }); + it('flags every unknown key in one node, and only those', () => { const findings = validateFlowNodeWrites({ objects: [dealObject], diff --git a/packages/lint/src/validate-flow-node-writes.ts b/packages/lint/src/validate-flow-node-writes.ts index 7a52736901..fae867443a 100644 --- a/packages/lint/src/validate-flow-node-writes.ts +++ b/packages/lint/src/validate-flow-node-writes.ts @@ -26,27 +26,31 @@ // // And the runtime consequence is not the benign "consumer skips the unknown // name and does the rest" that keeps `page-field-unknown` / `form-field-unknown` -// advisory. Nothing between the node and storage removes the key: the flow -// executor calls the data engine directly (bypassing the metadata-protocol -// ingress, which strips `readonly` — not unknown — keys anyway), the engine's -// write paths strip only readonly/readonlyWhen, and the SQL driver's -// `formatInput` / `applyWriteColumnMap` pass an unrecognized key straight -// through (`m[k] ?? k`). Every branch below was measured, not inferred: +// advisory. 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 — and [#13858] the declared-field door (#8682 insert, #8738 +// update) refuses a caller-named undeclared key from the object's field map +// before any statement is built. Every branch below was measured through the +// real AutomationEngine, the real builtin CRUD nodes, the real engine and BOTH +// driver families (driver-sql on better-sqlite3, driver-memory), not inferred: // -// • Through the engine, an undeclared key reaches `driver.update` / -// `driver.create` verbatim, alongside the audit stamps. -// • On SQLite/knex an UPDATE becomes `update "deal" set "name" = 'n2', -// "stagee" = 'won' … → no such column: stagee`. The statement is rejected -// WHOLE: `name` — spelled correctly, in the same payload — does not land -// either, and the step fails with a driver error naming a column, far from -// the authoring mistake. -// • An INSERT fails the same way (`table deal has no column named stagee`), -// and one notch harder: the row is never created at all, so every later -// node that expected `{.id}` is working from a record that does not -// exist. -// • On a schemaless datasource (memory, MongoDB) nothing rejects it, so the -// stray key is persisted into a column the object never declares — where no -// schema-driven read surface will return it. +// • Both families answer identically — `INVALID_FIELD` / 400, "Unknown field +// 'stagee' on object 'deal'". No driver is reached, so there is no split to +// observe. +// • The write is refused WHOLE: `name` — spelled correctly, in the same +// payload — does not land either. +// • On `create_record` the row is never created at all, so every later node +// that expected `{.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: Unknown field 'stagee' on object 'deal'`), +// so the RUN fails — far from the authoring mistake, which is exactly why +// an author-time rule is still worth having. +// +// ⚠️ Until #13858 this block described the pre-#8682 driver split (SQL rejected +// the statement, a schemaless datasource persisted the stray key). That is +// retired, not merely restated: the severity below is unchanged because neither +// the old outcome nor the new one is ever "the rest still works". // // No outcome is "the rest still works". That is the same call // `validate-searchable-fields` makes for a stale entry and @@ -290,12 +294,18 @@ export function validateFlowNodeWrites(stack: AnyRec): FlowNodeWriteFinding[] { where: `flow "${flowName}" › ${nodeWhere}`, path: `${nodePath}.config.fields.${fieldName}`, message: - `${node.type} writes '${fieldName}', but object '${objectName}' declares no such field. Nothing ` + - `between the node and storage removes the key: on a SQL datasource the driver rejects the whole ` + - `statement ('no such column'), so the correctly named fields in this same payload never land ` + - `either${ + // [#13858] The node hands `fields` to the data engine directly + // (`data.insert` / `data.update` in service-automation's + // crud-nodes), so it is a CALLER payload and the #8682/#8738 + // declared-field door refuses it before any datasource is reached. + // Measured on driver-sql and driver-memory alike. + `${node.type} writes '${fieldName}', but object '${objectName}' declares no such field. The ` + + `node hands its fields map to the engine as an ordinary caller payload, so the ` + + `declared-field door REFUSES the whole write — INVALID_FIELD / 400, identically on every ` + + `datasource, before any statement is built. The correctly named fields in this same payload ` + + `never land either${ node.type === 'create_record' ? ' and the record is never created at all' : '' - }; on a schemaless one the stray key is persisted into a column no read surface returns.`, + }, and the step fails the run.`, hint: fixHint(fieldName, [...known]), }); } diff --git a/packages/lint/src/validate-hook-body-writes.test.ts b/packages/lint/src/validate-hook-body-writes.test.ts index 6cea10bd03..eabc442142 100644 --- a/packages/lint/src/validate-hook-body-writes.test.ts +++ b/packages/lint/src/validate-hook-body-writes.test.ts @@ -246,6 +246,43 @@ describe('validateHookBodyWrites — ctx.api writes', () => { expect(findings[0].hint).toContain("'email'"); }); + // [#13858] The message is the whole product of an advisory rule, so the + // sentence IS the deliverable. It used to promise a driver-dependent outcome + // ("on a SQL driver … a driver-level error; on a schemaless driver … the + // stray key is persisted"), which has not been true for this path since + // #8682/#8738: `ctx.api` is a ScopedContext over the running engine, so the + // payload is CALLER-supplied and the declared-field door refuses it first. + // + // Measured before this text was written — real QuickJS sandbox, real hook + // body, real ObjectQL, real driver-sql (better-sqlite3) AND real + // driver-memory: both families answered `INVALID_FIELD` / 400, "Unknown field + // 'stagee' on object 'deal'", the target row was untouched, and the memory + // family stored no shadow column. Same door the caller-payload half of + // `undeclared-field-write-driver-split.integration.test.ts` pins. + it('states the measured refusal — INVALID_FIELD / 400 on every driver — and no driver split', () => { + const [finding] = validateHookBodyWrites( + stackWith("await ctx.api.object('crm_deal').update({ id, stag: 'won' });"), + ); + + // What the author actually gets, in the vocabulary #13657 landed for the + // `ctx.input` sibling one branch over — one door, one phrasing. + expect(finding.message).toContain('INVALID_FIELD / 400'); + expect(finding.message).toContain('identically on every driver'); + expect(finding.message).toContain('before any statement is built'); + // Why it is refused there rather than by a driver: the payload is a + // CALLER's, which is the fact the whole rewrite turns on. + expect(finding.message).toContain('ordinary CALLER write'); + // ...and the blast radius that makes an author-time rule worth having. + expect(finding.message).toContain('fails the operation that triggered the hook'); + + // The retired claim, in both halves. Neither may come back without a + // measurement saying it should. + expect(finding.message).not.toMatch(/driver-level error/); + expect(finding.message).not.toMatch(/schemaless/); + expect(finding.message).not.toMatch(/is persisted/); + expect(finding.message).not.toMatch(/write-path validator skips/); + }); + it('checks updateById payloads at argument 1, not 0', () => { const findings = validateHookBodyWrites( stackWith("await ctx.api.object('crm_deal').updateById(ctx.input.id, { stag: 'won' });"), diff --git a/packages/lint/src/validate-hook-body-writes.ts b/packages/lint/src/validate-hook-body-writes.ts index 5f2fcd5762..e5cc5a839c 100644 --- a/packages/lint/src/validate-hook-body-writes.ts +++ b/packages/lint/src/validate-hook-body-writes.ts @@ -18,6 +18,17 @@ // is refused `INVALID_FIELD` / 400 identically on every driver, before any // statement is built. // +// [#13858] The `ctx.api` example in that first sentence answers for a +// DIFFERENT reason, and it always did since #8682/#8738: `ctx.api` is a +// ScopedContext over the running engine (`ObjectQL.buildHookApi`), so its +// payload is CALLER-supplied and the PRE-hook declared-field door refuses it — +// the same `INVALID_FIELD` / 400, before the hooks and before any statement. +// `applyMutationsToInput` above describes the `ctx.input` half only. Measured +// for all three write-set rules (this one, `validate-action-body-writes.ts`, +// `validate-flow-node-writes.ts`) through a real QuickJS sandbox, a real +// engine and both driver families: neither half reaches a driver any more, so +// no message in this family describes a driver split. +// // ⚠️ That does NOT retire this rule — it changes what it is worth. The runtime // refusal arrives at WRITE time, on whichever record first exercises the // branch; this rule arrives at AUTHOR time and names the field, the object and @@ -863,9 +874,17 @@ 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 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).`, + // [#13858] ctx.api is a ScopedContext over the running engine + // (`ObjectQL.buildHookApi`), so this payload is CALLER-supplied and + // the declared-field door (#8682 insert, #8738 update) is what + // refuses it. Measured on both families; the ids stay in comments + // rather than in the string, which reaches authors and operators + // who cannot resolve a tracker number. + `object '${w.object}' declares no such field. ctx.api is a scoped handle on the running ` + + `engine, so the payload arrives as an ordinary CALLER write and the declared-field door ` + + `REFUSES it at run time — INVALID_FIELD / 400, identically on every driver (#4271), before ` + + `any statement is built. The nested write lands nothing, and the refusal escapes the body ` + + `and fails the operation that triggered the hook.`, hint: fixHint(w.field, [...known]), }); }