fix(objectql)!: a system write's readonly value is judged for its shape — a seed's malformed readonly datetime is refused, never stored (#21663)

.changeset/21663-readonly-value-shape-refused.md

@@ -0,0 +1,51 @@
+---
+'@objectstack/objectql': minor
+---
+
+fix(objectql)!: a system write's readonly value is judged for its shape — a seed's `'yesterday'` on a readonly datetime is refused with the sentence any other field gets, never stored (#21663)
+
+**BREAKING** — a write that keeps a readonly value now has that value's SHAPE
+checked. The static readonly strip still exempts a system write (seed replay,
+migration, `isSystem` plugin code, a `before*` hook's stamp) and still drops a
+non-system caller's readonly value; what changed is that the value the
+exemption keeps is no longer stored unjudged. Before, the record validator
+skipped every readonly field, so under `isSystem` a malformed readonly value
+reached the driver verbatim — a seed's `run_at: 'yesterday'` on a readonly
+`datetime`, an unresolved `cel` envelope from a seeder that skips its
+resolution, an authored `created_at` the seed now keeps — while the same value
+on a non-readonly field was refused.
+
+Now it is refused the same way: `VALIDATION_FAILED` (400 at an HTTP boundary),
+the same field code and the same sentence a non-readonly field gets
+(`Run At must be a valid datetime (ISO-8601)`), and a seed counts the row as a
+seed error. This holds on insert, on the dry run (`ObjectQL.validate`), and on
+both update paths, where the readonly values left after the strip are judged.
+
+Which checks a readonly value reaches — its type's shape, never a constraint:
+
+- refused: a `date` / `datetime` / `time` the platform does not read, a
+  non-number on a number-typed field, a non-boolean on a boolean, a non-array on
+  a multi-value field, a filter-operator object, and an ADR-0104 reference /
+  media / structured-JSON shape under the object's own posture (warn-first, as
+  on any other field, until the deployment's evidence enforces it);
+- NOT checked, exactly as before: option membership, `maxLength` /
+  `minLength`, `valueDomain`, `min` / `max` / `scale` / `precision`, the email /
+  url / phone formats, and `required`. Option membership stays out on purpose:
+  `sys_activity.type` is a readonly `select` whose options are the built-in set
+  of an open vocabulary, and an author-contributed value there is stored.
+
+A numeric string on a readonly number field is now written as its number, and a
+lone scalar on a readonly multi-value field as a one-member list, as on any
+other field — the door reads the value the same way it judges it.
+
+**What moves for consumers.** A seed, migration or `isSystem` write that puts a
+malformed value in a readonly field — or a hook that stamps one — is refused
+where it was stored. Fix the value at its producer: write an ISO-8601 instant
+(or a `Date`) into a readonly `datetime`, resolve a `cel` value before the
+write, and stamp numbers and booleans as such. Rows already stored are never
+re-read or rewritten. `validateRecord`, as exported, is unchanged: the readonly
+scope is the engine write path's own.
+
+Clause-②: no (narrowing)
+
+<!-- adr-0087: not-required (no-migration-prescription) a write-time refusal of a malformed value in a readonly field, judged by the same per-type shape checks a non-readonly field already gets. No authorable key, spelling, export or stored shape moves: the field schema is unchanged, the published validateRecord signature is unchanged, no stored row is read or rewritten, and which value a producer meant to write is not something a ledger entry can rewrite. The other categories are closed on facts: the package publishes (not unpublished); no ADR-0087 id covers this behaviour (not already-registered); and the change is a write-path verdict, not a declaration (not runtime-interface-only or type-surface-only). -->

packages/objectql/src/engine-insert-static-readonly-strip.test.ts

@@ -382,8 +382,12 @@ describe('#14147 — strictReadonlyWrites refuses before any driver dispatch', (
   });
 
   it('strict adds NO second policy — an isSystem write it would not strip is still accepted', async () => {
+    // A well-formed value: since #21663 a system writer's readonly value is
+    // judged for its SHAPE (a placeholder like `'x'` in a datetime is refused
+    // as `invalid_date`), which is a different policy from the one this case
+    // is about — `strictReadonlyWrites` adding nothing to the strip.
     const o = await observeInsert(
-      { title: 'T', completed_at: 'x' },
+      { title: 'T', completed_at: '2019-04-01T00:00:00Z' },
       { strictReadonlyWrites: true, context: { isSystem: true } },
     );
     expect(o.refusedCode).toBeNull();

packages/objectql/src/engine.ts

@@ -262,7 +262,7 @@ import { deriveViewContainerObject } from '@objectstack/metadata/view-container'
 // registrar and `os validate` both call.
 import { viewContainerNameRefusal } from './view-container-name-refusal.js';
 import { bindHooksToEngine } from './hook-binder.js';
-import { validateRecord, normalizeMultiValueFields, normalizeBlankTypedValues, normalizeNumericStringValues, coerceBooleanFields, ValidationError, buildFieldError, resolveFieldLabel, valueShapePostureSetByEnv, mediaPostureSetByEnv, isScannableValueShapeField, valueShapeStrictEffective, mediaStrictEffective } from './validation/record-validator.js';
+import { validateRecord, validateRecordInScope, normalizeMultiValueFields, normalizeBlankTypedValues, normalizeNumericStringValues, coerceBooleanFields, ValidationError, buildFieldError, resolveFieldLabel, valueShapePostureSetByEnv, mediaPostureSetByEnv, isScannableValueShapeField, valueShapeStrictEffective, mediaStrictEffective } from './validation/record-validator.js';
 import type { AdmittedValueShapeViolation, AdmittedValueShapeViolationSink } from './validation/record-validator.js';
 import type { RelatedFieldBinding, RelatedRecordBinding } from './validation/rule-validator.js';
 import { collectPredicateRelationships, evaluateValidationRules, optionVisibilityReadsPermissions, readsPermissionPredicate, referentialClearBinding, needsPriorRecord, stripReadonlyWhenFields, stripReadonlyWhenFieldsMulti, hasReadonlyWhenInPayload, hasParentScopedReadonlyWhenInPayload, hasParentScopedRequiredWhen, stripReadonlyFields, stripRuntimeOwnedFields, staticReadonlyInsertSubject, preserveAuditIgnoredOnInsertWarning } from './validation/rule-validator.js';
@@ -6063,9 +6063,10 @@ export class ObjectQL implements IObjectQLEngine {
         // so the same declaration behaved differently per datasource. That
         // split surfaced two ways: a validation-visible field was REJECTED by
         // the engine's own write validator ("must be a valid datetime"), and a
-        // `readonly`/`system` field — which `validateRecord` skips, i.e. the
-        // ~100 `created_at`/`updated_at` platform declarations — silently
-        // stored the four characters `NOW()`.
+        // `readonly`/`system` field — which `validateRecord` then skipped, i.e.
+        // the ~100 `created_at`/`updated_at` platform declarations — silently
+        // stored the four characters `NOW()`. (Since #21663 a readonly value's
+        // shape is judged too, so that literal would now be refused.)
         //
         // Resolved from the caller's `nowSnapshot`, so every defaulted field
         // in one insert (and every row of one batch) carries the SAME instant.
@@ -9884,12 +9885,19 @@ export class ObjectQL implements IObjectQLEngine {
    * — and not raw type membership, because the registry INJECTS covered-type
    * fields into every object it registers: `organization_id` and `owner_id`
    * (both `system`), plus `created_by` / `updated_by` (both in `SKIP_FIELDS`),
-   * are all `lookup`s. `validateRecord` skips every one of them before it ever
-   * reaches the value-shape check, so counting them made this answer `true` for
-   * literally every object — the dormancy rule above never fired, and this
-   * cache memoized a constant. Same predicate as the scanner for the same
-   * reason the scanner imports it: three readings of "a covered field" drifting
-   * by one clause is how a gate ends up governing fields nothing enforces.
+   * are all `lookup`s. A caller never writes any of them, so counting them made
+   * this answer `true` for literally every object — the dormancy rule above
+   * never fired, and this cache memoized a constant. Same predicate as the
+   * scanner for the same reason the scanner imports it: three readings of "a
+   * covered field" drifting by one clause is how a gate ends up governing
+   * fields nothing enforces.
+   *
+   * [#21663] The three that are `readonly` (`organization_id`, `created_by`,
+   * `updated_by`) DO reach the value-shape check now, on the value a system
+   * writer, hook or stamp stores. They still do not count here, so an object
+   * whose only covered fields are those stays warn-first for them: a malformed
+   * value is admitted, logged and reported, never stored silently. See
+   * `isScannableValueShapeField` for why widening this test is not the fix.
    */
   private objectHasCoveredValueField(objectSchema: any): boolean {
     if (!objectSchema?.fields) return false;
@@ -12469,9 +12477,11 @@ export class ObjectQL implements IObjectQLEngine {
    * call that fires side-effecting hooks (mail, outbound calls, writes to
    * other objects) is the #4052 defect in a new spelling, where a preview
    * quietly executes. So the gap is documented rather than closed: audit and
-   * ownership stamps are `system`/`readonly` and are skipped by validation
-   * anyway, so what remains is the narrow case of a hook deriving a
-   * *business* field that its object also validates.
+   * ownership stamps are `system`/`readonly`, so validation never requires
+   * them, and (#21663) the only thing it asks of a readonly value is its
+   * shape, which a platform stamp always has — so what remains is the narrow
+   * case of a hook deriving a *business* field that its object also
+   * validates.
    *
    * Nothing is written, no sequence is consumed, and no driver is touched —
    * validation is in-process, which is what makes row-by-row dry run of a
@@ -12724,7 +12734,11 @@ export class ObjectQL implements IObjectQLEngine {
         });
       };
       try {
-        validateRecord(schemaForValidation, row, mode, {
+        // [#21663] `'include'` in both modes: the caller-write strips ran
+        // above, so this is the payload the write stores — and the write
+        // judges its readonly values' shape (insert in the same call, update
+        // in a second pass after its own strip).
+        validateRecordInScope(schemaForValidation, row, mode, 'include', {
           mediaValueShapeStrict, valueShapeStrict, messages, onAdmittedValueShapeViolation,
         });
         evaluateValidationRules(schemaForValidation as any, row, mode, {
@@ -13515,8 +13529,13 @@ export class ObjectQL implements IObjectQLEngine {
         for (let i = 0; i < rows.length; i++) {
           if (rowErrors[i] !== undefined) continue;
           try {
-            normalizeMultiValueFields(schemaForValidation, rows[i]);
-            validateRecord(schemaForValidation, rows[i], 'insert', { mediaValueShapeStrict, valueShapeStrict, messages: msgCtx, onAdmittedValueShapeViolation });
+            // [#21663] `'include'`: the readonly strip ran above, so every
+            // readonly value still on the row is one the driver will store —
+            // a system writer's (seed, migration), a hook's or a stamp — and
+            // its SHAPE is judged here like any other field's. See
+            // `ReadonlyValueScope` (record-validator.ts).
+            normalizeMultiValueFields(schemaForValidation, rows[i], 'include');
+            validateRecordInScope(schemaForValidation, rows[i], 'insert', 'include', { mediaValueShapeStrict, valueShapeStrict, messages: msgCtx, onAdmittedValueShapeViolation });
             evaluateValidationRules(schemaForValidation as any, rows[i], 'insert', { logger: this.logger, currentUser: this.buildEvalUser(opCtx.context), skipStateMachine: shouldSkipStateMachine(opCtx.context), messages: msgCtx, parent: insertParentForRow?.(rows[i]), related: insertRelatedForRow(rows[i]), permissions: insertPermissionsFor(rows[i]) });
             await this.assertReferencesResolve(
               schemaForValidation, rows[i], suppliedPerRow[i], opCtx.context, msgCtx,
@@ -14866,7 +14885,13 @@ export class ObjectQL implements IObjectQLEngine {
                // secret channel (which carries the secret-arm refusal).
                this.refuseEmptyPasswordFields(object, hookContext.input.data as Record<string, unknown>);
                await this.encryptSecretFields(object, hookContext.input.data as Record<string, unknown>, opCtx.context, hookContext.input.options);
-               normalizeMultiValueFields(updateSchema, hookContext.input.data as Record<string, unknown>);
+               // [#21663] Scope `'skip'` — the public `validateRecord` IS that
+               // scope: the readonly strip has NOT run yet, so a readonly value
+               // here may be a caller's the strip is about to drop — judged, a
+               // whole-record write-back echoing a legacy stored value would
+               // become a refusal. Readonly values are judged after the strip
+               // (`validateRecordInScope(…, 'only')`, below).
+               normalizeMultiValueFields(updateSchema, hookContext.input.data as Record<string, unknown>, 'skip');
                validateRecord(updateSchema, hookContext.input.data as Record<string, unknown>, 'update', { mediaValueShapeStrict, valueShapeStrict, messages: updateMsgCtx, onAdmittedValueShapeViolation });
                // [#5284] Demand-driven, and the demand is asked PER OBJECT.
                //
@@ -15051,6 +15076,15 @@ export class ObjectQL implements IObjectQLEngine {
                // "you sent a read-only field" should not depend on whether some
                // other field also failed a business rule.
                assertNoStrictDrops();
+               // [#21663] The payload is FINAL here (see the seam below), so
+               // every readonly value on it is one the driver will store: a
+               // system writer's (the strip above never ran for it), a hook's,
+               // or a stamp. Its SHAPE is judged now, by the same arms and
+               // sentences as the caller-writable fields the first
+               // `validateRecord` above judged ahead of the strip — `'only'`,
+               // because those are already judged. See `ReadonlyValueScope`.
+               normalizeMultiValueFields(updateSchema, hookContext.input.data as Record<string, unknown>, 'only');
+               validateRecordInScope(updateSchema, hookContext.input.data as Record<string, unknown>, 'update', 'only', { mediaValueShapeStrict, valueShapeStrict, messages: updateMsgCtx, onAdmittedValueShapeViolation });
                // ── [#19989] The post-image seam on the BY-ID path ─────────────
                //
                // The by-id twin of the predicate-path call below, placed at the
@@ -15186,7 +15220,13 @@ export class ObjectQL implements IObjectQLEngine {
                // secret channel (which carries the secret-arm refusal).
                this.refuseEmptyPasswordFields(object, hookContext.input.data as Record<string, unknown>);
                await this.encryptSecretFields(object, hookContext.input.data as Record<string, unknown>, opCtx.context, hookContext.input.options);
-               normalizeMultiValueFields(updateSchema, hookContext.input.data as Record<string, unknown>);
+               // [#21663] Scope `'skip'` — the public `validateRecord` IS that
+               // scope: the readonly strip has NOT run yet, so a readonly value
+               // here may be a caller's the strip is about to drop — judged, a
+               // whole-record write-back echoing a legacy stored value would
+               // become a refusal. Readonly values are judged after the strip
+               // (`validateRecordInScope(…, 'only')`, below).
+               normalizeMultiValueFields(updateSchema, hookContext.input.data as Record<string, unknown>, 'skip');
                validateRecord(updateSchema, hookContext.input.data as Record<string, unknown>, 'update', { mediaValueShapeStrict, valueShapeStrict, messages: updateMsgCtx, onAdmittedValueShapeViolation });
                // [#2982] The middleware-composed AST — asserted present and
                // bound to the memoized row read in the pre-phase above, so the
@@ -15305,6 +15345,12 @@ export class ObjectQL implements IObjectQLEngine {
                // caller is told before N rows are written with a column missing
                // — the failure mode a bulk write makes N times larger.
                assertNoStrictDrops();
+               // [#21663] The predicate-path twin of the by-id second pass, at the
+               // same point and for the same reason: the readonly values left on
+               // the final payload are stored, so their SHAPE is judged — before
+               // N rows are written.
+               normalizeMultiValueFields(updateSchema, hookContext.input.data as Record<string, unknown>, 'only');
+               validateRecordInScope(updateSchema, hookContext.input.data as Record<string, unknown>, 'update', 'only', { mediaValueShapeStrict, valueShapeStrict, messages: updateMsgCtx, onAdmittedValueShapeViolation });
                // ── [#19950] The post-image seam on the PREDICATE path ─────────
                //
                // An enforcement layer's write `check` must hold for EVERY row a