From d797baf6fd3d4b96945c20610553b048b888fb62 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 08:51:09 +0000 Subject: [PATCH 1/5] wip(#13889): advisory aggregation scope + by-row advisory gating Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01ARYe3yQTQCUFm5qPYNgKaJ --- packages/core/src/index.ts | 9 ++ .../core/src/utils/advisory-aggregation.ts | Bin 0 -> 6853 bytes packages/metadata-protocol/src/seed-loader.ts | 63 ++++++++- .../objectql/src/validation/rule-validator.ts | 123 +++++++++++++++++- 4 files changed, 192 insertions(+), 3 deletions(-) create mode 100644 packages/core/src/utils/advisory-aggregation.ts diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 7e9d3eb3f0..e8b681f4e1 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -65,6 +65,15 @@ export * from './utils/internal-write-response.js'; // that `@objectstack/metadata-protocol`'s atomic `batchData` also uses. export * from './utils/migration-journal.js'; +// Export the advisory-hit aggregation scope (#13889) — the seam between the +// evaluator that PRODUCES an advisory hit (`@objectstack/objectql`) and the +// machine load path that REPORTS a run's worth of them as one summary +// (`@objectstack/metadata-protocol`'s seed loader). It lives here, on the floor +// both of those already stand on, because neither can import the other's +// package for it: objectql depends on metadata-protocol, so the dependency only +// runs one way. +export * from './utils/advisory-aggregation.js'; + // Export the runtime filter-placeholder resolver (framework#3582) export * from './utils/filter-tokens.js'; diff --git a/packages/core/src/utils/advisory-aggregation.ts b/packages/core/src/utils/advisory-aggregation.ts new file mode 100644 index 0000000000000000000000000000000000000000..8e7a04f7778d608b9abff1662c1c0f63d5f7d926 GIT binary patch literal 6853 zcmai2-)|h%73SH0#UVj>P1kEDs1-OrU;#rcId)_lqAH3qcW3VIPCYw!J9pNb#j=!2 zJhT;vJ|Lm0Z%-cl$35B2Q(JnQ$*jz5CH2@!e`?aPUBBMjlp?(Ct*mUk z_S%ZPCjIOr_pVy3kMmm0XjIvesdHD#&{ZIQ{NkG&X~L!#}9DKotHGO^y9ksjom+6D)V-1+cs ze@}Myq%ZI9AKaDBm3}RSHNK`e2j?nVm>Th_Z0f2^{MgNkOa@lGE#Q;QuB`RcM++-w zrgqh;$H8KO6H`~!l8#MT+Jf>JG|GhiN#RB#TlMI2($%L0V>v}qI+dHtf4`KI+(q4R zx((@m3AZFKYg?JL&QEM`7Qr<&CELe%O!Y)TPw+XD>@9rQkb!fxuPZavq|qkkjWNW@ z($!mtT?+rpb;R2YjJGn(D_`rupa>ce?S)EYTwrTGuH0-ira0AQCKeyXaCF5xT5P6M znU9hCU`t^~d7V!XTiBYqvNn0?WsmZ_`l8aRe5$cA5r?ArA6evdPm=rngS`ZxEv-jD zRQKUg|KRAud$PO#fqeJm3%G-zPXsvCj8=9%MRLUuBn*aRxfzwt*Lm87d3icU-c05p z_1LOJ{c33P!e%|W3uw$Lo6)$;%}d16nu&DAu1>4T)oa%`UtinOV+w(}SwKG?7xKfuKKt>D zZ=Zer$bf13ycRc#>%fCJS&zDdC{n!8f@t+jHq-9PWW96V|)e&ze zrbLo_R|{%nDO+Q#h#Q-!6*tt#qq2H(=b+JJ`Ore{dut<_XAx z73MN!vhJ!`N!kl^g^U8MBeBIY6JXK%~zT2{9rBg$G)IFLdV! zWk0Rpe@aeC8t*=8$@Bnzdyr4^w$m2j*&;T=&%Z6eyQI(POi&2KtE@w?$u3ymR)@Aa zLDT552V4>bYYbt$4l`<(I+a;zaJ;3tB-X(~vxaaZLbe={1(m*)nnE#AIuxg-DA+k9 z*?A9QxAo&5c*9op#%u~z7B)nyn{*;+j4|sD+O*rC5(=}hmC72}Vo6~LBe{so3afyQ z)KnGIUpj~&pEEa+(#iXfoAHqmpO?q9Es`Nhi;ZFeE|qznS!Vb-yA$>aEfpV`YC0Pf zxu=z_4X6gnWaBXBsEQvTt7T96$ozAdIJS050&Or4V)Rh2PwKg&G%!q{SbEo%d(!ud zGTn6u@*%ny+GTP9V4;@Q3-Es-!@>ZupqzYwMno3hio}jwX<*4Os5hnAb(SiuWq(_D z)ri4F3Z#?xg@1$xrp!Oq4xi9qL6#oQ3T>X+!F1FB#~9h&`DhyoE6@-q0pXZc zBuF%qc@A0S2IHj0!gZ9zwQKGp#}I2p0R)`J;+Rzbdg2 z5fXS!l2JGZcaTv(n@p&dH0DwBYLTA}UCK2r>dpC~jcuxn&<{MmRzi)s0yURyB+do_XID z834!-1Ni9S-7sB=Nz>V67UmHSDd^J>Oxb;OSDhb*;jG4mQ&-XnT+&sBT}xzyF=5Ky zOJ1dT3p(d?piY6mDel_lrflM0I|vAZCv`IkXg2^?0K)rw+d(93L?3Jq>ZIoBlyYFu z8tS59d{ixbC_!V077ReSGh+-K6?FBd#`>7!W>5_;<475P;ShxWULpyMDx8GS)3?zy zkK0N0(P97Yz1{6c2mALA&uAM4waFBm7bnXcrqG5kgv2bOdp`T-t7l*T?z<;nu{(}U zF>LxA3`Opin4?V3Mo7;$R#x(f&Y!1psj%fthLxMhYUwh&r6_tdcJA1(-q6WkwlAAz zV)h~-YTg*G2Jk6(Zt9SG0c+~8!36ebiG3UfVTW<)h2fA}^2?}sABFJzum^A*E{b;M$_ zX#qOJv^Chw<6z9*X}OzVMKXnrw#TntDAvvKb)Jz|z#PnVXv{%)e9)wq+rwbXGIL8fa2xue;OE)mAm2jVwP-%!0-;RhhcUhWCA6& zDUhWaXsKi~!cPOPU7!gh%4ti=*#!Nz!RCiQOEgQCfmTpV6y}wB4q|Q~H(>t3!y7Be z&gJmF>wumVV}TZv1_Fofn(57Ou-g`#CgQe=L03k>`!GzP#!EZD%tbewN^Mo#?yN;J z2uVG*m>iOq5=<*4dETKDVWvZe!mgo%E0-S)9m`TZ9L`GY#y}+w?+;@iYP<~nsQC^1 zTJS+#t1Je9Qj%9rgXpuDUOA=cvqW8r*CPAutw{bkt|E@ZJiu*&ra0KS zyp)K&ztF`v%dt`Sq`wQ&i6=o}nh^gQ`g3d6R9aU)CBy9Z6%mVZ@u-JZc2RxkL zg~C)5g|PR8ju^?K#YSkn4gSz6I!NMHi)1_iHn9HlK19J00F6B+*21j@+&$b#aA>xK zu1G$V&dcp*m}CQ}G{k%#}c zvrP}o_2P_b_09yvm*pNaKz5^M`nw9p+pS0?W7bhI565BWFSsSZ+BQbzTzSo)bllf@?@wmy-}~ zt~;Cx1{N8^9W1UNS^mwAvb?m!A~=L5?gi`67OoF;Rp$B~gRV_tf=f=`_rYy)CbmA_ zTp=@x2CjVEo#Uv7vv|YJG~7lZ>%mH0aOge*#=b+-xH98&y@uzu=H?t|^hm*BoMr^f z0T{S=OCgsx%|Rqgp}@gx6gnJxyf$I9#95l+>K<1B%ew{6T3i{!l`Jv!9J_~VXpw|u z+()Jxl5b(<%9}^GJ1EIvxh0)7xpiCaVI{)#C!VuZgKc;hXm;5abSQc7P>-BA%*QK0 z { + return runWithAdvisoryAggregation( + () => this.loadDatasets(request), + (groups) => this.reportAdvisorySummary(groups), + ); + } + + /** + * One summary line per advisory rule tripped during a load. + * + * Shape, from the ruling: 「N 行触发 advisory 规则 X,详见…」 — the count is + * of ROWS, the rule is named, and the 「详见…」 half is the structured meta: + * the rule's own message plus example row ids, which is what turns a summary + * back into something actionable. + * + * Emitted at `warn`, the level the per-row lines used: the defect is N lines + * where one belongs, NOT that the report exists. Dropping it to `info` would + * hide it under the default log level entirely — deleting the report rather + * than aggregating it, which is option C the ruling excluded. + */ + private reportAdvisorySummary(groups: AdvisoryGroup[]): void { + for (const group of groups) { + const examples = group.sampleRecordIds.length > 0 + ? ` Example row(s): ${group.sampleRecordIds.join(', ')}${group.rows > group.sampleRecordIds.length ? ', …' : ''}.` + : ''; + this.logger.warn( + `[SeedLoader] ${group.rows} row(s) triggered advisory rule '${group.rule}' (${group.severity}) on '${group.object}': ` + + `${group.message} — advisory rules are authoring guidance for interactive writes; they never block a seed load ` + + `and nothing here failed.${examples}`, + { + object: group.object, + rule: group.rule, + severity: group.severity, + rows: group.rows, + message: group.message, + sampleRecordIds: group.sampleRecordIds, + }, + ); + } + } + + private async loadDatasets(request: SeedLoaderRequestParsed): Promise { const startTime = Date.now(); // Pin the environment `Seed.env` is gated on BEFORE anything reads config. // Resolving it here — the one funnel every seeding path goes through — is diff --git a/packages/objectql/src/validation/rule-validator.ts b/packages/objectql/src/validation/rule-validator.ts index 0cc67e6f91..e789f3d24c 100644 --- a/packages/objectql/src/validation/rule-validator.ts +++ b/packages/objectql/src/validation/rule-validator.ts @@ -194,7 +194,8 @@ import { ExpressionEngine, collectCelRootIdentifiers } from '@objectstack/formula'; import type { Expression } from '@objectstack/spec'; -import { AUDIT_PROVENANCE_FIELDS, RUNTIME_OWNED_FIELD_TYPES } from '@objectstack/spec/data'; +import { AUDIT_PROVENANCE_FIELDS, RUNTIME_OWNED_FIELD_TYPES, resolveInjectedSystemColumns } from '@objectstack/spec/data'; +import { recordAdvisoryHit } from '@objectstack/core'; // [#8215] The canonical spelling of the primary-key column — the sanctioned use // of this registry ("what is the canonical spelling of the column that plays // role X"). The role is structural, not per-object: the driver provisions `id` @@ -1962,6 +1963,80 @@ function evaluateOptionVisibility( } } +/** + * The object's name, for an advisory group's key. Read off the schema rather + * than taken as a parameter: every caller already passes the registry's own + * object definition, and widening this module's signature for a diagnostic + * would put a reporting concern on a published surface. + */ +function advisoryObjectName(objectSchema: unknown): string { + const name = (objectSchema as { name?: unknown } | null | undefined)?.name; + return typeof name === 'string' && name.length > 0 ? name : '(unknown)'; +} + +/** One example row id for an advisory group, when the write carries one. */ +function advisoryRecordId( + merged: Record, + data: Record, +): string | undefined { + const id = merged.id ?? data.id; + return typeof id === 'string' || typeof id === 'number' ? String(id) : undefined; +} + +/** + * [#13889] Whether this UPDATE writes nothing but platform-injected columns — + * i.e. no business field moved, so the row's advisory rules have nothing new to + * say about it. + * + * The membership question is NOT answered by a list kept here. It is answered + * by `resolveInjectedSystemColumns` (`@objectstack/spec/data`), the declared + * per-object authority for *"which columns does the platform provision on THIS + * object without the author declaring them?"* — the same derivation + * `applySystemFields` injects from. That matters for correctness, not just for + * tidiness: the answer is per-object (`ownership: 'org'` carries no `owner_id`; + * `systemFields: false` carries nothing at all), so a hand-kept literal would be + * wrong on exactly the objects that differ, and would drift the moment a tier + * like ADR-0117's `owning_business_unit_id` is added. + * + * INSERT is deliberately excluded: a row being born is precisely when its + * advisory rules SHOULD speak, and that is the one ring 「按行不按写入」 + * preserves. + * + * ### What this does not see — stated, because a skip nobody expects is worse + * than a warning nobody wanted + * + * - **Soft delete.** `is_deleted` / `deleted_at` are not injected columns in + * this plan, so a soft-delete write still re-evaluates advisories. Left that + * way on purpose: retiring a row is a business event, not a stamp. + * - **A business field rewritten to the value it already held.** The payload + * names a business column, so this returns false and the advisory reports + * as it does today. Detecting "written but unchanged" would need a + * value-equality pass over `previous`, which is absent on the bulk-update + * seam (`previous: null`) and would change what an ordinary interactive + * re-PUT reports — a behaviour change the ruling did not ask for. + * - **An author who declares their own field named `owner_id`** on an + * ownership-eligible object. The plan reports the column either way (the + * registry lets the author's definition win, but the name is addressable + * regardless), so a write moving only that field skips its advisories. The + * row's advisories already rang when its content was written, so this + * under-reports by at most the second ring — the defect's own direction, + * not a new one. + */ +function writesOnlyInjectedSystemColumns( + objectSchema: unknown, + data: Record, + mode: Mode, +): boolean { + if (mode !== 'update') return false; + const keys = Object.keys(data); + // An empty payload is not a system write-back — it is a write that names + // nothing. Fail toward today's behaviour rather than widening the skip to a + // shape this was not reasoned about. + if (keys.length === 0) return false; + const injected = resolveInjectedSystemColumns(objectSchema).names; + return keys.every((key) => injected.has(key)); +} + /** * Evaluate an object's declared validation rules against an incoming write. * @@ -2082,9 +2157,38 @@ export function evaluateValidationRules( // not a security boundary. evaluateOptionVisibility(fields, data, merged, previous, opts.currentUser, errors, opts.logger, opts.messages); + // [#13889] Does this write move any BUSINESS field? A system write-back that + // touches only platform-injected columns does not, so re-running the object's + // ADVISORY rules over it would report the same row a second time for a change + // its author never made. Maintainer ruling (2026-09-01): + // 「同一行清库首启只响一次,按行不按写入」 — one row rings once per clean + // first boot, counted BY ROW, not by write. + // + // The worked case is `claimSeedOwnership` (`@objectstack/plugin-security`), + // the first-admin handoff that re-owns seeded rows: + // + // ql.update(name, { owner_id: adminUserId }, + // { where: predicate, multi: true, context: { isSystem: true } }) + // + // Seeding inserts the row (advisory rings — correctly, once), then the claim + // scan rewrites `owner_id` and every rule ran again: the same row rang twice + // on a clean first boot, and anyone counting startup warnings over-estimated + // by the number of claimed objects. + // + // ERROR-severity rules are deliberately NOT gated by this. An invariant is an + // invariant on every write, whoever issued it and however little it moved; + // only the ADVISORY half is a report about an authoring decision, and a + // report about a decision nobody made is the defect. + const advisoryAlreadyReported = writesOnlyInjectedSystemColumns(objectSchema, data, mode); + const ordered = (hasRules ? rules! : []) .filter((r): r is BaseRule => r != null && typeof r === 'object') .filter((r) => r.active !== false) + // [#13889] `按行不按写入` — see `advisoryAlreadyReported` above. The rule is + // dropped from the run rather than having its OUTPUT suppressed: the ruling + // asks for no re-EVALUATION, and a suppressed log would leave the wasted + // predicate evaluation in place on every system write-back. + .filter((r) => !(advisoryAlreadyReported && (r.severity ?? 'error') !== 'error')) // Seed writes (#3433) skip `state_machine` entirely: curated seed data is a // snapshot of established facts, not a record flowing through its lifecycle, // so neither the `initialStates` entry-point (insert) nor the transition @@ -2111,7 +2215,22 @@ export function evaluateValidationRules( const severity = rule.severity ?? 'error'; if (severity === 'error') { errors.push(violation); - } else { + } else if ( + // [#13889] On a machine load path (seed / bootstrap) the advisory is + // FOLDED into that run's one summary line instead of being logged per + // row — 「⛔ 不改规则语义,只改日志形状」: the rule evaluated exactly as + // it always did, and the hit is still reported, once per rule with its + // row count and example rows. Off that path no scope is active, + // `recordAdvisoryHit` returns false, and the per-write line below is + // emitted verbatim — an ordinary interactive write is untouched. + !recordAdvisoryHit({ + object: advisoryObjectName(objectSchema), + rule: String(rule.name ?? '(unnamed)'), + severity, + message: violation.message, + recordId: advisoryRecordId(merged, data), + }) + ) { opts.logger?.warn?.( `Validation rule '${rule.name}' (${severity}): ${violation.message}`, ); From 29e42ecb313c62a53acd9ed732366aec1a1cdcfe Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 09:00:33 +0000 Subject: [PATCH 2/5] feat(#13889): aggregate boot-path advisory hits; count by row, not by write Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01ARYe3yQTQCUFm5qPYNgKaJ --- .../core/src/utils/advisory-aggregation.ts | Bin 6853 -> 7495 bytes packages/metadata-protocol/src/seed-loader.ts | 6 +- .../src/validation/advisory-boot-path.test.ts | 344 ++++++++++++++++++ .../objectql/src/validation/rule-validator.ts | 21 +- 4 files changed, 364 insertions(+), 7 deletions(-) create mode 100644 packages/objectql/src/validation/advisory-boot-path.test.ts diff --git a/packages/core/src/utils/advisory-aggregation.ts b/packages/core/src/utils/advisory-aggregation.ts index 8e7a04f7778d608b9abff1662c1c0f63d5f7d926..c743c50af2e51211273de347ab96675a0b118509 100644 GIT binary patch delta 959 zcmY*Y!D2Fz?*}k{a!yEF>%wfu<8sgl)`}_56Zck zD^p{bpaBJ^bJfusFn7yK7;;$b(N)}maNaADO`dL(0OoP3^cN8Q}9Fiq<=eCMcx6_OpqHf z#=U1Oy~gKxX9aq3`jOx91q9KEamZ#hiGnF2!tty)4^TW<9v}q+Xf0W{)5P$IJzA7x ziyjW=Uv7U|y0@_JGHAzpCa5&_cp09oU2yDecx|b!;1nm^q#Y0Pw?~Hune$eeks3~N zpkdNh31z`@j4a2qIaX7kR%2cXyL&>)u-XRIf!bIY=59Z z#CO2f8jpA8$LqhBZ*T&$y^RO)ugf={Zc%VqRl9P#R6bi|G@EaHyTeyqxPCm{+=>n6 I7n|A5zouG9tpET3 delta 306 zcmX?Zb<}jjc5aT;wV0xgUZnJL9k z@#4hXf}GSKsO;nog44|v+>7!{3ly?bEA+AJxd#|i-ZNoaxq diff --git a/packages/metadata-protocol/src/seed-loader.ts b/packages/metadata-protocol/src/seed-loader.ts index 2f1076bf3d..c08c1f0977 100644 --- a/packages/metadata-protocol/src/seed-loader.ts +++ b/packages/metadata-protocol/src/seed-loader.ts @@ -437,8 +437,8 @@ export class SeedLoaderService implements ISeedLoaderService { */ private reportAdvisorySummary(groups: AdvisoryGroup[]): void { for (const group of groups) { - const examples = group.sampleRecordIds.length > 0 - ? ` Example row(s): ${group.sampleRecordIds.join(', ')}${group.rows > group.sampleRecordIds.length ? ', …' : ''}.` + const examples = group.sampleRows.length > 0 + ? ` Example row(s): ${group.sampleRows.join(', ')}${group.rows > group.sampleRows.length ? ', …' : ''}.` : ''; this.logger.warn( `[SeedLoader] ${group.rows} row(s) triggered advisory rule '${group.rule}' (${group.severity}) on '${group.object}': ` + @@ -450,7 +450,7 @@ export class SeedLoaderService implements ISeedLoaderService { severity: group.severity, rows: group.rows, message: group.message, - sampleRecordIds: group.sampleRecordIds, + sampleRows: group.sampleRows, }, ); } diff --git a/packages/objectql/src/validation/advisory-boot-path.test.ts b/packages/objectql/src/validation/advisory-boot-path.test.ts new file mode 100644 index 0000000000..9916085c5b --- /dev/null +++ b/packages/objectql/src/validation/advisory-boot-path.test.ts @@ -0,0 +1,344 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#13889] Advisory rules on the seed / bootstrap load path — the BEHAVIOURAL + * pins for maintainer ruling B (2026-09-01, verbatim 「同意」). + * + * The acceptance anchor is hotcrm#1203's measured record, and it is behavioural, + * so these pins are too: a REAL `ObjectQL` engine, a REAL object declaring a + * REAL `severity: 'warning'` rule, the REAL `SeedLoaderService`, and the REAL + * write shape `claimSeedOwnership` issues. Nothing below asserts against a + * hand-built double of the thing under test. + * + * The two halves of the ruling, and the pin for each: + * + * 1. 「⛔ 不改规则语义,只改日志形状」 — the seed load reports advisory hits as + * ONE summary line per rule, not one line per row (PIN 2). + * 2. 「同一行清库首启只响一次,按行不按写入」 — a system write-back whose + * business fields did not change does not re-evaluate advisory rules, so a + * row rings ONCE across a clean first boot rather than once per write + * (PIN 1). + * + * And the control that catches the most likely way to get B wrong: rule + * SEMANTICS must not move. An ordinary interactive write reports exactly what it + * always did, and `error`-severity rules are untouched by both halves (PIN 3). + * + * ## The boot this reproduces + * + * A clean-database first boot runs, in order: + * + * 1. `SeedLoaderService.load()` — writes the seeded rows. `owner_id` is left + * unset on purpose: no human user exists yet. + * 2. `claimSeedOwnership` (`@objectstack/plugin-security`) — the first-admin + * handoff, one predicate write per unowned shape: + * + * ql.update(name, { owner_id: adminUserId }, + * { where: { owner_id: null }, multi: true, + * context: { isSystem: true } }) + * + * Step 2 is issued verbatim below rather than by calling `claimSeedOwnership` + * itself: `@objectstack/plugin-security` does not depend on `@objectstack/ + * objectql` (its deps are core / formula / metadata-core / platform-objects / + * spec / types), so it cannot be driven against a real engine from this side of + * the graph. What CAN be wrong about a re-spelling is the payload, and the + * payload is the whole predicate under test — so it is quoted from the source + * above and kept to one key, exactly as that function writes it. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { SeedLoaderService } from '@objectstack/metadata-protocol'; +import { ObjectQL } from '../engine.js'; + +const ADVISORY_RULE = 'related_to_required'; +const ADVISORY_MESSAGE = 'At least one related record should be selected.'; + +/** + * The hotcrm shape: an internal task may legitimately have no parent record, and + * the rule that says otherwise is written for a person filling in a form. + * + * `adv_task` is ownership-eligible (no `managedBy`, not `sys_*`), so + * `resolveInjectedSystemColumns` reports `owner_id` for it — which is what makes + * the claim scan's payload a system-column-only write. + */ +const TASK = { + name: 'adv_task', + label: 'Task', + fields: { + name: { name: 'name', label: 'Name', type: 'text' }, + related_to: { name: 'related_to', label: 'Related to', type: 'text' }, + }, + validations: [ + { + type: 'script' as const, + name: ADVISORY_RULE, + severity: 'warning' as const, + condition: { dialect: 'cel', source: 'record.related_to == null' }, + message: ADVISORY_MESSAGE, + events: ['insert', 'update'] as Array<'insert' | 'update'>, + }, + { + // The `error`-severity control. Keyed on `owner_id` DELIBERATELY: that is + // the one column the claim scan writes, so this rule is evaluated by + // exactly the write the advisory half stops evaluating. If the by-row gate + // ever widened from "advisory" to "all rules", this is what goes red. + type: 'script' as const, + name: 'owner_must_not_be_forbidden', + severity: 'error' as const, + condition: { dialect: 'cel', source: 'record.owner_id == "usr_forbidden"' }, + message: 'That owner may not hold records.', + events: ['insert', 'update'] as Array<'insert' | 'update'>, + }, + ], +}; + +const SEED_CONFIG = { + dryRun: false, haltOnError: false, multiPass: true, + defaultMode: 'upsert', batchSize: 1000, transaction: false, +}; + +function makeMemoryDriver() { + const stores = new Map>>(); + const storeFor = (obj: string) => { + let s = stores.get(obj); + if (!s) { s = new Map(); stores.set(obj, s); } + return s; + }; + let nextId = 0; + const matchesWhere = (row: Record, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + for (const [k, v] of Object.entries(where)) { + if (k === '$and' && Array.isArray(v)) { + if (!v.every((w: any) => matchesWhere(row, w))) return false; + continue; + } + if (k === '$or' && Array.isArray(v)) { + if (!v.some((w: any) => matchesWhere(row, w))) return false; + continue; + } + if (k.startsWith('$')) continue; + const rowVal = row[k]; + if (v && typeof v === 'object' && '$in' in (v as any)) { + const list = (v as any).$in as unknown[]; + if (!list.includes(rowVal)) return false; + continue; + } + const expected = (v && typeof v === 'object' && '$eq' in (v as any)) ? (v as any).$eq : v; + const a = rowVal === undefined ? null : rowVal; + const b = expected === undefined ? null : expected; + if (a !== b) return false; + } + return true; + }; + const driver: any = { + name: 'memory', version: '0.0.0', supports: {} as any, + 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) => matchesWhere(r, ast?.where)); + }, + async findOne(object: string, ast: any) { + for (const r of storeFor(object).values()) if (matchesWhere(r, ast?.where)) return r; + return null; + }, + async create(object: string, data: Record) { + nextId += 1; + const id = (data.id as string) ?? `r_${nextId}`; + const row = { ...data, id }; + storeFor(object).set(id, row); + return row; + }, + async update(object: string, id: string, data: Record) { + const s = storeFor(object); + const cur = s.get(id); + if (!cur) throw new Error(`not found: ${object}/${id}`); + const updated = { ...cur, ...data, id }; + s.set(id, updated); + return updated; + }, + /** The predicate write `claimSeedOwnership` issues; resolves a COUNT (#4639). */ + async updateMany(object: string, ast: any, data: Record) { + const s = storeFor(object); + let count = 0; + for (const [id, row] of [...s.entries()]) { + if (!matchesWhere(row, ast?.where)) continue; + s.set(id, { ...row, ...data, id }); + count += 1; + } + return count; + }, + async upsert(object: string, data: Record) { + const id = data.id as string | undefined; + if (id && storeFor(object).has(id)) return this.update(object, id, data); + return this.create(object, data); + }, + async delete(object: string, id: string) { return storeFor(object).delete(id); }, + async count(object: string, ast: any) { return (await this.find(object, ast)).length; }, + async bulkCreate(object: string, rows: Record[]) { + return Promise.all(rows.map((r) => this.create(object, r))); + }, + async bulkUpdate() { return []; }, async bulkDelete() {}, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, storeFor }; +} + +/** Every `warn` line either the engine or the loader emitted, in order. */ +type WarnLog = string[]; + +function makeLogger(warns: WarnLog) { + return { + info() {}, debug() {}, error() {}, + warn(message: string) { warns.push(String(message)); }, + }; +} + +/** Lines that report an advisory hit, by either shape (per-row or summary). */ +function advisoryLines(warns: WarnLog): string[] { + return warns.filter((line) => line.includes(ADVISORY_RULE)); +} + +/** The per-row shape the evaluator has always used off the machine path. */ +function perRowLines(warns: WarnLog): string[] { + return warns.filter((line) => line.startsWith(`Validation rule '${ADVISORY_RULE}'`)); +} + +/** The aggregated shape (#13889). */ +function summaryLines(warns: WarnLog): string[] { + return warns.filter((line) => line.includes('[SeedLoader]') && line.includes('advisory rule')); +} + +describe('[#13889] advisory rules on the seed / bootstrap load path', () => { + let engine: ObjectQL; + let warns: WarnLog; + let loader: SeedLoaderService; + let storeFor: ReturnType['storeFor']; + + beforeEach(async () => { + warns = []; + const logger = makeLogger(warns); + // ONE logger for both, exactly as the boot has it: the engine's advisory + // lines and the loader's summary land in the same startup log, which is the + // log the ruling is about. + engine = new ObjectQL({ logger }); + const d = makeMemoryDriver(); + storeFor = d.storeFor; + engine.registerDriver(d.driver, true); + await engine.init(); + engine.registry.registerObject(TASK as never, 'com.objectstack.test.13889'); + const metadata = { getObject: async () => TASK, listObjects: async () => [TASK] }; + loader = new SeedLoaderService(engine as never, metadata as never, logger as never); + }); + + const seed = (records: Record[]) => loader.load({ + seeds: [{ + object: 'adv_task', + externalId: 'name', + mode: 'upsert', + env: ['prod', 'dev', 'test'], + records, + }], + config: SEED_CONFIG, + } as never); + + /** The claim scan's write, verbatim in shape. */ + const claimSeedOwnership = (adminUserId: string) => engine.update( + 'adv_task', + { owner_id: adminUserId }, + { where: { owner_id: null }, multi: true, context: { isSystem: true } } as never, + ); + + it('PIN 1 — one row rings ONCE across a clean first boot (seed insert, then the claim scan)', async () => { + // Step 1: the seed writes one legitimately parentless row. + const result = await seed([{ name: 'Internal chore', related_to: null }]); + expect(result.summary.totalInserted).toBe(1); + + // Step 2: the first-admin handoff re-owns it. Business fields did not move. + const claimed = await claimSeedOwnership('usr_admin_1'); + expect(claimed).toBe(1); + // …and it really landed, so this is not a pin over a write that never happened. + expect([...storeFor('adv_task').values()][0].owner_id).toBe('usr_admin_1'); + + // THE ANCHOR: the row rang exactly once for the whole boot. Before this + // card it rang twice — once on the seed insert, once more when the claim + // scan rewrote `owner_id` and every rule was re-evaluated. + const reports = advisoryLines(warns); + expect(reports).toHaveLength(1); + // Counted BY ROW: the one report is about one row, not two writes. + expect(reports[0]).toContain('1 row(s)'); + }); + + it('PIN 2 — a seed load reports ONE summary line for N rows, not N lines', async () => { + const result = await seed([ + { name: 'Chore A', related_to: null }, + { name: 'Chore B', related_to: null }, + { name: 'Chore C', related_to: null }, + ]); + expect(result.summary.totalInserted).toBe(3); + + // The shape from the ruling: 「N 行触发 advisory 规则 X,详见…」. + const summaries = summaryLines(warns); + expect(summaries).toHaveLength(1); + expect(summaries[0]).toContain('3 row(s)'); + expect(summaries[0]).toContain(ADVISORY_RULE); + expect(summaries[0]).toContain('warning'); + expect(summaries[0]).toContain('adv_task'); + // The rule's own sentence survives — aggregating must not cost the reader + // WHAT the advisory said, or the summary is a count with no remedy. + expect(summaries[0]).toContain(ADVISORY_MESSAGE); + // …and the 「详见…」 half points at real rows. + expect(summaries[0]).toMatch(/Example row\(s\):/); + + // ⛔ And NOT one line per row — that is the defect. + expect(perRowLines(warns)).toHaveLength(0); + expect(advisoryLines(warns)).toHaveLength(1); + }); + + it('PIN 3a — CONTROL: an ordinary interactive write still reports its advisory, unchanged', async () => { + await seed([{ name: 'Chore A', related_to: null }]); + const id = [...storeFor('adv_task').keys()][0]; + warns.length = 0; + + // A human edits the row's business content. No aggregation scope is open + // and the payload names a business field, so BOTH halves must stand aside. + await engine.update('adv_task', { id, name: 'Chore A (renamed)' } as never); + + const perRow = perRowLines(warns); + expect(perRow).toHaveLength(1); + // Byte-for-byte the historical sentence — the rule's semantics AND its + // report off the machine path are what this card must not move. + expect(perRow[0]).toBe( + `Validation rule '${ADVISORY_RULE}' (warning): ${ADVISORY_MESSAGE}`, + ); + expect(summaryLines(warns)).toHaveLength(0); + }); + + it('PIN 3b — CONTROL: an `error` rule is untouched by the by-row gate', async () => { + await seed([{ name: 'Chore A', related_to: null }]); + warns.length = 0; + + // A system-column-only write — the exact shape the advisory half skips — + // that violates an ERROR rule. It must still be rejected: an invariant is an + // invariant on every write, however little it moved. + await expect( + engine.update( + 'adv_task', + { owner_id: 'usr_forbidden' }, + { where: { owner_id: null }, multi: true, context: { isSystem: true } } as never, + ), + ).rejects.toThrow(/That owner may not hold records/); + + // …and it was the error rule that refused it, not an advisory turning into + // a rejection. + expect(advisoryLines(warns)).toHaveLength(0); + }); + + it('PIN 3c — CONTROL: an `error` rule still rejects an ordinary interactive write', async () => { + await seed([{ name: 'Chore A', related_to: null }]); + const id = [...storeFor('adv_task').keys()][0]; + + await expect( + engine.update('adv_task', { id, owner_id: 'usr_forbidden', name: 'edited' } as never), + ).rejects.toThrow(/That owner may not hold records/); + }); +}); diff --git a/packages/objectql/src/validation/rule-validator.ts b/packages/objectql/src/validation/rule-validator.ts index e789f3d24c..7c03fe5491 100644 --- a/packages/objectql/src/validation/rule-validator.ts +++ b/packages/objectql/src/validation/rule-validator.ts @@ -1974,13 +1974,26 @@ function advisoryObjectName(objectSchema: unknown): string { return typeof name === 'string' && name.length > 0 ? name : '(unknown)'; } -/** One example row id for an advisory group, when the write carries one. */ -function advisoryRecordId( +/** + * One example row reference for an advisory group, when the write carries one. + * + * The `name` fallback is not a nicety. On the path this exists for — a seed + * INSERT — the driver has not issued an id yet when rules are evaluated, so an + * id-only reference is `undefined` for exactly the rows the summary is about, + * and the 「详见…」 half of the ruling's shape would always be empty. `name` is + * this framework's canonical row handle (and the seed loader's own errors + * already name a row that way: `Failed to write sd_acct record #1 + * (name=bad_row)`), so the spelling is the loader's, not a new convention. + */ +function advisoryRecordRef( merged: Record, data: Record, ): string | undefined { const id = merged.id ?? data.id; - return typeof id === 'string' || typeof id === 'number' ? String(id) : undefined; + if (typeof id === 'string' || typeof id === 'number') return String(id); + const name = merged.name ?? data.name; + if (typeof name === 'string' && name.length > 0) return `name=${name}`; + return undefined; } /** @@ -2228,7 +2241,7 @@ export function evaluateValidationRules( rule: String(rule.name ?? '(unnamed)'), severity, message: violation.message, - recordId: advisoryRecordId(merged, data), + recordRef: advisoryRecordRef(merged, data), }) ) { opts.logger?.warn?.( From 24e7c61935d45e7ae44f12462ca440411d1a1b46 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 09:09:34 +0000 Subject: [PATCH 3/5] docs(#13889): changeset for advisory boot-path aggregation Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01ARYe3yQTQCUFm5qPYNgKaJ --- .changeset/advisory-boot-path-aggregation.md | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 .changeset/advisory-boot-path-aggregation.md diff --git a/.changeset/advisory-boot-path-aggregation.md b/.changeset/advisory-boot-path-aggregation.md new file mode 100644 index 0000000000..682e37fdeb --- /dev/null +++ b/.changeset/advisory-boot-path-aggregation.md @@ -0,0 +1,16 @@ +--- +"@objectstack/core": minor +"@objectstack/objectql": minor +"@objectstack/metadata-protocol": minor +--- + +Advisory validation rules no longer flood the startup log, and no longer count a row twice on a clean first boot. + +A `severity: 'warning'` (or `'info'`) validation rule is advisory: it never blocks a write, and its message is written for a person filling in a form. Evaluated across a seed load it produced one `WARN` line per row, so a clean-database first boot opened with a wall of form hints re-cast as boot diagnostics — and an app could reach "zero warnings" only by bending its data or deleting the rule. + +Two changes, and neither moves what a rule evaluates to: + +- **Aggregated reporting on the seed/boot path.** `SeedLoaderService.load()` now runs inside an advisory aggregation scope, and reports one summary line per rule — the rule, the object, the row count, the rule's own message and example rows — instead of one line per row. Off that path (an ordinary interactive write) nothing changes: the same per-write line is emitted verbatim. The new scope is `runWithAdvisoryAggregation` / `recordAdvisoryHit` in `@objectstack/core`. +- **Advisory rules are counted by row, not by write.** An `update` whose payload touches only platform-injected system columns — the shape `claimSeedOwnership` writes when it hands seeded rows to the first admin, `{ owner_id }` — changes no business field, so it no longer re-evaluates the object's advisory rules. Previously a seeded row rang once on insert and again when the claim scan rewrote `owner_id`, so anyone counting startup warnings over-estimated by the number of claimed objects. + +`error`-severity rules are untouched by both changes: an invariant is still enforced on every write, whoever issued it and however little it moved. Membership of the "system column" set is resolved per object by `resolveInjectedSystemColumns`, so an object that declares `ownership: 'org'` (no `owner_id`) or `systemFields: false` is judged on its own columns rather than a fixed list. From ddc95b1e62f56ded02a49c7596276b1c8b302590 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 09:19:47 +0000 Subject: [PATCH 4/5] fix(#13889): honour the caller's bound in the new test double; re-anchor census lines Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01ARYe3yQTQCUFm5qPYNgKaJ --- content/docs/permissions/system-context.mdx | 2 +- .../objectql/src/validation/advisory-boot-path.test.ts | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index c86c955925..61090d4cf3 100644 --- a/content/docs/permissions/system-context.mdx +++ b/content/docs/permissions/system-context.mdx @@ -193,7 +193,7 @@ assuming `isSystem` covers it is a documented source of bugs. | Assumption | Reality | Anchor | |:---|:---|:---| -| "It suppresses triggers / record-change automation" | **No.** Only `skipTriggers` does. A bare `{ isSystem: true }` on a seed write re-fired automation on freshly seeded rows and wedged first boot | `metadata-protocol/src/seed-loader.ts:1971` (rationale at `:1881`–`1883`, #3760), `flow.zod.ts:702` | +| "It suppresses triggers / record-change automation" | **No.** Only `skipTriggers` does. A bare `{ isSystem: true }` on a seed write re-fired automation on freshly seeded rows and wedged first boot | `metadata-protocol/src/seed-loader.ts:2032` (rationale at `:1942`–`1944`, #3760), `flow.zod.ts:702` | | "It skips the state machine" | **No.** That is `skipStateMachine`, carried by seed replay and by `treatAsHistorical` imports | `objectql/src/engine.ts` FSM gate; see [State Machine](/docs/protocol/objectql/state-machine) | | "It skips validation rules" | **No.** Field shape, `format`, `script` and the rest still run. The `readonly` strip runs *before* validation precisely so a discarded value is not judged | `objectql/src/engine.ts:10089`–`10106` | | "It preserves a supplied `updated_at` / `updated_by`" | **No.** That is `preserveAudit`, a separate opt-in — and an UPDATE-path exemption only | `field.zod.ts:1580` (#3493 / #6640) | diff --git a/packages/objectql/src/validation/advisory-boot-path.test.ts b/packages/objectql/src/validation/advisory-boot-path.test.ts index 9916085c5b..10e5d221ea 100644 --- a/packages/objectql/src/validation/advisory-boot-path.test.ts +++ b/packages/objectql/src/validation/advisory-boot-path.test.ts @@ -134,7 +134,12 @@ function makeMemoryDriver() { 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) => matchesWhere(r, ast?.where)); + const rows = Array.from(storeFor(object).values()).filter((r) => matchesWhere(r, ast?.where)); + // Hold the caller's bound, AFTER the filter and by PRESENCE. Not + // decoration: `claimSeedOwnership`'s paged fallback reads with + // `limit: CLAIM_PAGE_ROWS`, so a limit-blind double would answer a paged + // read with the whole table and quietly test a different function. + 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 (matchesWhere(r, ast?.where)) return r; From 834991e11648c9f8c044b7a44fdde559ae68751e Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 09:46:25 +0000 Subject: [PATCH 5/5] docs(#13889): correct advisory-severity prose the change falsifies Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01ARYe3yQTQCUFm5qPYNgKaJ --- content/docs/data-modeling/validation.mdx | 7 +++++-- content/docs/protocol/objectql/state-machine.mdx | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/content/docs/data-modeling/validation.mdx b/content/docs/data-modeling/validation.mdx index 0291807e9c..4b9a4059c3 100644 --- a/content/docs/data-modeling/validation.mdx +++ b/content/docs/data-modeling/validation.mdx @@ -83,9 +83,11 @@ All validation types share these base properties: | Severity | Behavior | | :--- | :--- | | `error` | Prevents the record from being saved | -| `warning` | Shows a warning but allows save | +| `warning` | Allows the save; advisory only — logged server-side, not returned to the caller | | `info` | Informational message, no blocking | +Advisory rules (`warning` / `info`) are reported, never enforced: on an ordinary write each hit is logged as it happens; on a seed/boot load a run's hits are folded into **one summary line per rule**; and an `update` touching only platform-injected system columns does not re-evaluate them at all, so a row is reported once rather than once per write (#13889). + ## Validation Types ### Script Validation @@ -112,7 +114,8 @@ fix it. Until protocol 17 such a rule was logged at WARN and *skipped*, so the w went through while the rule stayed declared and enforced nothing; a validation exists to reject a write, and "the rule could not be checked" must never resolve to "allowed" (#4649). `severity` still governs blocking — an unevaluable `warning` / -`info` rule is logged and does not throw. +`info` rule is logged and does not throw, on the writes where advisory rules are +evaluated at all (see above). The record a predicate reads is the stored row overlaid with this write's payload, **total over the object's declared fields** (`null` for a declared field present in diff --git a/content/docs/protocol/objectql/state-machine.mdx b/content/docs/protocol/objectql/state-machine.mdx index 0b0617b2cd..3cdb53e9dd 100644 --- a/content/docs/protocol/objectql/state-machine.mdx +++ b/content/docs/protocol/objectql/state-machine.mdx @@ -108,7 +108,7 @@ transitions: { - **On insert**, the `transitions` table is not consulted — there is no prior state to transition from. If the rule declares `initialStates`, the created value must be one of them, or the write is rejected with `invalid_initial_state` (the FSM entry point). Without `initialStates`, insert is a no-op and the starting value is constrained only by the field-level `select` option-membership check (`invalid_option`), so any declared option is a legal start. - **On update**, if the state field changed and the new value is **not** in `transitions[oldValue]`, the write is rejected. Clearing the field (writing `null`) is exempt. - The check is **lenient where it cannot reason**: if the prior state has no key in `transitions` (legacy or externally-written data, or a state you simply forgot to declare), it does not block. Only an explicit `[]` makes a state a hard dead-end. -- Only a rule with `severity: 'error'` (the default) blocks the write; `warning`/`info` are logged. +- Only a rule with `severity: 'error'` (the default) blocks the write. `warning`/`info` are advisory: logged per write on an ordinary write, folded into **one summary line per rule** on a seed/boot load, and not evaluated at all on an `update` whose payload touches only platform-injected system columns (#13889). - **Seed writes are exempt** (#3433). Curated seed data — package bootstrap fixtures, marketplace templates, per-org replay, all loaded by `SeedLoaderService` — is a snapshot of established facts, not a record walking its lifecycle, so it bypasses the `state_machine` rule entirely: a seed may be born mid-lifecycle (a `completed` project, a `closed_won` opportunity) and neither `initialStates` (insert) nor `transitions` (update) is enforced. Every *other* validation still runs, so a seed must still satisfy field shape, `format`, `script`, and the rest. `os lint` warns when a seeded value is not a state the machine declares, so a typo is still caught before boot. - **A "historical" data import is exempt too** (#3479). Migrating established facts — a batch of already-`closed` tickets, `closed_won` deals — is the same "snapshot, not a lifecycle event" situation. Set `treatAsHistorical: true` on the import request (default **off**) and the runner puts `skipStateMachine` on the write context, so `initialStates` doesn't reject those mid-lifecycle rows. A normal import leaves it off and still walks the FSM — the strict behavior is the default, so the exemption is always an explicit opt-in. - **`treatAsHistorical` also preserves the original audit timeline** (#3493) — **on the rows an import UPDATES** (#6640). Skipping the FSM is only half of migrating established facts; the other half is keeping *when* they happened and *who* did them. Under the same flag the write context also carries `preserveAudit`, which (1) makes `updated_at` / `updated_by` **client-preferred** — a supplied historical last-modified survives instead of being stamped with the import instant — and (2) admits a **whitelist** through the static-`readonly` write strip: the audit/timestamp family plus author-declared business `readonly` fields (`closed_at`, `resolved_by`, …) — but never the record's own primary key (`id`), which is the address of the write rather than a fact being restored (#8215). Platform-managed `system` columns outside that family (`organization_id` and other tenancy/generated columns) stay stripped — a historical import reinstates facts, it does not forge tenancy. Like the FSM exemption this is opt-in: a normal write still auto-stamps `updated_at`/`updated_by` and strips `readonly` exactly as before, and permissions / RLS / field-level security are unchanged.