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. 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/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/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. 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 0000000000..c743c50af2 --- /dev/null +++ b/packages/core/src/utils/advisory-aggregation.ts @@ -0,0 +1,183 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Advisory-hit aggregation for machine load paths (#13889). + * + * ## The defect this closes + * + * A `severity: 'warning'` (or `'info'`) validation rule is ADVISORY: it never + * blocks a write, and its message is written for a HUMAN IN A FORM ("At least + * one related record should be selected"). The evaluator's only report channel + * for one is `logger.warn`, one line per (row × violated rule). + * + * On an interactive write that is exactly right. On the seed / bootstrap load + * path it is not: a clean-database first boot writes every seeded row through + * the same evaluator, so an app whose seed legitimately contains N rows that + * trip one advisory rule gets N `WARN` lines in the STARTUP LOG — a form hint + * re-cast as a boot diagnostic, which reads like the boot failed. Measured + * downstream (hotcrm#1203): the only ways an app could reach "zero warnings" + * were to bend its data (attach a parent record it does not have) or delete the + * rule (weaken a real guard). Both are worse than the noise. + * + * Maintainer ruling (2026-09-01, verbatim 「同意」 on option B): + * 「⛔ 不改规则语义,只改日志形状」 — do not change rule semantics, change + * only the shape of the log. So this module changes NOTHING about what a rule + * evaluates to, who it applies to, or whether it blocks. It changes where the + * REPORT goes while a machine load path is running, and nothing else. + * + * ## Why an ambient scope rather than a parameter + * + * The producer (`evaluateValidationRules`, `@objectstack/objectql`) and the + * scope owner (`SeedLoaderService.load`, `@objectstack/metadata-protocol`) are + * separated by the whole engine write path: the loader calls + * `IDataEngine.insert/update`, and the evaluator is reached several layers + * below through a contract that carries no reporting channel. Threading a sink + * through would mean widening `IDataEngine`'s options — a published contract — + * for a diagnostic concern. An ambient scope keeps the change inside the two + * ends that care. + * + * `AsyncLocalStorage` rather than a module-level flag, deliberately: seed loads + * are NOT boot-only. A per-org replay (`sys_organization` insert) runs a full + * seed load on a LIVE server, concurrently with ordinary interactive traffic. A + * plain global would capture those interactive writes' advisories into the + * replay's summary — silently swallowing a report meant for someone else. ALS + * scopes the capture to the load's own async context, which is the difference + * between aggregating and losing. + * + * ## Folded on arrival, never accumulated per row + * + * A hit is folded into its `(object, rule)` group as it arrives, so a load of + * 100 000 rows that all trip one rule holds ONE group, not 100 000 records. + * The group keeps what a reader needs to act — the rule, the object, the + * severity, the message, the row count, and up to {@link ADVISORY_SAMPLE_ROWS} + * example rows — which is the 「详见…」 half of the ruling's summary shape. + */ + +import { AsyncLocalStorage } from 'node:async_hooks'; + +/** Example row references a group carries, so the summary can point at real rows. */ +export const ADVISORY_SAMPLE_ROWS = 5; + +/** One advisory rule hit, as the evaluator reports it. */ +export interface AdvisoryHit { + /** Object the row belongs to. */ + object: string; + /** The declared rule's `name`. */ + rule: string; + /** The rule's declared severity — `'warning'` or `'info'`; never `'error'`. */ + severity: string; + /** The rule's author-written message, in the caller's locale. */ + message: string; + /** + * A reference to the row, when the write carries one. + * + * NOT necessarily an id: on the path this exists for — a seed INSERT — the + * driver has not issued an id yet at validation time, so an id-only reference + * would be empty for exactly the case the aggregation was built for. The + * producer sends the best stable handle it has (`id`, else `name=`, + * the same way the seed loader names a row in its own errors). + */ + recordRef?: string; +} + +/** Every hit for one `(object, rule)` pair, folded. */ +export interface AdvisoryGroup { + object: string; + rule: string; + severity: string; + /** The first message seen for this group (they differ only by interpolation). */ + message: string; + /** How many ROWS tripped this rule during the scope. */ + rows: number; + /** Up to {@link ADVISORY_SAMPLE_ROWS} example row references. */ + sampleRows: string[]; +} + +interface AdvisoryCollector { + groups: Map; +} + +const storage = new AsyncLocalStorage(); + +/** + * Group key. + * + * `JSON.stringify` of the pair rather than a delimiter-joined string: any + * single-character delimiter is a claim about what an object or rule name + * cannot contain, and a wrong claim collides two groups into one silently. + * Encoding the pair makes the key injective with nothing to be wrong about. + */ +function keyOf(object: string, rule: string): string { + return JSON.stringify([object, rule]); +} + +/** + * Offer one advisory hit to the active aggregation scope. + * + * @returns `true` when a scope captured it — the caller must then NOT log its + * own per-row line, because the scope owner reports the whole group. `false` + * when no scope is active, which is the ordinary interactive case: the caller + * logs exactly as it always did. A caller that ignores the return value + * degrades to today's behaviour rather than losing the report. + */ +export function recordAdvisoryHit(hit: AdvisoryHit): boolean { + const collector = storage.getStore(); + if (!collector) return false; + const key = keyOf(hit.object, hit.rule); + const existing = collector.groups.get(key); + if (existing) { + existing.rows += 1; + if (hit.recordRef != null && existing.sampleRows.length < ADVISORY_SAMPLE_ROWS) { + existing.sampleRows.push(hit.recordRef); + } + return true; + } + collector.groups.set(key, { + object: hit.object, + rule: hit.rule, + severity: hit.severity, + message: hit.message, + rows: 1, + sampleRows: hit.recordRef != null ? [hit.recordRef] : [], + }); + return true; +} + +/** + * Whether an advisory aggregation scope is active on this async context. + * + * Exported for tests and for a caller that wants to skip building a message it + * is about to discard; {@link recordAdvisoryHit}'s return value is the one that + * decides. + */ +export function isAggregatingAdvisories(): boolean { + return storage.getStore() !== undefined; +} + +/** + * Run `fn` with advisory hits aggregated, then hand the folded groups to + * `report`. + * + * `report` runs in a `finally`, so a load that throws still reports what it + * tripped before failing — the diagnostics of a half-finished seed are the ones + * most worth having. It is called only when there is something to report, and + * its own failure is never allowed to replace the caller's outcome: a reporting + * bug must not turn a successful seed load into a failed one. + */ +export async function runWithAdvisoryAggregation( + fn: () => Promise, + report: (groups: AdvisoryGroup[]) => void, +): Promise { + const collector: AdvisoryCollector = { groups: new Map() }; + try { + return await storage.run(collector, fn); + } finally { + if (collector.groups.size > 0) { + try { + report([...collector.groups.values()]); + } catch { + // Reporting is a diagnostic, never an outcome. + } + } + } +} diff --git a/packages/metadata-protocol/src/seed-loader.ts b/packages/metadata-protocol/src/seed-loader.ts index c9b945e811..c08c1f0977 100644 --- a/packages/metadata-protocol/src/seed-loader.ts +++ b/packages/metadata-protocol/src/seed-loader.ts @@ -15,7 +15,7 @@ import type { } from '@objectstack/spec/data'; import { SeedLoaderConfigSchema, isMultiValueField } from '@objectstack/spec/data'; import { resolveSeedRecord } from '@objectstack/formula'; -import { bulkWrite, withTransientRetry, defaultIsTransientError, type BulkWriteRowResult } from '@objectstack/core'; +import { bulkWrite, withTransientRetry, defaultIsTransientError, type BulkWriteRowResult, runWithAdvisoryAggregation, type AdvisoryGroup } from '@objectstack/core'; // [#8442] The repo's ONE recogniser for "this throw is a record-validation // failure" — duck-typed on `code`/`name`, the same predicate `mapDataError` and // both dispatcher error exits use. Imported rather than re-spelled so the seed @@ -395,7 +395,68 @@ export class SeedLoaderService implements ISeedLoaderService { // Public API // ========================================================================== + /** + * Load every dataset in the request. + * + * [#13889] The whole load runs inside an ADVISORY AGGREGATION SCOPE. A + * `severity: 'warning'` validation rule is a hint written for a human in a + * form; evaluated across a seed load it produces one `WARN` line per row, and + * a clean-database first boot then opens with a wall of them that reads like a + * failed boot. Inside the scope those hits are folded by rule and reported by + * {@link reportAdvisorySummary} as ONE line each — maintainer ruling + * 2026-09-01, 「⛔ 不改规则语义,只改日志形状」. + * + * The scope is opened HERE rather than at the callers because this method is + * the one funnel every seeding path goes through — boot inline seed, per-org + * replay, hot reload, package apply, draft publish, marketplace install — the + * same reason `resolveEnvConfig` is resolved here rather than at the six call + * sites that build a request (#4704). A caller that writes seed rows through + * the engine directly, bypassing this loader (app-plugin's metadata-service + * fallback), keeps the per-row lines: no scope is open, so nothing changes for + * it. + */ async load(request: SeedLoaderRequestParsed): Promise { + 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.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}': ` + + `${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, + sampleRows: group.sampleRows, + }, + ); + } + } + + 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/advisory-boot-path.test.ts b/packages/objectql/src/validation/advisory-boot-path.test.ts new file mode 100644 index 0000000000..10e5d221ea --- /dev/null +++ b/packages/objectql/src/validation/advisory-boot-path.test.ts @@ -0,0 +1,349 @@ +// 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) { + 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; + 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 0cc67e6f91..7c03fe5491 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,93 @@ 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 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; + 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; +} + +/** + * [#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 +2170,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 +2228,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, + recordRef: advisoryRecordRef(merged, data), + }) + ) { opts.logger?.warn?.( `Validation rule '${rule.name}' (${severity}): ${violation.message}`, );