From 3f6c246d2ddf5bc2be0bbd3871f18bfe0c800905 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 24 Sep 2026 18:23:47 +0000 Subject: [PATCH 1/4] fix(objectql): a field-rule predicate that faults refuses the write (ADR-0137 D2) [wip] Claude-Session: https://claude.ai/code/session_019c3Hi6ZMU1p6m6aA6Bz45d Co-authored-by: Claude --- packages/objectql/src/cel-fault.ts | 10 +- packages/objectql/src/master-detail.ts | 14 +- .../objectql/src/validation/rule-validator.ts | 307 ++++++++++++++---- 3 files changed, 255 insertions(+), 76 deletions(-) diff --git a/packages/objectql/src/cel-fault.ts b/packages/objectql/src/cel-fault.ts index d66fa2b0233..b530ac2a553 100644 --- a/packages/objectql/src/cel-fault.ts +++ b/packages/objectql/src/cel-fault.ts @@ -11,10 +11,12 @@ * writing them twice — the same argument that put {@link * ./declared-fields.js#materializeDeclaredFields} in front of every server-side * evaluator: two when this module was written, THREE since #4953 added the - * field `readonlyWhen` strips. That third seam is fail-open rather than - * rejecting, so it reads only {@link unknownVariableOf} (the #4889 - * unbound-root branch) and never {@link describeCelFault}'s rejection - * sentences. + * field `readonlyWhen` strips. That third seam, and the field `requiredWhen` + * block beside it, were fail-open until ADR-0137 D2 made a faulting field-rule + * predicate refuse the write; both now word that refusal through + * {@link describeCelFault} too, via `rule-validator.ts`'s one refusal builder. + * The `readonlyWhen` strip still reads {@link unknownVariableOf} first — the + * #4889 unbound-root branch, which LOCKS rather than refusing. * * This module has no opinion about what a caller does with a fault. It answers * three questions and hands back a sentence: diff --git a/packages/objectql/src/master-detail.ts b/packages/objectql/src/master-detail.ts index 05d5f3b4b2f..d5f26b09bc2 100644 --- a/packages/objectql/src/master-detail.ts +++ b/packages/objectql/src/master-detail.ts @@ -28,13 +28,13 @@ * gate in `@objectstack/lint` (`validate-expressions`) rejects at authoring * time — for `readonlyWhen` since #4889 and for `requiredWhen` since #4977. * - * The two runtimes then part ways on the unbindable case, deliberately: an - * unbound `parent` leaves a `readonlyWhen` field LOCKED (#4889 — refusing to - * wave a declared lock through), while a `requiredWhen` stays fail-OPEN - * (#4977 — a 422 on a write whose header is merely unreadable was ruled too - * loud, and left to the next review of ADR-0058 D5). That asymmetry is why the - * build-time gate covers BOTH slots: it is the only thing standing between an - * unbindable `requiredWhen` and a requirement that enforces nothing in silence. + * The two runtimes then part ways on the unbindable case: an unbound `parent` + * leaves a `readonlyWhen` field LOCKED (#4889 — refusing to wave a declared + * lock through), while a `requiredWhen` REFUSES the write (ADR-0137 D2 — the + * review of ADR-0058 D5 that #4977 left the 422 to; until then it stayed + * fail-OPEN). Neither is a runtime an author wants to meet on every write, which + * is why the build-time gate covers BOTH slots: the unbindable declaration is + * refused at authoring rather than at the first write. * * ## The tolerance, and the measurement behind it * diff --git a/packages/objectql/src/validation/rule-validator.ts b/packages/objectql/src/validation/rule-validator.ts index 7fb0dd6f56f..5bfc1762634 100644 --- a/packages/objectql/src/validation/rule-validator.ts +++ b/packages/objectql/src/validation/rule-validator.ts @@ -94,7 +94,9 @@ * thrown exception is an engine fault the author has no remedy for (rejecting * on it would brick every write with nothing to fix). Step 1 makes the * field-level predicates evaluate far more often anyway, since their fault mode - * was the same missing key. + * was the same missing key. (ADR-0137 D2 has since moved `requiredWhen` and + * `readonlyWhen` off that list — see the section of that name below. Option + * `visibleWhen`, `format` and `json_schema` stay on it.) * * ## `readonlyWhen`: the UNBOUND-ROOT case is fail-CLOSED (#4889) * @@ -104,9 +106,10 @@ * header in hand — is not a broken predicate; it is a supported construct the * evaluation site could not answer, and answering "not locked" writes a field * the author declared frozen. That single case now resolves to LOCKED. Every - * OTHER `readonlyWhen` fault (undeclared key, null overload, parse error) keeps - * the fail-open policy this section describes, and `requiredWhen` / option - * `visibleWhen` are untouched. See {@link isReadonlyWhenLocked}. + * OTHER `readonlyWhen` fault (undeclared key, null overload, parse error) kept + * the fail-open policy this section describes until ADR-0137 D2, which REFUSES + * the write instead; this carve-out is unchanged by D2 and still LOCKS. See + * {@link isReadonlyWhenLocked}. * * This is a NARROWING of ADR-0058 D5's "non-security predicate ⇒ fail soft" * line, recorded as an addendum on that ADR alongside the same narrowing #4649 @@ -143,7 +146,7 @@ * at that moment) is a louder failure than the `readonlyWhen` case, where the * cost of the conservative answer is one refused field. That is option B of * #4977 and it was explicitly NOT taken; it is reserved for the next review - * of ADR-0058 D5. + * of ADR-0058 D5. **Superseded by ADR-0137 D2** — that review, below. * - **Catch it at BUILD time instead.** `@objectstack/lint`'s * `validate-expressions` rejects a `parent`-scoped `requiredWhen` on an object * that declares no single `master_detail`, so the unbindable declaration — @@ -162,10 +165,33 @@ * `record.x == null`. Pinned by test so the app-side `has(...)` idiom cannot be * broken silently from under it. * + * ## ADR-0137 D2 — a field-rule predicate that FAULTS refuses the submit + * + * ADR-0137 writes the field-rule row of ADR-0058 D5's failure table — the + * review the #4977 ruling reserved option B for — and its Context measured the + * two server arms above as the defect it rules on. Its D2: "At submit time, a + * field-rule predicate that cannot be evaluated refuses the write and names the + * field and the rule. Nothing is persisted." So both arms now refuse: + * + * - `requiredWhen` — every fault, an unbound `parent` included, adds a + * refusal to this call's `ValidationError` instead of `continue`-ing. + * - `readonlyWhen` — every fault EXCEPT the unbound-root carve-out above + * (which still LOCKS) throws a `ValidationError` from the strip, before + * anything is written. + * + * Both refusals are {@link unevaluableRuleError}'s envelope, the one a broken + * validation rule has carried since #4649, with the field as `field` and the + * slot as `constraint.rule`. What D2 does NOT reach: option `visibleWhen` + * ({@link evaluateOptionVisibility}) — D2 names a FIELD-rule predicate, an + * option's visibility is not one, and it stays fail-open; and the RENDER side, + * which D3 keeps fail-open for display. The consequence ADR-0137 names is + * deliberate: a stored predicate that silently did nothing now refuses writes, + * and that loud state is what reveals it. + * * ## `readonlyWhen` sees a TOTAL record too (#4953) * * The paragraph above ("Deliberately NOT changed here") is about the fail-open - * POLICY, and that policy is still what the field-level predicates use. What + * POLICY, which the field-level predicates kept until ADR-0137 D2. What * changed in #4953 is the other half — what the predicate is evaluated * AGAINST. `materializeDeclaredFields` was wired into two seams and not the * third: the strip functions on the write path merged `{ ...previous, ...data }` @@ -220,7 +246,7 @@ import { // that evaluate CEL against "the record" cannot drift apart on what that record // contains — see the module's own doc comment. import { materializeDeclaredFields } from '../declared-fields.js'; -import { describeCelFault, unknownVariableOf } from '../cel-fault.js'; +import { describeCelFault, missingKeyOf, unknownVariableOf } from '../cel-fault.js'; // [#19929] Which `record` fields a `readonlyWhen` predicate reads, from the // AST of the canonical parse — see `recordFieldsRead`. import { parseCelToAst } from '@objectstack/formula'; @@ -471,9 +497,11 @@ export function needsPriorRecord( * (#4649) — the one policy under which a rule that cannot be evaluated refuses * the write instead of waving it through. The field-level `requiredWhen` / * `readonlyWhen` / option `visibleWhen` predicates are deliberately NOT - * collected here: they fail OPEN, so a rule that could not be evaluated would - * silently not enforce their gate — the opposite of what this capability's - * refusal is for. They are their own card. + * collected here: this capability hydrates for a rule whose refusal is the + * point, and the field level is not hydrated at all — a field predicate that + * reads through a reference faults, and since ADR-0137 D2 that fault REFUSES + * the write with a sentence saying so ({@link unevaluableFieldRuleError}). + * Hydrating them is a capability of its own, not a consequence of D2. * * ## Only REFERENCE-typed fields * @@ -619,8 +647,8 @@ export type RelatedRecordBinding = Readonly> * predicates disagreed about what "the record" contains: ``requiredWhen: * P`record.b != null` `` was a working guard while ``readonlyWhen: * P`record.b != null` `` on the same field faulted whenever the driver did not - * return `b` — and a faulting `readonlyWhen` is fail-OPEN, so the field the - * author declared frozen was written. Whether it was written depended on which + * return `b` — and a faulting `readonlyWhen` was fail-OPEN then, so the field + * the author declared frozen was written. Whether it was written depended on which * columns a driver happened to echo back, which is not something an author can * see or control (#4953; maintainer ruling 2026-08-06: the SERVER seams are * unified, the cross-process ones are deferred). @@ -638,8 +666,9 @@ export type RelatedRecordBinding = Readonly> * the only place holding both the master's schema and the just-read header: * `ObjectQL.resolveMasterDetailParent(s)` (`engine.ts#materializeParentHeader`), * which serves this seam and the `requiredWhen` one below from one resolution. - * This function's own contract is unchanged — hand it a sparse header and it - * still fails open — and the ABSENT-parent signal above is untouched, because + * This function's own contract is unchanged — hand it a sparse header and its + * predicate still faults (which, since ADR-0137 D2, refuses the write) — and + * the ABSENT-parent signal above is untouched, because * materialisation is only ever applied to a header row that EXISTS. * * ## Consequences, both directions (measured, not asserted) @@ -663,8 +692,9 @@ export type RelatedRecordBinding = Readonly> * consequence rather than a discovery. * * Ordering comparisons still fault over a total record (`null < null` is `no - * such overload`), so the fail-open branch is not dead — the very reason - * `@objectstack/lint`'s null-guard gate exists. + * such overload`), so the fault branch is not dead — the very reason + * `@objectstack/lint`'s null-guard gate exists. Since ADR-0137 D2 that branch + * refuses the write rather than letting the change through. * * ## Only materialise when the persisted state is IN HAND * @@ -867,8 +897,10 @@ function isCallerSuppliedValue( * `undefined` when the object is not a detail or the payload's predicates never * name `parent`, and the binding is simply absent. * - * A predicate that faults is fail-open (the change is allowed through) EXCEPT - * when the fault is an unbound scope root — see {@link isReadonlyWhenLocked}. + * A predicate that faults REFUSES the write — a `ValidationError` naming the + * field and the rule, thrown before anything is persisted (ADR-0137 D2) — + * EXCEPT when the fault is an unbound scope root, which holds the lock (#4889). + * See {@link isReadonlyWhenLocked}. * * The `record` / `previous` bindings are TOTAL over the object's declared * fields (#4953) — see {@link readonlyWhenBindings}. `record` is the payload @@ -892,11 +924,11 @@ export function stripReadonlyWhenFields( const stored = options?.stored ?? data; const judged = judgedReadonlyWhenKeys(fields, data, supplied); if (judged.length === 0 || !judgesOnly(judged, options?.only)) return data; - const settled = settleReadonlyWhenDrops( + const settled = settleOrRefuse(logger, () => settleReadonlyWhenDrops( readonlyWhenLockGroups(fields, judged), (dropped) => readonlyWhenBindings(withoutKeys(stored, dropped), previous, fields), - (name, view, warn) => isReadonlyWhenLocked(fields[name]!, view.merged, view.previous, name, { warn }, parent), - ); + (name, view, warn) => isReadonlyWhenLocked(fields[name]!, view.merged, view.previous, name, { warn }, parent, fields), + )); return applyReadonlyWhenDrops(data, judged, settled, options?.only, logger, (name) => `Field '${name}' is read-only (readonlyWhen) — ignoring incoming change`, ); @@ -1290,16 +1322,27 @@ function applyReadonlyWhenDrops( * dropped. Shared by the single-id ({@link stripReadonlyWhenFields}) and bulk * ({@link stripReadonlyWhenFieldsMulti}) strips. * - * ## Two faults, two answers (#4889) + * ## Two faults, two answers (#4889, ADR-0137 D2) * * Until #4889 every fault took one exit — WARN and `false`, "not locked" — and * that single answer had to serve two very different situations: * * - **The predicate is broken on this record.** A typo'd key, a `null` - * ordering overload, a parse error. The author has a bug; the field is not - * demonstrably locked; the historical (and deliberate, documented) policy is - * fail-OPEN. Unchanged here — an engine fault the author cannot act on must - * not brick every write to the object. + * ordering overload, a parse error, a column read through a reference the + * field level never hydrates. The author has a bug and the lock has no + * verdict. This arm was fail-OPEN — WARN, `false`, the change allowed + * through — which wrote a field the author declared frozen whenever the + * predicate could not run. **ADR-0137 D2 closes it: the write is REFUSED**, + * naming the field and the rule, before anything is persisted. It THROWS + * the {@link ValidationError} rather than answering a boolean, because + * neither boolean is true: `false` invents "unlocked", and `true` would + * silently drop a value nobody showed to be locked. The refusal is what the + * author can act on — the message says which key or which overload. + * + * Thrown from INSIDE the settlement ({@link settleReadonlyWhenDrops}) on + * purpose: a lock whose verdict is unknown on any view the settlement asks + * about cannot be settled, and a partial settlement is the thing the + * settlement exists to prevent. * * - **The predicate names a ROOT this operation did not bind.** `parent.status * == 'paid'` where no master-detail header was resolved. The expression is @@ -1328,13 +1371,15 @@ function isReadonlyWhenLocked( name: string, logger?: EvaluateRulesOptions['logger'], parent?: ParentBinding, + fields?: Record, ): boolean { const res = ExpressionEngine.evaluate(toExpression(def.readonlyWhen!), { record: merged, previous, // Bound ONLY when resolved. An absent binding is what makes the unbound-root // fault below reachable, and that fault is the signal — binding `null` here - // would turn it into a `No such key` and re-open the fail-open hole. + // would turn it into a `No such key`, which the arm after it REFUSES + // (ADR-0137 D2) instead of holding the lock. ...(parent != null ? { extra: { parent } } : {}), }); if (!res.ok) { @@ -1347,12 +1392,33 @@ function isReadonlyWhenLocked( ); return true; } - logger?.warn?.(`readonlyWhen for '${name}' failed to evaluate — change allowed through`); - return false; + // [ADR-0137 D2] Every OTHER fault refuses the write — see the docblock. + throw new ValidationError([ + unevaluableFieldRuleError('readonlyWhen', name, res.error, def.readonlyWhen!, fields), + ]); } return res.value === true; } +/** + * [ADR-0137 D2] Run a `readonlyWhen` settlement, and say a refusal in the log + * before handing it to the caller — the operator needs the fault in the log even + * though the caller gets it in the response, as a broken validation rule's + * refusal is (#4649). Logged HERE rather than where the fault is read, because + * {@link settleReadonlyWhenDrops} defers every warning until a verdict is + * settled, and a refusal settles none. + */ +function settleOrRefuse(logger: EvaluateRulesOptions['logger'] | undefined, settle: () => T): T { + try { + return settle(); + } catch (err) { + if (err instanceof ValidationError) { + for (const field of err.fields) logger?.warn?.(field.message); + } + throw err; + } +} + /** * True when at least one `readonlyWhen` predicate the UPDATE payload touches * reads the `parent` root (#4889) — the gate the engine uses to decide whether @@ -1506,9 +1572,12 @@ export function hasReadonlyWhenInPayload( * lock the field for some rows and write it for others, so a field locked in any * target row is fail-safe-dropped for all (narrow the `where` to reach the rows * where it is unlocked). A field NO matched row locks is written normally — a - * legitimate bulk edit of an unlocked conditional field is unaffected. A broken - * predicate is fail-open for that row. INSERT is exempt (update path only), - * symmetric with the single-id strip. + * legitimate bulk edit of an unlocked conditional field is unaffected. A + * predicate that faults on ANY matched row refuses the whole write, naming the + * field, the rule and the row (ADR-0137 D2) — so every row is judged, not only + * the rows up to the first that locks: stopping there would let the refusal + * depend on the order the driver returned the rows in. INSERT is exempt (update + * path only), symmetric with the single-id strip. * * `parentForRow` (#4889) supplies each matched row's master-detail header, since * N rows can hang off N different masters — the bulk counterpart of the @@ -1559,26 +1628,52 @@ export function stripReadonlyWhenFieldsMulti( // in every row against THAT row's view with the other drops reverted, and is // locked when it locks in ≥1 row — so an exact set drops only keys locked in // some row and keeps only keys unlocked in every row. - const settled = settleReadonlyWhenDrops( + const settled = settleOrRefuse(logger, () => settleReadonlyWhenDrops( readonlyWhenLockGroups(fields, judged), (dropped) => { const payload = withoutKeys(stored, dropped); return rows.map((row) => readonlyWhenBindings(payload, row, fields)); }, - (name, views, warn) => - views.some((view, i) => - isReadonlyWhenLocked( - fields[name]!, - view.merged, - view.previous, - name, - { warn }, - // Resolved per (field, row) exactly as before — the header lookup is - // the caller's, and its call pattern is not this change's business. - parentForRow?.(rows[i] ?? undefined), - ), - ), - ); + (name, views, warn) => { + // [ADR-0137 D2] EVERY row, never `some`: a row that faults refuses the + // write wherever it sits among the matched rows. The rows' warnings are + // said once each — N rows under one unbound header word one line N times. + const said = new Set(); + const once = (message: string): void => { + if (said.has(message)) return; + said.add(message); + warn(message); + }; + let locked = false; + views.forEach((view, i) => { + const row = rows[i] ?? undefined; + try { + if ( + isReadonlyWhenLocked( + fields[name]!, + view.merged, + view.previous, + name, + { warn: once }, + // Resolved per (field, row) exactly as before — the header lookup is + // the caller's, and its call pattern is not this change's business. + parentForRow?.(row), + fields, + ) + ) { + locked = true; + } + } catch (err) { + // Name the row, as the bulk validation refusal does (`engine.ts`). + if (err instanceof ValidationError && row?.id != null) { + throw new ValidationError(err.fields.map((f) => ({ ...f, message: `${f.message} (record ${String(row.id)})` }))); + } + throw err; + } + }); + return locked; + }, + )); return applyReadonlyWhenDrops(data, judged, settled, options?.only, logger, (name) => `Field '${name}' is read-only (readonlyWhen) in ≥1 matched row — ignoring incoming change on bulk update`, ); @@ -2997,8 +3092,9 @@ export function evaluateValidationRules( // Field-level conditional rules (B2): a field whose `requiredWhen` // predicate is TRUE over the merged record must have a value — enforced // server-side so the rule can't be bypassed. (`readonlyWhen` is handled by - // stripReadonlyWhenFields on the write path, not here.) A broken predicate - // is fail-open (logged, skipped). + // stripReadonlyWhenFields on the write path, not here.) A predicate that + // cannot be evaluated REFUSES the write, naming the field and the rule + // (ADR-0137 D2) — see the fault arm below. // // ADR-0113 non-regression: reject iff the MERGED state violates AND the // PRE state complied. A write may not take the record from compliant to @@ -3024,22 +3120,27 @@ export function evaluateValidationRules( if (!pred) continue; const res = ExpressionEngine.evaluate(toExpression(pred), { record: merged, previous, ...parentScope }); if (!res.ok) { - // Fail-OPEN, unchanged (#4977 ruling: bind the scope, keep the - // evaluation semantics). An unevaluable `requiredWhen` — including one - // whose `parent` the engine could not resolve — is logged and skipped, - // NOT turned into a rejection: that is option B, deliberately not taken - // here and left to the next review of ADR-0058 D5. All that changes is - // the diagnostic: an unbound ROOT is named, because "the header could - // not be read" and "the author typo'd a key" are different faults with - // different remedies and only one line of signal to tell them apart. - const unbound = unknownVariableOf(res.error); + // [ADR-0137 D2] A `requiredWhen` that cannot be evaluated REFUSES the + // write and names the field and the rule; nothing is persisted (the + // caller throws below, before any driver call). Until D2 this arm + // logged and `continue`d — #4977 kept it fail-OPEN and left the + // rejection ("option B") to the next review of ADR-0058 D5. ADR-0137 is + // that review: it writes the field-rule row of D5's table, and its own + // Context measured this arm ("A record saves with the field empty") as + // the defect it rules on. A rule that could not run has no verdict, and + // reading "no verdict" as "not required" is the whole defect. + // + // Every fault takes this arm, an unbound `parent` included — a header + // that could not be resolved for this write is exactly the relationship + // read #18682's ruling says must fail loudly, never silently true. + // Refused whatever the write does to the field: D2 refuses the SUBMIT, + // and a supplied value does not supply the missing verdict. The + // ADR-0113 pre-check below is not consulted either — it asks whether a + // legacy row may rest under a verdict, and there is none to rest under. opts.logger?.warn?.( - unbound - ? `requiredWhen for '${name}' reads '${unbound}', which is not bound for this operation — ` + - `skipped (the requirement is NOT enforced for this write). ` + - `A 'parent'-scoped predicate needs the object to declare exactly one master_detail relationship.` - : `requiredWhen for '${name}' failed to evaluate — skipped`, + `requiredWhen for '${name}' failed to evaluate (${res.error.kind}: ${String(res.error.message).split('\n')[0]}) — write rejected`, ); + errors.push(unevaluableFieldRuleError('requiredWhen', name, res.error, pred, fields)); continue; } if (res.value === true && isMissing(merged[name])) { @@ -3321,22 +3422,35 @@ function checkStateMachine( * evaluators that reject a write for the same reason must not describe it in * two dialects — the same argument that made `materializeDeclaredFields` * shared. + * + * [ADR-0137 D2] The field-rule predicates (`requiredWhen` / `readonlyWhen`) + * refuse through this SAME builder — {@link unevaluableFieldRuleError} passes + * the `subject` that names the field and the slot instead of a rule name, and + * nothing else about the envelope moves: one refusal shape for every predicate + * the server could not run. */ function unevaluableRuleError( ruleName: string, field: string, error: { kind: string; message: string }, what: 'predicate' | 'when-predicate', + subject: { prose: string; detail?: string } = { prose: `Validation rule '${ruleName}'` }, ): FieldValidationError { - const { summary, missingKey, nullOverload, detail } = describeCelFault(error, { + const described = describeCelFault(error, { what, undeclaredKeyFix: "fix the rule's condition, or declare the field", }); + const { summary, nullOverload } = described; + // A subject that words its own detail has read the fault more precisely than + // the generic sentence can (a key read THROUGH a reference is not a key this + // object fails to declare), so the generic `missingKey` does not travel with it. + const detail = subject.detail ?? described.detail; + const missingKey = subject.detail === undefined ? described.missingKey : undefined; return { field, code: 'rule_violation', message: - `Validation rule '${ruleName}' could not be evaluated (${summary}) — write rejected.${detail}`, + `${subject.prose} could not be evaluated (${summary}) — write rejected.${detail}`, constraint: { rule: ruleName, reason: 'unevaluable', @@ -3347,6 +3461,69 @@ function unevaluableRuleError( }; } +/** The two field-rule slots the server evaluates on a write (ADR-0137 D2). */ +type FieldRuleSlot = 'requiredWhen' | 'readonlyWhen'; + +/** + * [ADR-0137 D2] The refusal a field-rule predicate that CANNOT BE EVALUATED at + * submit produces: it names the field and the rule, and the write is rejected + * with nothing persisted. + * + * Built by {@link unevaluableRuleError}, so the envelope is the one a broken + * validation rule has produced since #4649 — `code: 'rule_violation'`, and + * `constraint.reason: 'unevaluable'` — with `constraint.rule` naming the SLOT + * (`requiredWhen` / `readonlyWhen`). A slot name cannot collide with a + * validation rule's name: a rule name is a snake_case machine name, and both + * slot names are camelCase. + * + * Two faults get their own sentence, because the generic one would send the + * author to the wrong repair: + * + * - **A column read THROUGH a reference field** (`record.account.tier`). The + * related record is never read for a field-level predicate — only a + * validation rule's condition is hydrated one hop — so the reference holds a + * bare id and CEL reports `No such key: tier`. "Declare the field" would have + * the author add `tier` to the wrong object. + * - **An unbound `parent`**: the master-detail header could not be resolved for + * this write. The generic sentence lists only `record` / `previous` as + * roots, which is the wrong half of the story for a `parent`-scoped rule. + */ +function unevaluableFieldRuleError( + slot: FieldRuleSlot, + name: string, + error: { kind: string; message: string }, + pred: string | Expression, + fields: Record | undefined, +): FieldValidationError { + const expr = toExpression(pred); + const source = expr.dialect === 'cel' && typeof expr.source === 'string' ? expr.source : ''; + let detail: string | undefined; + const missing = missingKeyOf(error); + if (missing && source && fields) { + for (const [ref, related] of analysisFor(source)?.traversals ?? []) { + const target = referenceTargetOf(fields[ref]); + if (!target || !related.has(missing)) continue; + detail = + ` The predicate reads '${missing}' through '${ref}', a reference to '${target}'. A field-level` + + ` \`${slot}\` is evaluated against this record alone and never reads the related record` + + ' (only a `validations[]` rule\'s condition reads one hop through a reference), so the' + + ` reference holds a bare id there. Express the check as a \`validations[]\` \`script\` rule,` + + ' or read a column this object declares.'; + break; + } + } + if (detail === undefined && unknownVariableOf(error) === PARENT_ROOT) { + detail = + ` The predicate reads 'parent', the master-detail header, and no header could be resolved for` + + ' this write — the record names none, or it was not found or could not be read. The rule has' + + ' no verdict, so the write is rejected rather than allowed on an unchecked rule.'; + } + return unevaluableRuleError(slot, name, error, 'predicate', { + prose: `Field '${name}' ${slot}`, + ...(detail !== undefined ? { detail } : {}), + }); +} + /** * CEL predicate check (`script` / `cross_field`). The predicate expresses the * *failure* condition: if it evaluates TRUE the rule is violated. A predicate From bc55b48bfecac1e39f9abfb1442c6cc4c9c0133c Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 24 Sep 2026 18:36:37 +0000 Subject: [PATCH 2/4] fix(lint): name the field-rule refusal a faulting requiredWhen now produces (ADR-0137 D2) [wip] Claude-Session: https://claude.ai/code/session_019c3Hi6ZMU1p6m6aA6Bz45d Co-authored-by: Claude --- .../lint/src/validate-expressions.test.ts | 42 ++++++----- packages/lint/src/validate-expressions.ts | 74 ++++++++++--------- packages/lint/src/validate-null-guards.ts | 28 ++++--- 3 files changed, 80 insertions(+), 64 deletions(-) diff --git a/packages/lint/src/validate-expressions.test.ts b/packages/lint/src/validate-expressions.test.ts index 4b6dd2dfa64..d930c410e57 100644 --- a/packages/lint/src/validate-expressions.test.ts +++ b/packages/lint/src/validate-expressions.test.ts @@ -985,10 +985,11 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { }); // #4977 — the same gate, extended to the slot the same issue gave a server - // `parent` binding. `requiredWhen` stays FAIL-OPEN at runtime, so this build - // gate is the only thing that stops an unbindable declaration from shipping - // and enforcing nothing forever — which is why the message must name that - // consequence and not `readonlyWhen`'s opposite one. + // `parent` binding. `requiredWhen` REFUSES the write at runtime when the + // predicate cannot be evaluated (ADR-0137 D2; it was fail-OPEN until then), + // so this build gate is what stops an unbindable declaration from shipping + // and refusing writes in production — which is why the message must name + // that consequence and not `readonlyWhen`'s LOCKED one. describe('parent-scoped `requiredWhen` needs a resolvable master (#4977)', () => { const parentScopeIssues = (obj: Record) => validateStackExpressions({ objects: [obj] }).filter((i) => /reads `parent`/.test(i.message)); @@ -1006,8 +1007,9 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { expect(issues[0]!.where).toMatch(/field 'description' requiredWhen/); expect(issues[0]!.message).toMatch(/declares no `master_detail` relationships/); // The CONSEQUENCE clause is what separates this from its `readonlyWhen` - // twin: fail-open there, fail-closed here, opposite fixes. - expect(issues[0]!.message).toMatch(/the requirement would never be enforced/); + // twin: that one LOCKS the field, this one REFUSES the write. + expect(issues[0]!.message).toMatch(/writes would be refused/); + expect(issues[0]!.message).not.toMatch(/never be enforced/); expect(issues[0]!.message).not.toMatch(/locked on every write/); }); @@ -1883,10 +1885,11 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { expect(m).toMatch(/editable/); }); - it('`requiredWhen` — says the requirement is never enforced, not anything about visibility', () => { + it('`requiredWhen` — says the server refuses the write, not anything about visibility', () => { const m = messageFor('requiredWhen'); - expect(m).toMatch(/never enforced/); - expect(m).toMatch(/saves with the field empty/); + expect(m).toMatch(/server REFUSES a write/); + expect(m).not.toMatch(/never enforced/); + expect(m).not.toMatch(/saves with the field empty/); expect(m).not.toMatch(/VISIBLE/); expect(m).not.toMatch(/showing for everyone/); }); @@ -2517,9 +2520,9 @@ describe('null-guard gate (#4763)', () => { // #4811 — the one surface the coverage review found to MEET the gate's // totality criterion: `evaluateValidationRules` evaluates a field's // `requiredWhen` against the same `materializeDeclaredFields`-merged record - // the object's validation rules see. It is also the quietest failure of the - // three covered surfaces: a faulting `requiredWhen` is fail-OPEN (logged and - // skipped), so the field is simply never required and the write sails through. + // the object's validation rules see. It WAS the quietest failure of the + // three covered surfaces — a faulting `requiredWhen` was fail-OPEN (logged + // and skipped) — until ADR-0137 D2 made it refuse the write like the rest. describe('field `requiredWhen` — covered since #4811', () => { const withField = (requiredWhen: string) => validateStackExpressions({ @@ -2552,14 +2555,15 @@ describe('null-guard gate (#4763)', () => { }); // The consequence clause is per-surface, and getting it wrong sends the - // author to the wrong place. `requiredWhen` is fail-OPEN — `rule-validator` - // logs and skips — so it must NOT borrow the validation rules' "the write - // is rejected fail-closed" wording. - it('reports the fail-OPEN consequence, not the validation rules’ fail-closed one', () => { + // author to the wrong place. Since ADR-0137 D2 a faulting `requiredWhen` + // REFUSES the write — `rule-validator` no longer logs and skips — so it + // takes the validation rules' "the write is rejected fail-closed" wording + // and must NOT keep promising that the write goes through. + it('reports the fail-CLOSED consequence — a faulting requiredWhen refuses the write (ADR-0137 D2)', () => { const [issue] = withField('has(record.budget) && record.budget > 100'); - expect(issue.message).toContain('SKIPPED fail-open'); - expect(issue.message).toContain('the field is never actually required'); - expect(issue.message).not.toContain('rejected fail-closed'); + expect(issue.message).toContain('rejected fail-closed'); + expect(issue.message).not.toContain('SKIPPED fail-open'); + expect(issue.message).not.toContain('the field is never actually required'); }); it('leaves the fail-closed wording on the surfaces that really fail closed', () => { diff --git a/packages/lint/src/validate-expressions.ts b/packages/lint/src/validate-expressions.ts index fe17761521b..99b55164f06 100644 --- a/packages/lint/src/validate-expressions.ts +++ b/packages/lint/src/validate-expressions.ts @@ -527,13 +527,14 @@ function rulePredicates(rule: AnyRec, path: string): Array<{ label: string; raw: * * ## Why it is an error and not a warning * - * Every fault direction available here is silent, and two of the three are - * the opposite of what the author declared (see the per-slot table below): - * a `visibleWhen` written to HIDE leaves the field visible to everyone, a - * `readonlyWhen` written to unlock-under-a-condition locks the field on every - * write, and a `requiredWhen` simply never fires. None of the three produces - * a runtime error an author can find; the only signal that exists is this one - * (#6146). + * Every fault direction available here is either silent or the opposite of + * what the author declared (see the per-slot table below): a `visibleWhen` + * written to HIDE leaves the field visible to everyone, a `readonlyWhen` + * written to unlock-under-a-condition locks the field on every write, and a + * `requiredWhen` — silent until ADR-0137 D2 — now refuses every write that + * reaches the root, because that root is never bound there. Only the last + * produces a runtime error at all, and it produces it on the first write in + * production; build time is where the author should meet it (#6146). * * Verdict scope is the field level only. Per-option `visibleWhen` is checked * by the loop in the field walk and deliberately NOT passed through here: @@ -631,14 +632,15 @@ function rulePredicates(rule: AnyRec, path: string): Array<{ label: string; raw: * success, and the value silently never lands. The old sentence told this * author the field would be VISIBLE TO EVERYONE — the opposite failure, and * the opposite troubleshooting direction. - * - **`requiredWhen` — fail-OPEN at both ends, and never about visibility.** - * Server: the `requiredWhen` block logs `unknownVariableOf`'s name and - * `continue`s — #4977 deliberately did not copy #4889's carve-out, so the - * required-check is skipped for that write. Client: `fallback: false`, so - * the form does not mark the field required either. Both ends agree and - * both do nothing: the requirement is never enforced anywhere, and a record - * saves with the field empty. "Falls back to VISIBLE" was not merely - * imprecise here, it named the wrong property of the field. + * - **`requiredWhen` — the server REFUSES, and never about visibility.** + * Server: a `requiredWhen` that cannot be evaluated refuses the write, + * naming the field and the rule (ADR-0137 D2). It was fail-OPEN until then + * — the block logged `unknownVariableOf`'s name and `continue`d, and a + * record saved with the field empty. Client: `fallback: false`, so the form + * does not mark the field required, and the save it submits is refused. An + * unbound root faults on every write that reaches it. "Falls back to + * VISIBLE" was not merely imprecise here, it named the wrong property of + * the field. * * `conditionalRequired` also reaches this helper (the field walk still passes * it). It is a `retiredKey` in `FieldSchema` — the strict schema rejects it by @@ -803,9 +805,10 @@ const FIELD_RULE_SLOT_CONSEQUENCE: Record = { 'field editable (`fallback: false`). The server is the one that decides: ' + 'the field looks writable, the save reports success, and the value silently never lands', requiredWhen: - 'the predicate faults and the requirement is never enforced anywhere — the server logs it ' + - 'and SKIPS the check (fail-open, #4977 deliberately did not take #4889\'s carve-out) and ' + - 'the form does not mark the field required either, so a record saves with the field empty', + 'the predicate faults on every write that reaches that root, and the server REFUSES a write ' + + 'whose requirement it cannot evaluate (ADR-0137 D2: the refusal names the field and the ' + + 'rule) — while the form does not mark the field required, so nothing on screen says why the ' + + 'save failed', // Listed rather than left to the `??` below, so the map covers every slot // the field walk passes and the default stays unreachable. `FieldSchema` // declares this key only as a `retiredKey`, which rejects it by name, so @@ -1640,13 +1643,13 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { // What they do NOT share is the CONSEQUENCE, and the message has to name // the right one or it prescribes the wrong fix (the same reason #4811's // null-guard gate passes its outcome in explicitly instead of inferring - // it). The two runtimes fail in OPPOSITE directions on an unbindable - // `parent`: `readonlyWhen` fails CLOSED (#4889 — an unbound scope root - // resolves to LOCKED, so the field becomes unwritable forever), while - // `requiredWhen` stays fail-OPEN (#4977's ruling deliberately did not copy - // the carve-out), so the requirement silently enforces NOTHING. This gate - // is why fail-open is affordable there: the declaration that would rot - // unnoticed at runtime cannot ship in the first place. + // it). The two runtimes answer an unbindable `parent` differently: + // `readonlyWhen` fails CLOSED (#4889 — an unbound scope root resolves to + // LOCKED, so the field becomes unwritable forever), while `requiredWhen` + // REFUSES the write (ADR-0137 D2 — it was fail-OPEN until then, #4977) + // wherever the predicate reads `parent`. Either way the declaration is + // unusable, and this gate is where the author meets it instead of the + // first write in production. // // `conditionalRequired` (retired alias) and `visibleWhen` (no // server-enforced `parent` binding of its own) keep their verdicts @@ -1658,7 +1661,7 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { // indexed read here would have disarmed that scan silently. for (const [slot, raw, consequence] of [ ['readonlyWhen', f.readonlyWhen, `the field would be locked on every write`], - ['requiredWhen', f.requiredWhen, `the requirement would never be enforced — the predicate faults, the server logs and skips it, and the field stays optional in the database`], + ['requiredWhen', f.requiredWhen, `writes would be refused — the predicate faults wherever it reads \`parent\`, and the server refuses a write whose requirement it cannot evaluate`], ] as const) { const source = celSourceOf(raw); if (masters === 1 || !source || !readsParentRoot(source)) continue; @@ -1679,11 +1682,12 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { // null-guard gate's totality criterion: `evaluateValidationRules` // evaluates it against the SAME `materializeDeclaredFields`-merged // record the object's validation rules see. It is also the surface - // where an unguarded predicate hurts most quietly — a faulting - // `requiredWhen` is fail-OPEN (`rule-validator.ts` logs - // "failed to evaluate — skipped"), so the field is simply never - // required and the write sails through. Validation rules at least - // reject fail-closed since #4761. + // where an unguarded predicate used to hurt most quietly — a faulting + // `requiredWhen` was fail-OPEN, so the field was simply never required + // and the write sailed through. Since ADR-0137 D2 it refuses the write + // (`rule-validator.ts`: "failed to evaluate … — write rejected"), the + // same outcome as a validation rule's since #4761, so it takes the + // `'fail-closed'` clause below. // // `readonlyWhen` is still NOT included even though it sits on the same // field — but no longer because its binding is sparse. Since #6454 @@ -1691,9 +1695,9 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { // `materializeDeclaredFields`, so `!= null` IS the right prescription // there now; what holds the wiring back is clause 3 of the #4953 ruling // (both server-side seams first — the flow trigger-record half is - // outstanding). When it is wired it needs `'fail-open'`, like - // `requiredWhen` above and unlike the validation-rule surface. Same for - // `conditionalRequired` / `visibleWhen`, which have no record-scoped + // outstanding). When it is wired it needs `'fail-closed'`, like + // `requiredWhen` above: a faulting `readonlyWhen` refuses the write since + // ADR-0137 D2 too. `conditionalRequired` / `visibleWhen` have no record-scoped // total binding of their own. See the surface ledger in // `validate-null-guards.ts`. checkNullGuards( @@ -1701,7 +1705,7 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { `field '${fname}' requiredWhen`, f.requiredWhen, objectName, - 'fail-open', + 'fail-closed', ); if (f.expression) { // `expression` is the key `FieldSchema` declares for a computed field — diff --git a/packages/lint/src/validate-null-guards.ts b/packages/lint/src/validate-null-guards.ts index 0ef6c22972a..d35b5e617c1 100644 --- a/packages/lint/src/validate-null-guards.ts +++ b/packages/lint/src/validate-null-guards.ts @@ -119,15 +119,16 @@ * * Two facts to carry into that widening; neither is bookkeeping: * - * 1. **The fail policy is the OPPOSITE of the two surfaces #4763 wired.** - * Validation rules and hook `condition`s are fail-CLOSED. A faulting - * `readonlyWhen` is fail-OPEN: `isReadonlyWhenLocked` logs - * `failed to evaluate — change allowed through`, and the field the - * author declared frozen is WRITTEN. (One exception, #4889 — a fault - * naming an UNBOUND ROOT resolves to LOCKED.) So this face wants - * {@link nullGuardMessage}'s `'fail-open'` outcome for the same reason - * the `requiredWhen` row already carries it: the damage is a declared - * lock that silently enforces nothing, not a rejected write. + * 1. **The fail policy is now the SAME as the two surfaces #4763 wired.** + * Validation rules and hook `condition`s are fail-CLOSED, and since + * ADR-0137 D2 so is a faulting `readonlyWhen`: `isReadonlyWhenLocked` + * refuses the write, naming the field and the rule. (Until then it + * logged `failed to evaluate — change allowed through` and the field + * the author declared frozen was WRITTEN. One exception stands, #4889: + * a fault naming an UNBOUND ROOT resolves to LOCKED.) So this face + * wants {@link nullGuardMessage}'s `'fail-closed'` outcome, as the + * `requiredWhen` row does since the same decision: the damage is a + * refused write, not a declared lock that silently enforces nothing. * 2. **Making the binding total moved one verdict the OTHER way.** On a * total record `has(record.)` is uniformly TRUE and * `!has(record.)` uniformly FALSE, so a lock spelled @@ -581,7 +582,14 @@ export function findUnguardedNullableOperands( export type NullGuardOutcome = /** Validation rules + hook conditions: the fault propagates, the write is refused (#4761). */ | 'fail-closed' - /** Field `requiredWhen`: `rule-validator.ts` logs and skips, so nothing is enforced (#4811). */ + /** + * A surface whose runtime logs and SKIPS the aborted predicate, so nothing is + * enforced. Written for the field `requiredWhen` (#4811), which no longer + * behaves this way: since ADR-0137 D2 a faulting `requiredWhen` refuses the + * write and takes `'fail-closed'`. No surface passes this outcome today, and + * its clause below still describes the `requiredWhen` runtime it was written + * for — re-word it before a surface that genuinely skips adopts it. + */ | 'fail-open'; const OUTCOME_CLAUSE: Record = { From b25e969e1db2c3c1b71a58aed47c991079e3e66c Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 24 Sep 2026 18:48:03 +0000 Subject: [PATCH 3/4] test(objectql): pin the ADR-0137 D2 refusal and move the fail-open pins it retires [wip] Claude-Session: https://claude.ai/code/session_019c3Hi6ZMU1p6m6aA6Bz45d Co-authored-by: Claude --- ...eld-rule-predicate-fault-refuses-submit.md | 79 +++++ .../src/engine-field-predicate-fault.test.ts | 331 ++++++++++++++++++ ...readonly-when-interdependent-locks.test.ts | 28 +- .../src/engine-readonly-when-parent.test.ts | 42 ++- .../src/engine-reference-tenant-scope.test.ts | 47 ++- .../src/engine-required-when-parent.test.ts | 65 ++-- .../src/validation/rule-fail-closed.test.ts | 22 +- .../src/validation/rule-validator.test.ts | 159 +++++---- 8 files changed, 642 insertions(+), 131 deletions(-) create mode 100644 .changeset/19727-field-rule-predicate-fault-refuses-submit.md create mode 100644 packages/objectql/src/engine-field-predicate-fault.test.ts diff --git a/.changeset/19727-field-rule-predicate-fault-refuses-submit.md b/.changeset/19727-field-rule-predicate-fault-refuses-submit.md new file mode 100644 index 00000000000..dc90080fff3 --- /dev/null +++ b/.changeset/19727-field-rule-predicate-fault-refuses-submit.md @@ -0,0 +1,79 @@ +--- +'@objectstack/objectql': minor +'@objectstack/lint': patch +--- + +fix(objectql)!: a field-level `requiredWhen` / `readonlyWhen` predicate that cannot be evaluated now REFUSES the write, naming the field and the rule, instead of letting it through (ADR-0137 D2) + +Clause-②: no (narrowing) + +**BREAKING**: shipped as `minor` under the launch-window convention +(`check-changeset-no-major` refuses `major` until GA). The banner and the ADR-0087 +disposition below carry the breaking change, not the level. + +**Writes that used to save now fail.** ADR-0137 D2 says: "At submit time, a +field-rule predicate that cannot be evaluated refuses the write and names the +field and the rule. Nothing is persisted." The server now enforces that on the +two arms that let such a write through: + +- **`requiredWhen`**: a predicate that faults used to be logged + (`requiredWhen for '' failed to evaluate — skipped`), and the record + saved with the field empty. It now refuses the insert or update. This covers + every fault, including a `parent`-scoped rule whose master-detail header + could not be resolved for the write. +- **`readonlyWhen`**: a predicate that faults used to be logged + (`failed to evaluate — change allowed through`), and the field the author + declared frozen was written. It now refuses the update. On a bulk update, a + fault in any matched row refuses the whole write, and the refusal names that + row. One case is unchanged: a predicate that faults because the header it + reads as `parent` could not be resolved still holds the lock, as before. + +The refusal is the same `ValidationError` a broken validation rule has thrown +since #4649: `VALIDATION_FAILED`, served as `400`. Its entry for the field +carries `code: 'rule_violation'` and +`constraint: { rule: 'requiredWhen' | 'readonlyWhen', reason: 'unevaluable', fault }`, +with `missingKey` or `hint: 'null-comparison'` when the fault is one of those. +The message names the field and the rule. It is refused before anything is +written, on insert, single-id update and bulk update alike. The operator also +gets a `warn` line saying the write was rejected. + +The refusal applies to the whole submit. A `requiredWhen` whose predicate +faults refuses the write even when the write supplies the field, because the +rule has no verdict to judge that value against. + +**What starts refusing.** A stored predicate that faults on the writes it +judges: + +- a key the object does not declare, usually a typo (`record.statsu`); +- an ordering comparison or arithmetic over a `null` (`record.amount > 100` + where `amount` is empty). Guard it with `!= null`. `has(x)` is true for a + declared field holding null, so it does not guard this; +- a column read through a lookup (`record.account.tier`). The field level never + reads the related record, so the reference holds a bare id there. The refusal + says so, and names the reference and its target object; +- an envelope with no evaluable `source`: blank, or `ast`-only. + +Nothing in this repository's own metadata is affected. A census of every +`requiredWhen` / `readonlyWhen` under `packages/`, `examples/` and `apps/` +found none that faults on a write it judges. How many stored predicates in a +deployment fault is unknown, and ADR-0137 names that as the point: the loud +state is what finds them. + +**Fix.** Read the refusal. It names the field, the rule, and the key or +overload that faulted. Then correct the predicate: fix the key's spelling, +guard the null operand with `!= null`, or move a check that reads through a +lookup into a `validations[]` `script` rule, whose condition does read one hop +through a reference. + +Unchanged: a predicate that evaluates is judged exactly as before, in both +directions. So is the ADR-0113 legacy-row rule for an evaluated `requiredWhen`. +Option-level `visibleWhen` is not a field-rule predicate, so D2 does not reach +it, and it stays fail-open. The render side is not touched here (ADR-0137 D3 +keeps its directions for display). + +`@objectstack/lint`: the build-time messages for a field `requiredWhen` no +longer say the server "skips" a faulting predicate. The unbound-root message, +the `parent`-without-a-master message and the null-guard message now say the +server refuses the write. + + diff --git a/packages/objectql/src/engine-field-predicate-fault.test.ts b/packages/objectql/src/engine-field-predicate-fault.test.ts new file mode 100644 index 00000000000..c478b8aff25 --- /dev/null +++ b/packages/objectql/src/engine-field-predicate-fault.test.ts @@ -0,0 +1,331 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// ADR-0137 D2 — a field-rule predicate that FAULTS refuses the SUBMIT. +// +// "At submit time, a field-rule predicate that cannot be evaluated refuses the +// write and names the field and the rule. Nothing is persisted." The server +// half of that decision lands here, on the two arms ADR-0137's own Context +// measured as fail-open: +// +// - `requiredWhen` — the block logged and `continue`d, so a record saved with +// the field empty; +// - `readonlyWhen` — every fault but the unbound-root one logged `change +// allowed through`, so a field the author declared frozen was written. +// +// Driven end-to-end through the real engine and a real (in-memory) driver, so +// "nothing is persisted" is read off the store rather than inferred from a +// throw (PD #10: check the CALL SITE, bulk path included). The two CONTROL +// blocks pin what must NOT move: a predicate that evaluates behaves exactly as +// before in both directions, and option `visibleWhen` — which D2 does not +// reach — stays fail-open. + +import { describe, it, expect, beforeEach } from 'vitest'; +import { validationFailureDetails } from '@objectstack/types'; +import { ObjectQL } from './engine.js'; + +function makeDriver() { + const stores = new Map>(); + const storeFor = (o: string) => { + let s = stores.get(o); + if (!s) { s = new Map(); stores.set(o, s); } + return s; + }; + const checkOp = (value: any, cond: any): boolean => { + if (cond === null || typeof cond !== 'object' || Array.isArray(cond) || cond instanceof Date) { + return value === cond; + } + return Object.entries(cond).every(([op, target]: [string, any]) => { + switch (op) { + case '$eq': return value === target; + case '$ne': return value !== target; + case '$in': return Array.isArray(target) && target.includes(value); + default: return true; + } + }); + }; + const matches = (row: any, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + return Object.entries(where).every(([k, v]: [string, any]) => { + if (k === '$and') return (v as any[]).every((w) => matches(row, w)); + if (k === '$or') return (v as any[]).some((w) => matches(row, w)); + if (k === '$not') return !matches(row, v); + return checkOp(row?.[k], v); + }); + }; + let n = 0; + const driver: any = { + name: 'memory', version: '0.0.0', supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; }, + async find(object: string, ast: any) { + return Array.from(storeFor(object).values()).filter((r) => matches(r, ast?.where)); + }, + async findOne(object: string, ast: any) { + for (const r of storeFor(object).values()) if (matches(r, ast?.where)) return r; + return null; + }, + async create(object: string, data: Record) { + n += 1; + const id = (data.id as string) ?? `r_${n}`; + const row = { ...data, id }; + storeFor(object).set(id, row); + return row; + }, + async update(object: string, id: string, data: Record) { + const s = storeFor(object); + const row = { ...s.get(id), ...data, id }; + s.set(id, row); + return row; + }, + async updateMany(object: string, ast: any, data: Record) { + const s = storeFor(object); + let count = 0; + for (const row of [...s.values()]) { + if (!matches(row, ast?.where)) continue; + s.set(row.id, { ...row, ...data, id: row.id }); + count += 1; + } + return count; + }, + async delete(object: string, id: string) { return storeFor(object).delete(id); }, + async count() { return 0; }, + async bulkCreate(object: string, rows: Record[]) { + return Promise.all(rows.map((r) => this.create(object, r, undefined))); + }, + async bulkUpdate() { return []; }, async bulkDelete() {}, + async beginTransaction() { return { __trx: true, commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, storeFor }; +} + +/** The error a write threw, or a failure saying it was accepted. */ +async function refusalOf(run: () => Promise): Promise { + let thrown: unknown; + let threw = false; + try { + await run(); + } catch (err) { + threw = true; + thrown = err; + } + expect(threw).toBe(true); + return thrown as any; +} + +/** + * The D2 envelope, asserted as a whole: the error is the engine's + * `ValidationError` (`VALIDATION_FAILED`, which the HTTP boundary serves as a + * 400 through `validationFailureDetails`), and it carries ONE field entry that + * names the field and the rule — `code: 'rule_violation'`, the envelope a broken + * validation rule has carried since #4649, with the SLOT as `constraint.rule`. + */ +function expectFieldRuleRefusal(err: any, field: string, slot: 'requiredWhen' | 'readonlyWhen') { + expect(err.name).toBe('ValidationError'); + expect(err.code).toBe('VALIDATION_FAILED'); + expect(validationFailureDetails(err)).toMatchObject({ code: 'VALIDATION_FAILED' }); + const entry = (err.fields as any[]).find((f) => f.field === field); + expect(entry).toMatchObject({ + field, + code: 'rule_violation', + constraint: expect.objectContaining({ rule: slot, reason: 'unevaluable' }), + }); + // Names the field and the rule, in the prose too. + expect(entry.message).toContain(`'${field}'`); + expect(entry.message).toContain(slot); + return entry; +} + +describe('ADR-0137 D2 — a faulting field-rule predicate refuses the submit (server half)', () => { + let engine: ObjectQL; + let storeFor: ReturnType['storeFor']; + + beforeEach(async () => { + engine = new ObjectQL(); + const d = makeDriver(); + storeFor = d.storeFor; + engine.registerDriver(d.driver, true); + await engine.init(); + engine.registry.registerObject({ + name: 'fp_account', + fields: { name: { type: 'text' }, tier: { type: 'text' } }, + } as any, 'test-package'); + engine.registry.registerObject({ + name: 'fp_ticket', + fields: { + status: { type: 'text' }, + // An author typo — `statsu` is declared nowhere. The rule can never run. + resolution: { type: 'text', requiredWhen: "record.statsu == 'closed'" }, + }, + } as any, 'test-package'); + engine.registry.registerObject({ + name: 'fp_order', + fields: { + account: { type: 'lookup', reference: 'fp_account' }, + // Reads THROUGH a lookup. The field level never hydrates the related + // record (only a validation rule's condition does), so `account` holds a + // bare id here and the predicate faults on every write that reaches it. + po_number: { type: 'text', requiredWhen: "record.account.tier == 'enterprise'" }, + }, + } as any, 'test-package'); + engine.registry.registerObject({ + name: 'fp_invoice', + fields: { + status: { type: 'text' }, + // The typo, on a lock. + amount: { type: 'number', readonlyWhen: "record.statsu == 'paid'" }, + }, + } as any, 'test-package'); + engine.registry.registerObject({ + name: 'fp_batch', + fields: { + cap: { type: 'number' }, + // Evaluates on a row that has a `cap`; faults (`null > int`) on one that does not. + amount: { type: 'number', readonlyWhen: 'record.cap > 100' }, + }, + } as any, 'test-package'); + engine.registry.registerObject({ + name: 'fp_control', + fields: { + status: { type: 'text' }, + reason: { type: 'text', requiredWhen: "record.status == 'closed'" }, + amount: { type: 'number', readonlyWhen: "record.status == 'paid'" }, + }, + } as any, 'test-package'); + engine.registry.registerObject({ + name: 'fp_option', + fields: { + tier: { + type: 'select', + options: [{ value: 'basic' }, { value: 'gold', visibleWhen: "record.statsu == 'vip'" }], + }, + }, + } as any, 'test-package'); + storeFor('fp_account').set('acc_1', { id: 'acc_1', name: 'Acme', tier: 'enterprise' }); + }); + + const rows = (object: string) => [...storeFor(object).values()]; + + // ── (a) requiredWhen ────────────────────────────────────────────────────── + + describe('(a) a `requiredWhen` whose predicate faults', () => { + it('refuses an INSERT, naming the field and the rule — and nothing is stored', async () => { + const err = await refusalOf(() => engine.insert('fp_ticket', { status: 'closed' })); + const entry = expectFieldRuleRefusal(err, 'resolution', 'requiredWhen'); + // The fault itself travels machine-readably, as a broken validation rule's does. + expect(entry.constraint).toMatchObject({ missingKey: 'statsu' }); + expect(rows('fp_ticket')).toHaveLength(0); + }); + + it('refuses an UPDATE — and the stored row is untouched', async () => { + storeFor('fp_ticket').set('t1', { id: 't1', status: 'open', resolution: 'n/a' }); + const err = await refusalOf(() => engine.update('fp_ticket', { id: 't1', status: 'closed', resolution: '' })); + expectFieldRuleRefusal(err, 'resolution', 'requiredWhen'); + expect(storeFor('fp_ticket').get('t1')).toEqual({ id: 't1', status: 'open', resolution: 'n/a' }); + }); + + it('refuses the submit even when the write supplies the field — a value is not a verdict', async () => { + // D2 refuses the SUBMIT. The rule could not run, so whether this field + // was required is unknown; a supplied value does not supply that answer. + const err = await refusalOf(() => engine.insert('fp_ticket', { status: 'closed', resolution: 'fixed' })); + expectFieldRuleRefusal(err, 'resolution', 'requiredWhen'); + expect(rows('fp_ticket')).toHaveLength(0); + }); + + it('refuses a predicate that reads THROUGH a lookup, and says why instead of "declare the field"', async () => { + const err = await refusalOf(() => engine.insert('fp_order', { account: 'acc_1' })); + const entry = expectFieldRuleRefusal(err, 'po_number', 'requiredWhen'); + // The generic sentence would tell the author to declare `tier` on + // `fp_order` — the wrong object. The refusal names the reference instead. + expect(entry.message).toContain("through 'account', a reference to 'fp_account'"); + expect(entry.message).not.toContain('which this object does not declare'); + expect(entry.constraint).not.toHaveProperty('missingKey'); + expect(rows('fp_order')).toHaveLength(0); + }); + + it('refuses on the BULK path too, before any matched row is written', async () => { + storeFor('fp_ticket').set('b1', { id: 'b1', status: 'open', resolution: 'a' }); + storeFor('fp_ticket').set('b2', { id: 'b2', status: 'open', resolution: 'b' }); + const err = await refusalOf(() => + engine.update('fp_ticket', { status: 'closed' }, { where: { status: 'open' }, multi: true } as any)); + expectFieldRuleRefusal(err, 'resolution', 'requiredWhen'); + expect(rows('fp_ticket').map((r) => r.status)).toEqual(['open', 'open']); + }); + }); + + // ── (b) readonlyWhen ────────────────────────────────────────────────────── + + describe('(b) a `readonlyWhen` whose predicate faults on a non-root key', () => { + it('refuses the UPDATE — the value is neither written nor silently dropped', async () => { + storeFor('fp_invoice').set('i1', { id: 'i1', status: 'paid', amount: 100 }); + const err = await refusalOf(() => engine.update('fp_invoice', { id: 'i1', amount: 999 })); + const entry = expectFieldRuleRefusal(err, 'amount', 'readonlyWhen'); + expect(entry.constraint).toMatchObject({ missingKey: 'statsu' }); + expect(storeFor('fp_invoice').get('i1')).toEqual({ id: 'i1', status: 'paid', amount: 100 }); + }); + + it('refuses a BULK update when ANY matched row faults, naming that row', async () => { + // `bad` faults (no cap ⇒ `null > int`) and arrives FIRST; `ok` evaluates + // (cap 500 ⇒ locked). + storeFor('fp_batch').set('bad', { id: 'bad', cap: null, amount: 1 }); + storeFor('fp_batch').set('ok', { id: 'ok', cap: 500, amount: 1 }); + expect(rows('fp_batch').map((r) => r.id)).toEqual(['bad', 'ok']); + const err = await refusalOf(() => + engine.update('fp_batch', { amount: 9 }, { where: { amount: 1 }, multi: true } as any)); + const entry = expectFieldRuleRefusal(err, 'amount', 'readonlyWhen'); + expect(entry.message).toContain('(record bad)'); + expect(rows('fp_batch').map((r) => r.amount)).toEqual([1, 1]); + }); + + it('refuses whatever order the matched rows arrive in — a locking row ahead does not hide the fault', async () => { + // The same two rows, the LOCKING one first. Judging rows only until the + // first that locks would drop `amount` for the batch and never read `bad`. + storeFor('fp_batch').set('ok', { id: 'ok', cap: 500, amount: 1 }); + storeFor('fp_batch').set('bad', { id: 'bad', cap: null, amount: 1 }); + expect(rows('fp_batch').map((r) => r.id)).toEqual(['ok', 'bad']); + const err = await refusalOf(() => + engine.update('fp_batch', { amount: 9 }, { where: { amount: 1 }, multi: true } as any)); + expectFieldRuleRefusal(err, 'amount', 'readonlyWhen'); + expect(rows('fp_batch').map((r) => r.amount)).toEqual([1, 1]); + }); + }); + + // ── (c) CONTROL: a predicate that evaluates behaves exactly as before ───── + + describe('(c) CONTROL — an evaluable predicate is judged exactly as before, both directions', () => { + it('requiredWhen TRUE and the field empty ⇒ refused as `required`, not as a fault', async () => { + const err = await refusalOf(() => engine.insert('fp_control', { status: 'closed' })); + expect(err.name).toBe('ValidationError'); + expect(err.fields).toContainEqual(expect.objectContaining({ field: 'reason', code: 'required' })); + expect((err.fields as any[]).some((f) => f.code === 'rule_violation')).toBe(false); + expect(rows('fp_control')).toHaveLength(0); + }); + + it('requiredWhen FALSE ⇒ accepted with the field empty', async () => { + const row = await engine.insert('fp_control', { status: 'open' }); + expect(row).toMatchObject({ status: 'open' }); + expect(rows('fp_control')).toHaveLength(1); + }); + + it('readonlyWhen TRUE ⇒ the change is dropped and the write lands (no refusal)', async () => { + storeFor('fp_control').set('c1', { id: 'c1', status: 'paid', reason: 'r', amount: 100 }); + await engine.update('fp_control', { id: 'c1', amount: 999 }); + expect(storeFor('fp_control').get('c1')).toMatchObject({ amount: 100 }); + }); + + it('readonlyWhen FALSE ⇒ the change is written', async () => { + storeFor('fp_control').set('c2', { id: 'c2', status: 'open', reason: 'r', amount: 100 }); + await engine.update('fp_control', { id: 'c2', amount: 999 }); + expect(storeFor('fp_control').get('c2')).toMatchObject({ amount: 999 }); + }); + }); + + // ── (d) CONTROL: option `visibleWhen` is outside D2 ─────────────────────── + + describe('(d) CONTROL — option `visibleWhen` is not a field-rule predicate, and stays fail-open', () => { + it('a faulting option predicate still lets the choice through', async () => { + const row = await engine.insert('fp_option', { tier: 'gold' }); + expect(row).toMatchObject({ tier: 'gold' }); + expect(rows('fp_option')).toHaveLength(1); + }); + }); +}); diff --git a/packages/objectql/src/engine-readonly-when-interdependent-locks.test.ts b/packages/objectql/src/engine-readonly-when-interdependent-locks.test.ts index 148b83b9447..fea6e5532ce 100644 --- a/packages/objectql/src/engine-readonly-when-interdependent-locks.test.ts +++ b/packages/objectql/src/engine-readonly-when-interdependent-locks.test.ts @@ -247,7 +247,8 @@ describe('a value one readonlyWhen lock drops no longer unlocks another (#19911) tag: { type: 'text' }, }, } as any); - // An FK whose own lock faults (text compared with a number): fail-open. + // An FK whose own lock faults (text compared with a number): the write is + // refused (ADR-0137 D2 — it was fail-open until then). engine.registry.registerObject({ name: 'line_fault', fields: { @@ -520,10 +521,27 @@ describe('a value one readonlyWhen lock drops no longer unlocks another (#19911) expect(reads).toEqual(['inv_a']); }); - it('a landing FK is re-judged with the rest, and its fail-open fault is said ONCE', async () => { - await engine.update('line_fault', { id: 'f1', invoice: 'inv_b', amount: 5 }); - expect(row('line_fault', 'f1')).toMatchObject({ invoice: 'inv_b', amount: 5 }); - expect(warns.filter((w) => w.includes("readonlyWhen for 'invoice' failed to evaluate"))).toHaveLength(1); + it('a landing FK whose own lock faults REFUSES the write (ADR-0137 D2), and the refusal is said ONCE', async () => { + // Until D2 the fault was fail-open: the FK landed and the fault was said + // once, however many times the settlement judged it. The fault now refuses + // the write where the settlement first meets it — the FK's own judgement, + // before the header it names is bound for the rest — and still says so once. + const before = { ...row('line_fault', 'f1') }; + let err: any; + try { + await engine.update('line_fault', { id: 'f1', invoice: 'inv_b', amount: 5 }); + } catch (e) { + err = e; + } + expect(err?.name).toBe('ValidationError'); + expect(err?.code).toBe('VALIDATION_FAILED'); + expect(err.fields).toContainEqual(expect.objectContaining({ + field: 'invoice', + code: 'rule_violation', + constraint: expect.objectContaining({ rule: 'readonlyWhen', reason: 'unevaluable' }), + })); + expect(row('line_fault', 'f1')).toEqual(before); + expect(warns.filter((w) => w.includes("Field 'invoice' readonlyWhen could not be evaluated"))).toHaveLength(1); }); // ── The settlement in the other direction: stays → moves ───────────── diff --git a/packages/objectql/src/engine-readonly-when-parent.test.ts b/packages/objectql/src/engine-readonly-when-parent.test.ts index 1aec06b0363..9161118ed3c 100644 --- a/packages/objectql/src/engine-readonly-when-parent.test.ts +++ b/packages/objectql/src/engine-readonly-when-parent.test.ts @@ -144,7 +144,8 @@ describe('parent-scoped readonlyWhen is enforced server-side (#4889)', () => { has_guard: { type: 'text', readonlyWhen: 'has(parent.status)' }, // [#6457] The #4649 line, unmoved: materialisation covers the master's // DECLARED fields only, so an author typo on the header stays - // unevaluable — and therefore fail-OPEN — instead of reading as null. + // unevaluable — and therefore REFUSED since ADR-0137 D2 (fail-OPEN + // before it) — instead of reading as null. typo_guard: { type: 'text', readonlyWhen: 'parent.stauts == null' }, }, } as any); @@ -276,11 +277,11 @@ describe('parent-scoped readonlyWhen is enforced server-side (#4889)', () => { it('ROW 1 (header carries the key): evaluates as it always did, verdict unchanged', async () => { // INV-1003 carries `status: 'paid'`, so `parent.status == null` is FALSE and - // the field is writable. No fault ⇒ nothing on the fail-open channel. + // the field is writable. No fault ⇒ nothing on the refusal channel. const warns = await warningsDuring(() => engine.update('showcase_invoice_line', { id: 'line_paid', locked_until_status: 'written' })); expect(line('line_paid')).toMatchObject({ locked_until_status: 'written' }); - expect(warns.some((w) => w.includes('failed to evaluate — change allowed through'))).toBe(false); + expect(warns.some((w) => w.includes('readonlyWhen could not be evaluated'))).toBe(false); }); it('ROW 2 — THE FIX: a header missing the key now LOCKS instead of failing open', async () => { @@ -292,9 +293,9 @@ describe('parent-scoped readonlyWhen is enforced server-side (#4889)', () => { const warns = await warningsDuring(() => engine.update('showcase_invoice_line', { id: 'line_sparse', locked_until_status: 'forged' })); expect(line('line_sparse')).toMatchObject({ locked_until_status: 'kept' }); - // The verdict came from an EVALUATION: the fail-open exit is not on the - // channel at all… - expect(warns.some((w) => w.includes('failed to evaluate — change allowed through'))).toBe(false); + // The verdict came from an EVALUATION: the fault exit (a refusal since + // ADR-0137 D2) is not on the channel at all… + expect(warns.some((w) => w.includes('readonlyWhen could not be evaluated'))).toBe(false); expect(warns.some((w) => w.includes("Field 'locked_until_status' is read-only (readonlyWhen)"))).toBe(true); // …and it is NOT #4889's unbound-root exit either — `parent` IS bound here. // This is the assertion that keeps ROW 2 and ROW 3 distinguishable. @@ -331,7 +332,7 @@ describe('parent-scoped readonlyWhen is enforced server-side (#4889)', () => { const warns = await warningsDuring(() => engine.update('showcase_invoice_line', { id: 'line_sparse', quantity: 42 })); expect(line('line_sparse')).toMatchObject({ quantity: 42 }); - expect(warns.some((w) => w.includes('failed to evaluate — change allowed through'))).toBe(false); + expect(warns.some((w) => w.includes('readonlyWhen could not be evaluated'))).toBe(false); }); it('CONSEQUENCE: `has(parent.)` is uniformly TRUE — it locks even on a sparse header', async () => { @@ -342,13 +343,28 @@ describe('parent-scoped readonlyWhen is enforced server-side (#4889)', () => { expect(line('line_sparse')).toMatchObject({ has_guard: 'kept' }); }); - it('BOUNDARY: an UNDECLARED key on the header stays unevaluable — fail-OPEN (#4649 unmoved)', async () => { + it('BOUNDARY: an UNDECLARED key on the header stays unevaluable — and is REFUSED (#4649 unmoved, ADR-0137 D2)', async () => { // `parent.stauts` is a typo, not a sparse column. Materialising it would - // paper over the bug; it must stay reportable. - const warns = await warningsDuring(() => - engine.update('showcase_invoice_line', { id: 'line_sparse', typo_guard: 'written' })); - expect(line('line_sparse')).toMatchObject({ typo_guard: 'written' }); - expect(warns.some((w) => w.includes('failed to evaluate — change allowed through'))).toBe(true); + // paper over the bug; it must stay reportable — and since ADR-0137 D2 the + // report is a refused write naming the field and the rule, not a change + // let through with a log line. + let err: any; + const warns = await warningsDuring(async () => { + try { + await engine.update('showcase_invoice_line', { id: 'line_sparse', typo_guard: 'written' }); + } catch (e) { + err = e; + } + }); + expect(err?.name).toBe('ValidationError'); + expect(err?.code).toBe('VALIDATION_FAILED'); + expect(err.fields).toContainEqual(expect.objectContaining({ + field: 'typo_guard', + code: 'rule_violation', + constraint: expect.objectContaining({ rule: 'readonlyWhen', reason: 'unevaluable', missingKey: 'stauts' }), + })); + expect(line('line_sparse')).toMatchObject({ typo_guard: 'kept' }); + expect(warns.some((w) => w.includes("Field 'typo_guard' readonlyWhen could not be evaluated"))).toBe(true); }); it('does NOT mutate the stored header row — the materialised copy stays local', async () => { diff --git a/packages/objectql/src/engine-reference-tenant-scope.test.ts b/packages/objectql/src/engine-reference-tenant-scope.test.ts index 2f4d2f5252f..5ba49e927ac 100644 --- a/packages/objectql/src/engine-reference-tenant-scope.test.ts +++ b/packages/objectql/src/engine-reference-tenant-scope.test.ts @@ -447,6 +447,15 @@ describe('[#19808] the lookup existence probe is scoped to the caller\'s organiz * that exists nowhere gets — so every pair below answers the same, and the * controls prove the predicates still evaluate wherever the header is the * caller's to read. + * + * [ADR-0137 D2] What "absent" answers moved, and it moved ALIKE for every pair: + * a `requiredWhen` whose `parent` is unbound used to be skipped (fail-open), so + * a write under an unreadable header either committed or fell through to the + * reference guard's `header: reference_not_found`. It now refuses the write as + * `note: rule_violation` (`reason: 'unevaluable'`) — the rule has no verdict — + * and, because `evaluateValidationRules` runs before `assertReferencesResolve`, + * that refusal is the one the caller meets. The invariant these pins exist for + * is unchanged: a locked, an open and a missing header give one answer. */ describe('[#19837] the master-detail parent binding is scoped to the caller\'s organization', () => { let engine: ObjectQL; @@ -527,7 +536,9 @@ describe('[#19837] the master-detail parent binding is scoped to the caller\'s o const open = await refusalOf(() => insertLine('hy_open')); const nowhere = await refusalOf(() => insertLine('h_nowhere')); - const expected = { status: 400, code: 'VALIDATION_FAILED', fields: [{ field: 'header', code: 'reference_not_found' }] }; + // ADR-0137 D2: the unbound header leaves `note`'s requirement without a + // verdict, and that refusal precedes the reference guard's. + const expected = { status: 400, code: 'VALIDATION_FAILED', fields: [{ field: 'note', code: 'rule_violation' }] }; expect(locked).toBeInstanceOf(ValidationError); expect(envelopeOf(locked)).toEqual(expected); expect(envelopeOf(open)).toEqual(expected); @@ -545,7 +556,8 @@ describe('[#19837] the master-detail parent binding is scoped to the caller\'s o const locked = await refusalOf(() => repoint('hy_locked')); const open = await refusalOf(() => repoint('hy_open')); - const expected = { status: 400, code: 'VALIDATION_FAILED', fields: [{ field: 'header', code: 'reference_not_found' }] }; + // ADR-0137 D2: see the describe's docblock — alike, and the rule's refusal. + const expected = { status: 400, code: 'VALIDATION_FAILED', fields: [{ field: 'note', code: 'rule_violation' }] }; expect(envelopeOf(locked)).toEqual(expected); expect(envelopeOf(open)).toEqual(expected); expect(storeFor('pb_line').get('ln_x')?.header).toBe('hx_open'); @@ -594,16 +606,22 @@ describe('[#19837] the master-detail parent binding is scoped to the caller\'s o // Clearing `note` would violate `requiredWhen` against the locked header // (ADR-0113: the pre-state complied). It used to be refused for `hy_locked` // and committed for `hy_open`; the header is not the caller's to read, so - // both are now judged with `parent` unbound (#4977: fail-open) — alike. - await engine.update('pb_line', { note: '' }, { where: { id: 'ln_yl' }, context: MEMBER_X } as any); - await engine.update('pb_line', { note: '' }, { where: { id: 'ln_yo' }, context: MEMBER_X } as any); - // An OPERATOR on `id` is a predicate, so these take the bulk branch and its - // batch header read; a scalar `where.id` would route to the by-id branch - // even under `multi: true` (`resolveEngineUpdateDispatch`). - await engine.update('pb_line', { note: '' }, { where: { id: { $in: ['ln_bl'] } }, multi: true, context: MEMBER_X } as any); - await engine.update('pb_line', { note: '' }, { where: { id: { $in: ['ln_bo'] } }, multi: true, context: MEMBER_X } as any); - - expect(['ln_yl', 'ln_yo', 'ln_bl', 'ln_bo'].map((id) => lines.get(id)?.note)).toEqual(['', '', '', '']); + // both are judged with `parent` unbound — alike. Unbound was fail-open + // (#4977: both committed) until ADR-0137 D2; it now REFUSES both, the same + // envelope on every door, and nothing is written. + const refusals = [ + await refusalOf(() => engine.update('pb_line', { note: '' }, { where: { id: 'ln_yl' }, context: MEMBER_X } as any)), + await refusalOf(() => engine.update('pb_line', { note: '' }, { where: { id: 'ln_yo' }, context: MEMBER_X } as any)), + // An OPERATOR on `id` is a predicate, so these take the bulk branch and its + // batch header read; a scalar `where.id` would route to the by-id branch + // even under `multi: true` (`resolveEngineUpdateDispatch`). + await refusalOf(() => engine.update('pb_line', { note: '' }, { where: { id: { $in: ['ln_bl'] } }, multi: true, context: MEMBER_X } as any)), + await refusalOf(() => engine.update('pb_line', { note: '' }, { where: { id: { $in: ['ln_bo'] } }, multi: true, context: MEMBER_X } as any)), + ]; + + const expected = { status: 400, code: 'VALIDATION_FAILED', fields: [{ field: 'note', code: 'rule_violation' }] }; + for (const err of refusals) expect(envelopeOf(err)).toEqual(expected); + expect(['ln_yl', 'ln_yo', 'ln_bl', 'ln_bo'].map((id) => lines.get(id)?.note)).toEqual(['n', 'n', 'n', 'n']); }); it('lit controls: a same-organization `locked` header still requires `note`; an `open` one commits', async () => { @@ -656,12 +674,13 @@ describe('[#19837] the master-detail parent binding is scoped to the caller\'s o expect([...(read[0].options?.tenantIds ?? [])].sort()).toEqual([ORG_X, ORG_Y].sort()); // Lit control: a member whose set is org X alone cannot see the org-Y - // header, so it binds absent and the write answers `reference_not_found`. + // header, so it binds absent and the write answers as an absent header does + // (ADR-0137 D2: `note`'s requirement has no verdict, and is refused). const ONLY_X = { ...MEMBER_X, accessible_org_ids: [ORG_X] } as unknown as ExecutionContext; const outside = await refusalOf(() => engine.insert('pb_line', { id: 'ln_x_only', header: 'hy_locked' }, { context: ONLY_X } as any)); - expect(envelopeOf(outside)).toEqual({ status: 400, code: 'VALIDATION_FAILED', fields: [{ field: 'header', code: 'reference_not_found' }] }); + expect(envelopeOf(outside)).toEqual({ status: 400, code: 'VALIDATION_FAILED', fields: [{ field: 'note', code: 'rule_violation' }] }); }); it('the header read runs ELEVATED and TENANT-SCOPED — the two halves of `{ ...context, isSystem: true }`', async () => { diff --git a/packages/objectql/src/engine-required-when-parent.test.ts b/packages/objectql/src/engine-required-when-parent.test.ts index 1ed5ca3cd97..f10f7a5069c 100644 --- a/packages/objectql/src/engine-required-when-parent.test.ts +++ b/packages/objectql/src/engine-required-when-parent.test.ts @@ -15,10 +15,13 @@ // // The maintainer ruled A+C (2026-08-06): bind the scope, keep the evaluation // semantics FAIL-OPEN, and catch the unbindable declaration at build time -// instead (`@objectstack/lint`). Option B — 422 on an unresolvable header — was -// explicitly NOT taken, so the "header cannot be read" case below asserts the -// write is ACCEPTED. That is the deliberate asymmetry with #4889's fail-CLOSED -// twin, and it is pinned here so nobody "fixes" one into the other by accident. +// instead (`@objectstack/lint`). Option B — refuse the write on an unresolvable +// header — was NOT taken then and was left to the next review of ADR-0058 D5. +// ADR-0137 is that review: its D2 refuses the SUBMIT of a field-rule predicate +// that cannot be evaluated, naming the field and the rule, so the "header +// cannot be read" cases below now assert the write is REFUSED and nothing is +// stored. `readonlyWhen`'s #4889 twin still LOCKS instead — the two slots still +// answer an unbound header differently, and both answers are loud. // // Driven end-to-end through the real engine + a real driver, not through the // evaluator in isolation (PD #10: a `case` label is not enforcement — check the @@ -199,34 +202,42 @@ describe('parent-scoped requiredWhen is enforced server-side (#4977)', () => { expect(row).toMatchObject({ description: 'seat' }); }); - // ── parent MISSING — fail-OPEN (the deliberate asymmetry with #4889) ────── + // ── parent MISSING — REFUSED (ADR-0137 D2; #4889's twin LOCKS instead) ──── - it('is FAIL-OPEN when the header cannot be resolved (option B was NOT taken)', async () => { + it('REFUSES the write when the header cannot be resolved (ADR-0137 D2 — option B, taken)', async () => { // A stored row whose header is gone — #4889's own orphan fixture, and the // only spelling that reaches this branch (a DANGLING FK in an insert // payload is refused earlier by the #4441 reference guard, so the header // can only go missing under a row that already exists). // - // #4889's `readonlyWhen` twin treats an unbound `parent` as LOCKED. The - // ruling on #4977 explicitly declined the symmetric answer (reject the - // write), so here the requirement is skipped and the write lands — even - // though `description` is empty and the predicate, could it have been - // evaluated, might well have said it is required. + // #4977 left this arm fail-open (the requirement skipped, the write + // landed with `description` empty). The predicate has no verdict — it + // might well have said the field is required — so D2 refuses the write + // instead, and the stored row does not move. storeFor('showcase_invoice_line').set('orphan', { id: 'orphan', invoice: 'GONE', description: '', quantity: 1 }); - await engine.update('showcase_invoice_line', { id: 'orphan', quantity: 9 }); - expect(line('orphan')).toMatchObject({ quantity: 9, description: '' }); + const err = await rejectionOf(() => engine.update('showcase_invoice_line', { id: 'orphan', quantity: 9 })); + expect(err.name).toBe('ValidationError'); + expect(err.code).toBe('VALIDATION_FAILED'); + expect(err.fields).toContainEqual(expect.objectContaining({ + field: 'description', + code: 'rule_violation', + constraint: expect.objectContaining({ rule: 'requiredWhen', reason: 'unevaluable' }), + })); + expect(line('orphan')).toMatchObject({ quantity: 1, description: '' }); }); - it('names the unbound ROOT in the skip diagnostic (fail-open, but not silent)', async () => { + it('names the unbound HEADER in the refusal and in the log (loud, never skipped)', async () => { const warns: string[] = []; const base = (engine as any).logger; (engine as any).logger = new Proxy(base, { get: (t: any, k: string) => (k === 'warn' ? (m: string) => warns.push(String(m)) : t[k]), }); storeFor('showcase_invoice_line').set('orphan', { id: 'orphan', invoice: 'GONE', description: '', quantity: 1 }); - await engine.update('showcase_invoice_line', { id: 'orphan', quantity: 9 }); - expect(warns.some((w) => /requiredWhen for 'description' reads 'parent'/.test(w))).toBe(true); - expect(warns.some((w) => /NOT enforced/.test(w))).toBe(true); + const err = await rejectionOf(() => engine.update('showcase_invoice_line', { id: 'orphan', quantity: 9 })); + const entry = (err.fields as any[]).find((f) => f.field === 'description'); + expect(entry.message).toContain("reads 'parent', the master-detail header"); + expect(warns.some((w) => /requiredWhen for 'description' failed to evaluate/.test(w) && /write rejected/.test(w))).toBe(true); + expect(warns.some((w) => /NOT enforced/.test(w))).toBe(false); }); // ── repoint ─────────────────────────────────────────────────────────────── @@ -357,13 +368,13 @@ describe('parent-scoped requiredWhen is enforced server-side (#4977)', () => { // The `requiredWhen` half of the same change. #4977 bound the scope; what it // could not fix from here is what the bound header CONTAINS — a driver that // returns only the columns it stored hands over a header missing the very key - // the predicate reads, and a `requiredWhen` that faults is fail-OPEN, so the - // requirement silently enforces nothing. + // the predicate reads, and a `requiredWhen` that faulted was fail-OPEN then, so + // the requirement silently enforced nothing. // // Note which line moves and which does not. The middle row (header RESOLVED // but sparse) becomes evaluable and therefore ENFORCED. The bottom row (header - // UNRESOLVABLE) keeps #4977's deliberate fail-OPEN asymmetry with #4889 — - // option B was not taken here and is not taken here now either. + // UNRESOLVABLE) was not moved by #6457 — option B was not taken there. It was + // moved later, by ADR-0137 D2: an unevaluable requirement refuses the write. /** Every warning the engine emitted during one write. */ async function warningsDuring(run: () => Promise): Promise { @@ -422,15 +433,15 @@ describe('parent-scoped requiredWhen is enforced server-side (#4977)', () => { expect(warns.some((w) => /requiredWhen for 'reason' failed to evaluate/.test(w))).toBe(false); }); - it('ROW 3 (unresolvable header): still FAIL-OPEN, still names the unbound root (#4977 asymmetry)', async () => { - // The line #4977 drew and this issue does NOT move: a header that resolves - // to nothing leaves `parent` unbound, the predicate is skipped, and the - // write lands with the field empty. Option B (422) stays not taken. + it('ROW 3 (unresolvable header): REFUSED, naming the rule (ADR-0137 D2 moved it; #6457 did not)', async () => { + // The line #4977 drew and #6457 did NOT move: a header that resolves to + // nothing leaves `parent` unbound. ADR-0137 D2 is what moved it — the + // predicate has no verdict, so the write is refused and nothing is stored. storeFor('showcase_invoice_line').set('orphan', { id: 'orphan', invoice: 'GONE', reason: '', quantity: 1 }); const warns = await warningsDuring(() => engine.update('showcase_invoice_line', { id: 'orphan', quantity: 9 })); - expect(line('orphan')).toMatchObject({ quantity: 9, reason: '' }); - expect(warns.some((w) => /requiredWhen for 'reason' reads 'parent'/.test(w) && /NOT enforced/.test(w))).toBe(true); + expect(line('orphan')).toMatchObject({ quantity: 1, reason: '' }); + expect(warns.some((w) => /requiredWhen for 'reason' failed to evaluate/.test(w) && /write rejected/.test(w))).toBe(true); }); it('ADR-0113 still holds on the newly-evaluable predicate: a legacy row may rest', async () => { diff --git a/packages/objectql/src/validation/rule-fail-closed.test.ts b/packages/objectql/src/validation/rule-fail-closed.test.ts index 544d6ce7563..8e980a41fa4 100644 --- a/packages/objectql/src/validation/rule-fail-closed.test.ts +++ b/packages/objectql/src/validation/rule-fail-closed.test.ts @@ -385,14 +385,30 @@ describe('#4649 — unchanged neighbours', () => { .not.toThrow(); }); - it('a broken field-level `requiredWhen` stays fail-open (out of scope, deliberately)', () => { + // #4649 left the field-level `requiredWhen` out of scope, and it stayed + // fail-open until ADR-0137 D2 ruled the field-rule row: a field-rule predicate + // that cannot be evaluated refuses the write and names the field and the rule. + // It left this neighbour list for that reason — the dedicated pins live in + // `engine-field-predicate-fault.test.ts`. + it('a broken field-level `requiredWhen` is no longer a neighbour: it refuses (ADR-0137 D2)', () => { const schema = { fields: { a: { name: 'a', type: 'text', requiredWhen: { dialect: 'cel', source: 'this is (( not valid' } }, }, validations: [], }; - expect(() => evaluateValidationRules(schema, { a: null }, 'update', { previous: { a: null } })) - .not.toThrow(); + let err: unknown; + try { + evaluateValidationRules(schema, { a: null }, 'update', { previous: { a: null } }); + } catch (e) { + err = e; + } + expect(err).toBeInstanceOf(ValidationError); + expect((err as ValidationError).code).toBe('VALIDATION_FAILED'); + expect((err as ValidationError).fields).toContainEqual(expect.objectContaining({ + field: 'a', + code: 'rule_violation', + constraint: expect.objectContaining({ rule: 'requiredWhen', reason: 'unevaluable' }), + })); }); }); diff --git a/packages/objectql/src/validation/rule-validator.test.ts b/packages/objectql/src/validation/rule-validator.test.ts index 4a1cd7be4cc..e538fb7d652 100644 --- a/packages/objectql/src/validation/rule-validator.test.ts +++ b/packages/objectql/src/validation/rule-validator.test.ts @@ -210,19 +210,21 @@ describe('parent-scoped readonlyWhen (#4889)', () => { expect(warnings.some((w) => w.includes("reads 'parent'") && w.includes('LOCKED'))).toBe(true); }); - it('keeps fail-OPEN for a predicate that is simply broken (undeclared key)', () => { + it('REFUSES a predicate that is simply broken (undeclared key) — ADR-0137 D2', () => { // Not an unbound root — `record` IS bound, the key under it is not declared. - // #4649 deliberately left this fail-open for field predicates; #4889 must - // not have widened itself into that case. + // #4649 left this fail-open for field predicates and #4889 did not widen + // itself into it; ADR-0137 D2 is what closed it — the write is refused, + // naming the field and the rule, and the refusal is logged too. const warnings: string[] = []; - const out = stripReadonlyWhenFields( + const entry = fieldRuleRefusal(() => stripReadonlyWhenFields( { fields: { amount: { type: 'currency', readonlyWhen: "record.no_such_field == 'paid'" } } }, { amount: 999 }, { amount: 1 }, { warn: (m: string) => warnings.push(m) } as never, - ); - expect(out).toEqual({ amount: 999 }); - expect(warnings.some((w) => w.includes('change allowed through'))).toBe(true); + ), 'amount', 'readonlyWhen'); + expect(entry.constraint).toMatchObject({ missingKey: 'no_such_field' }); + expect(warnings.some((w) => w.includes("Field 'amount' readonlyWhen could not be evaluated"))).toBe(true); + expect(warnings.some((w) => w.includes('change allowed through'))).toBe(false); }); it('leaves a RECORD-scoped lock on the same object working unchanged', () => { @@ -303,6 +305,29 @@ const sentLineFields = { }, }; +/** + * [ADR-0137 D2] The refusal a field-rule predicate that cannot be evaluated + * produces: the thrown `ValidationError`'s entry for `field`, asserted to be + * the unevaluable-rule envelope for `slot`. Fails when nothing was thrown. + */ +function fieldRuleRefusal(run: () => unknown, field: string, slot: 'requiredWhen' | 'readonlyWhen') { + let err: unknown; + try { + run(); + } catch (e) { + err = e; + } + expect(err).toBeInstanceOf(ValidationError); + expect((err as ValidationError).code).toBe('VALIDATION_FAILED'); + const entry = (err as ValidationError).fields.find((f) => f.field === field); + expect(entry).toMatchObject({ + field, + code: 'rule_violation', + constraint: expect.objectContaining({ rule: slot, reason: 'unevaluable' }), + }); + return entry!; +} + /** Collect the ValidationError field codes, or `null` when the write is accepted. */ function violations( schema: unknown, @@ -333,34 +358,41 @@ describe('parent-scoped requiredWhen (#4977)', () => { }); it('accepts the write once the field is supplied', () => { - expect(violations(sentLineFields, { invoice: 'inv1', description: 'seat' }, 'insert', { + // `quantity` is supplied too: the row-scoped `note` rule orders it against + // 100, and over a null that faults — which since ADR-0137 D2 refuses the + // write instead of silently skipping `note`'s requirement. + expect(violations(sentLineFields, { invoice: 'inv1', description: 'seat', quantity: 1 }, 'insert', { parent: { id: 'inv1', status: 'sent' }, })).toBeNull(); }); - it('is FAIL-OPEN when `parent` could not be bound — the #4889 asymmetry', () => { - // The twin above resolves an unbound root to LOCKED. Here the ruling - // deliberately kept the historical fail-open exit: the requirement is - // skipped and the write is ACCEPTED. Do not "restore symmetry" — that is - // option B, reserved for the next review of ADR-0058 D5. + it('REFUSES when `parent` could not be bound — ADR-0137 D2 (option B, taken)', () => { + // The `readonlyWhen` twin resolves an unbound root to LOCKED. This arm kept + // the historical fail-open exit under #4977 and left the refusal ("option + // B") to the next review of ADR-0058 D5 — ADR-0137 is that review, and its + // D2 refuses the write, naming the field and the rule. const warnings: string[] = []; - expect(violations(sentLineFields, { invoice: 'inv1', quantity: 1 }, 'insert', { + const entry = fieldRuleRefusal(() => evaluateValidationRules(sentLineFields as never, { invoice: 'inv1', quantity: 1 }, 'insert', { logger: { warn: (m: string) => warnings.push(m) }, - })).toBeNull(); - expect(warnings.some((w) => w.includes("reads 'parent'") && w.includes('NOT enforced'))).toBe(true); + } as never), 'description', 'requiredWhen'); + // The unbound header is named as such — not as the generic record/previous scope. + expect(entry.message).toContain("reads 'parent', the master-detail header"); + expect(warnings.some((w) => w.includes("requiredWhen for 'description' failed to evaluate") && w.includes('write rejected'))).toBe(true); + expect(warnings.some((w) => w.includes('NOT enforced'))).toBe(false); }); - it('keeps the plain fail-open message for a predicate that is simply broken', () => { + it('REFUSES a predicate that is simply broken, naming the key and not `parent`', () => { // Not an unbound root — `record` IS bound, the key under it is undeclared. const warnings: string[] = []; - expect(violations( - { fields: { amount: { type: 'currency', requiredWhen: "record.no_such_field == 'x'" } } }, + const entry = fieldRuleRefusal(() => evaluateValidationRules( + { fields: { amount: { type: 'currency', requiredWhen: "record.no_such_field == 'x'" } } } as never, { amount: 1 }, 'insert', - { logger: { warn: (m: string) => warnings.push(m) } }, - )).toBeNull(); - expect(warnings.some((w) => w.includes('failed to evaluate — skipped'))).toBe(true); - expect(warnings.some((w) => w.includes("reads 'parent'"))).toBe(false); + { logger: { warn: (m: string) => warnings.push(m) } } as never, + ), 'amount', 'requiredWhen'); + expect(entry.constraint).toMatchObject({ missingKey: 'no_such_field' }); + expect(entry.message).not.toContain("reads 'parent'"); + expect(warnings.some((w) => w.includes('— skipped'))).toBe(false); }); it('leaves the ROW-scoped requiredWhen on the same object working unchanged', () => { @@ -535,15 +567,11 @@ describe('readonlyWhen binds a TOTAL record (#4953)', () => { it('does NOT materialise when the prior row is not in hand (no fabrication)', () => { // `declared-fields.ts`'s standing rule: without the persisted state, // defaulting a declared field to null would FABRICATE a value that - // contradicts the stored row. So this case keeps the historical fault → - // fail-open exit, and the engine avoids it by fetching the prior row - // whenever the object declares a readonlyWhen field (`needsPriorRecord`). - const warnings: string[] = []; - const out = stripReadonlyWhenFields(sparseLockFields, { amount: 999 }, null, { - warn: (m: string) => warnings.push(m), - } as never); - expect(out).toEqual({ amount: 999 }); - expect(warnings.some((w) => w.includes('failed to evaluate — change allowed through'))).toBe(true); + // contradicts the stored row. So this case keeps the historical fault — + // which since ADR-0137 D2 REFUSES the write instead of letting the change + // through — and the engine avoids it by fetching the prior row whenever + // the object declares a readonlyWhen field (`needsPriorRecord`). + fieldRuleRefusal(() => stripReadonlyWhenFields(sparseLockFields, { amount: 999 }, null), 'amount', 'readonlyWhen'); }); it('never mutates the caller\'s prior record (it is the engine\'s hookContext.previous)', () => { @@ -556,31 +584,28 @@ describe('readonlyWhen binds a TOTAL record (#4953)', () => { expect('approved_at' in rows[0]!).toBe(false); }); - it('leaves the fail-open branch ALIVE — an ordering comparison still faults over a total record', () => { + it('leaves the fault branch ALIVE — an ordering comparison still faults over a total record, and refuses', () => { // `null < null` is `no such overload`, so materialising does not make every - // predicate evaluable. This is exactly why the null-guard gate exists. - const warnings: string[] = []; - const out = stripReadonlyWhenFields( + // predicate evaluable. This is exactly why the null-guard gate exists — + // and since ADR-0137 D2 the fault refuses the write rather than writing it. + const entry = fieldRuleRefusal(() => stripReadonlyWhenFields( { fields: { ...sparseLockFields.fields, amount: { type: 'currency', readonlyWhen: 'record.notes < record.approved_at' } } }, { amount: 999 }, sparsePrior(), - { warn: (m: string) => warnings.push(m) } as never, - ); - expect(out).toEqual({ amount: 999 }); - expect(warnings.some((w) => w.includes('failed to evaluate — change allowed through'))).toBe(true); + ), 'amount', 'readonlyWhen'); + expect(entry.constraint).toMatchObject({ hint: 'null-comparison' }); }); - it('keeps fail-OPEN for an UNDECLARED key — materialising covers declared fields only', () => { + it('keeps an UNDECLARED key unevaluable — materialising covers declared fields only — and refuses it', () => { // The #4649 line, unmoved: a typo must stay unevaluable so it is reported, - // not silently read as null. - const warnings: string[] = []; - expect(stripReadonlyWhenFields( + // not silently read as null. What ADR-0137 D2 moved is the report: the + // write is refused, naming the key, instead of the change going through. + const entry = fieldRuleRefusal(() => stripReadonlyWhenFields( { fields: { amount: { type: 'currency', readonlyWhen: 'record.stauts == null' } } }, { amount: 999 }, { id: 'r1', amount: 100 }, - { warn: (m: string) => warnings.push(m) } as never, - )).toEqual({ amount: 999 }); - expect(warnings.some((w) => w.includes('failed to evaluate — change allowed through'))).toBe(true); + ), 'amount', 'readonlyWhen'); + expect(entry.constraint).toMatchObject({ missingKey: 'stauts' }); }); // ── the consequence that moves the OTHER way, pinned rather than discovered ── @@ -639,31 +664,27 @@ describe('readonlyWhen binds a TOTAL record (#4953)', () => { warn: (m: string) => warnings.push(m), } as never)).toEqual({}); expect(warnings.some((w) => w.includes("reads 'parent'") && w.includes('LOCKED'))).toBe(true); - // [#6457 — RE-ANNOTATED, verdict deliberately NOT flipped here.] + // [#6457 — RE-ANNOTATED; the verdict flipped with ADR-0137 D2, not here.] // // A parent that IS bound but does not carry the key is still a fault at - // THIS seam: fail-open, the change goes through. That was the hole #6457 - // closed, and the sentence below is the reason this assertion nonetheless - // stays exactly as PR #6454 wrote it. + // THIS seam — `No such key`, not the unbound-root fault. That was the hole + // #6457 closed from the ENGINE side: its ruling materialises the header + // inside `resolveMasterDetailParent(s)`, using the MASTER object's + // declared-field table — the one thing this pure function does not and + // cannot have. So the strip's own contract is unchanged (its signature + // never grew a second field table), and a caller that hands it a genuinely + // sparse header still meets the fault. // - // #6457's ruling materialises the header INSIDE the engine's - // `resolveMasterDetailParent(s)`, using the MASTER object's declared-field - // table — the one thing this pure function does not and cannot have. So the - // strip's own contract is unchanged (its signature never grew a second field - // table), and a caller that hands it a genuinely sparse header still gets - // the fail-open answer pinned here. What changed is that the ENGINE no - // longer hands it one. - // - // The moved verdict therefore lives where the change lives, and is pinned - // end-to-end against a real driver in `engine-readonly-when-parent.test.ts` - // ("ROW 2 — THE FIX"), with its `requiredWhen` mirror in - // `engine-required-when-parent.test.ts`. Read the two together: this one - // says the strip did not move, that one says the write path did. - const warnings2: string[] = []; - expect(stripReadonlyWhenFields(invoiceLineFields, { quantity: 9999 }, { id: 'l1', invoice: 'inv1' }, { - warn: (m: string) => warnings2.push(m), - } as never, { id: 'inv1' })).toEqual({ quantity: 9999 }); - expect(warnings2.some((w) => w.includes('failed to evaluate — change allowed through'))).toBe(true); + // What the fault DOES moved with ADR-0137 D2: it was fail-open (the change + // went through) and now REFUSES the write. The #6457 verdict on the write + // path is pinned end-to-end in `engine-readonly-when-parent.test.ts` ("ROW + // 2 — THE FIX"), with its `requiredWhen` mirror in + // `engine-required-when-parent.test.ts`. + fieldRuleRefusal( + () => stripReadonlyWhenFields(invoiceLineFields, { quantity: 9999 }, { id: 'l1', invoice: 'inv1' }, undefined, { id: 'inv1' }), + 'quantity', + 'readonlyWhen', + ); }); }); From 326ad2555be345ac4284e0d0235d45ad217693c5 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 24 Sep 2026 19:26:32 +0000 Subject: [PATCH 4/4] test(objectql): hold the caller's limit in the D2 test double; shrink the prose-id baseline the lint rewrite burned down [wip] Claude-Session: https://claude.ai/code/session_019c3Hi6ZMU1p6m6aA6Bz45d Co-authored-by: Claude --- packages/objectql/src/engine-field-predicate-fault.test.ts | 3 ++- scripts/doc-authoring-prose-id.baseline.json | 3 +-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/objectql/src/engine-field-predicate-fault.test.ts b/packages/objectql/src/engine-field-predicate-fault.test.ts index c478b8aff25..3d950d8be36 100644 --- a/packages/objectql/src/engine-field-predicate-fault.test.ts +++ b/packages/objectql/src/engine-field-predicate-fault.test.ts @@ -57,7 +57,8 @@ function makeDriver() { name: 'memory', version: '0.0.0', supports: {}, async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; }, async find(object: string, ast: any) { - return Array.from(storeFor(object).values()).filter((r) => matches(r, ast?.where)); + const rows = Array.from(storeFor(object).values()).filter((r) => matches(r, ast?.where)); + return typeof ast?.limit === 'number' ? rows.slice(0, ast.limit) : rows; }, async findOne(object: string, ast: any) { for (const r of storeFor(object).values()) if (matches(r, ast?.where)) return r; diff --git a/scripts/doc-authoring-prose-id.baseline.json b/scripts/doc-authoring-prose-id.baseline.json index cb8f53e6a44..e4df982353e 100644 --- a/scripts/doc-authoring-prose-id.baseline.json +++ b/scripts/doc-authoring-prose-id.baseline.json @@ -258,8 +258,7 @@ }, "packages/lint/src/validate-expressions.ts": { "#4343": 1, - "#4889": 2, - "#4977": 1, + "#4889": 1, "#6010": 2, "#6146": 1 },