diff --git a/.changeset/milestone-summary-lookup-titles.md b/.changeset/milestone-summary-lookup-titles.md new file mode 100644 index 0000000000..7d20709f50 --- /dev/null +++ b/.changeset/milestone-summary-lookup-titles.md @@ -0,0 +1,38 @@ +--- +"@objectstack/plugin-audit": patch +--- + +fix(plugin-audit): `activityMilestones` summary tokens render referenced record titles, not raw ids (#7290) + +ADR-0052 §5b.2 lets an object declare a semantic milestone whose summary +interpolates `{token}`s from the record — `{ field: 'stage', value: +'closed_won', summary: 'Deal won by {owner}' }`. A token naming a `lookup` / +`master_detail` / `user` field was interpolated with no title map, so it fell +through to the raw stored id and the record timeline read `Deal won by oBK25…`. + +The milestone branch takes **precedence** over the tracked-change branch, so on +every object that declares `activityMilestones` this was the string users saw, +and #7230's fix to the tracked-change branch never reached them. + +A reference token now renders the referenced record's title, resolved through +ADR-0079's `resolveDisplayField`. The author's template wording is untouched — +only what a token resolves to changes — and the change is restore-invariant: an +id with no resolvable title (target removed out of band, unregistered object, +failing read) renders exactly as it did before. The empty-token rule is +unchanged too: an empty value still renders as the empty string, not `∅`. + +**Read cost.** `matchMilestone` was split into detection and rendering so the +read plan is built from the tokens of the template that actually **fired**, and +only then. Measured with a copy-returning counting driver: every create, every +delete, every update of a milestone-declaring object that fires no milestone, +and every fired milestone whose template names no reference token all add +**zero** reads; a fired milestone with reference tokens costs **one read per +distinct target object** (two tokens onto the same object are one batched +`id: { $in: [...] }`), paid only on the transition itself. The alternative +placement — resolving before knowing whether a milestone fired — was measured at +**3 reads on a write that fires nothing**, and was rejected: it is the shape +#6656's Option A+ ruling was obtained to remove from this write path. + +A target object that designates a credential field as its title is **not read +at all** — the same `collectMaskedReadFields` predicate the ledger masks with, +so no secret can reach a user-facing summary through this path. diff --git a/packages/plugins/plugin-audit/src/audit-milestone-summary.test.ts b/packages/plugins/plugin-audit/src/audit-milestone-summary.test.ts new file mode 100644 index 0000000000..79e3feda30 --- /dev/null +++ b/packages/plugins/plugin-audit/src/audit-milestone-summary.test.ts @@ -0,0 +1,759 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#7290] `sys_activity.summary`'s MILESTONE branch (ADR-0052 §5b.2) — a + * `{token}` naming a `lookup` / `master_detail` / `user` field renders the + * referenced record's title instead of its raw 32-char id. + * + * ## Why this needed its own card and its own file + * + * #7230 / PR #7291 fixed the same rendering defect on the tracked-CHANGE + * branch, keyed on the diff. The milestone branch takes PRECEDENCE over it, so + * on an object that declares `activityMilestones` the string users actually see + * never went through that fix: `'Deal won by {owner}'` shipped as + * `'Deal won by oBK25…'`. + * + * The two branches also cannot share a gate. #7230's gate is "a field that is + * BOTH `trackHistory: true` AND a reference actually moved" — evaluated from + * the diff, before anything is read. A milestone token is none of those things: + * it is read from the whole AFTER-row, it is whatever the author wrote (the + * showcase's own `'Deal won by {owner}'` names an UNTRACKED field — pinned + * below), and whether a milestone fires at all is only known once + * `matchMilestone` has run. + * + * ## The read budget is the reason this file measures anything + * + * This is the third card in a row on this file's write hot path. + * #6656 / PR #6977 retired `captureBefore`'s redundant pre-image read under + * maintainer ruling Option A+ (2 → 1 per single-id write, 3 → 0 per predicate + * write); #7230 / PR #7291 preserved that by adding 0 reads to every write + * shape but "a tracked reference moved". This card had two placements available + * and they are NOT equivalent: + * + * BEFORE `matchMilestone` — speculate on the after-row, pay on EVERY update + * of a milestone-declaring object, including the + * overwhelming majority where nothing fires. This + * is the shape ruling A+ was obtained to remove. + * AFTER it fires — plan from the tokens of the template that + * MATCHED. Zero on every non-firing update; one + * read per distinct target object on the + * transition itself, which is rare by construction + * (`before !== value && after === value`, and + * never again while the record sits at that value). + * + * The second was implemented. Section 3 pins it as counts, not prose, and the + * zero-read cases run against a row that HOLDS references — measured, that is + * the difference between a guard and a guard-shaped no-op (PR #7291's own + * mutation D passed 160/160 against a reference-free row before its fixture was + * hardened; the same trap is live here and is avoided the same way). + * + * ⚠️ Read counts and rendering are pinned SEPARATELY on purpose: the read lives + * in `resolveLookupTitles`, the rendering in `renderMilestoneSummary`. Deleting + * the render limb leaves every count green. + */ + +import { describe, it, expect } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { createMemoryI18n } from '@objectstack/core'; +import { installAuditWriters } from './audit-writers.js'; + +// --------------------------------------------------------------------------- +// Fixtures +// --------------------------------------------------------------------------- + +const f = (name: string, type: string, extra: Record = {}) => + ({ name, label: name, type, ...extra }) as any; + +const sysAuditLog = { + name: 'sys_audit_log', label: 'Audit Log', + fields: { + id: f('id', 'text', { primaryKey: true }), action: f('action', 'text'), + user_id: f('user_id', 'text'), object_name: f('object_name', 'text'), + record_id: f('record_id', 'text'), old_value: f('old_value', 'textarea'), + new_value: f('new_value', 'textarea'), tenant_id: f('tenant_id', 'text'), + }, +}; + +const sysActivity = { + name: 'sys_activity', label: 'Activity', + fields: { + id: f('id', 'text', { primaryKey: true }), type: f('type', 'text'), + timestamp: f('timestamp', 'datetime'), summary: f('summary', 'text'), + actor_id: f('actor_id', 'text'), object_name: f('object_name', 'text'), + record_id: f('record_id', 'text'), record_label: f('record_label', 'text'), + metadata: f('metadata', 'textarea'), + }, +}; + +/** Target objects. Title fields are DERIVED (ADR-0079) unless stated. */ +const crmAccount = { + name: 'crm_account', label: 'Account', + fields: { id: f('id', 'text', { primaryKey: true }), name: f('name', 'text') }, +}; + +const crmRegion = { + name: 'crm_region', label: 'Region', + fields: { id: f('id', 'text', { primaryKey: true }), title: f('title', 'text') }, +}; + +const sysUserObj = { + name: 'sys_user', label: 'User', + fields: { + id: f('id', 'text', { primaryKey: true }), name: f('name', 'text'), + email: f('email', 'email'), + }, +}; + +/** + * [#6656 non-regression] A target object that DESIGNATES a credential field as + * its title. `resolveDisplayField` honours an explicit `nameField` even when it + * points at a title-INELIGIBLE type, so without the mask check this object's + * `api_key` would be read and interpolated into a user-facing summary. + */ +const crmVault = { + name: 'crm_vault', label: 'Vault', nameField: 'api_key', + fields: { id: f('id', 'text', { primaryKey: true }), api_key: f('api_key', 'secret') }, +}; + +/** + * The audited object. Field-level notes, because each one is load-bearing: + * + * - `owner` is a `user` reference and is deliberately **UNTRACKED**. It is the + * exact field class and the exact declaration of the reported symptom + * (`'Deal won by {owner}'`), and it is what proves the milestone branch + * cannot reuse #7230's `trackHistory` gate. + * - `account_id` / `partner_id` point at the SAME target, so batching is + * measurable; `account_id` is tracked and `partner_id` is not, so precedence + * against the tracked branch is measurable too. + * - `region_id` gives a second distinct target. + * - `parent_id` is the `master_detail` member of the reference set. + * - `vault_id` points at the masked-title object above. + * - `priority` is a select: its option-label rendering must survive this + * change untouched (localizing those labels is #7289, a different card). + */ +const bizDeal = { + name: 'biz_deal', label: 'Deal', + fields: { + id: f('id', 'text', { primaryKey: true }), + title: f('title', 'text'), + stage: f('stage', 'text', { label: 'Stage', trackHistory: true }), + owner: f('owner', 'user', { label: 'Owner', reference: 'sys_user' }), + account_id: f('account_id', 'lookup', { + label: 'Account', reference: 'crm_account', trackHistory: true, + }), + partner_id: f('partner_id', 'lookup', { label: 'Partner', reference: 'crm_account' }), + region_id: f('region_id', 'lookup', { label: 'Region', reference: 'crm_region' }), + parent_id: f('parent_id', 'master_detail', { label: 'Parent', reference: 'crm_account' }), + vault_id: f('vault_id', 'lookup', { label: 'Vault', reference: 'crm_vault' }), + priority: f('priority', 'select', { + label: 'Priority', + options: [{ value: 'p1', label: 'Highest' }, { value: 'p2', label: 'Normal' }], + }), + }, + /** + * Every milestone watches `stage`, so exactly one can fire per write and each + * case selects its own by the value it transitions into. + */ + activityMilestones: [ + // One reference token, on an UNTRACKED field — the reported declaration. + { field: 'stage', value: 'closed_won', summary: 'Deal won by {owner}', type: 'completed' }, + // Two tokens onto the SAME target object → must batch into ONE read. + { field: 'stage', value: 'handover', summary: 'Handed to {account_id} via {partner_id}' }, + // Two DISTINCT target objects → one read each. + { field: 'stage', value: 'localized', summary: 'Won by {owner} for {region_id}' }, + // The SAME token twice → still one read, and both occurrences resolve. + { field: 'stage', value: 'escalated', summary: 'Escalated by {owner}, notified {owner}' }, + // No reference token at all — the shipped showcase shape → ZERO reads. + { field: 'stage', value: 'archived', summary: '✅ Archived: {title}' }, + // A select token: option-label rendering must be untouched by this change. + { field: 'stage', value: 'prioritized', summary: 'Priority set to {priority}' }, + // A token whose value is empty renders as the EMPTY STRING, not `∅`. + { field: 'stage', value: 'unassigned', summary: 'Unassigned deal:{owner}' }, + // master_detail is in the reference set. + { field: 'stage', value: 'merged', summary: 'Merged into {parent_id}' }, + // The masked-title target: the read must be SKIPPED, not masked downstream. + { field: 'stage', value: 'vaulted', summary: 'Vaulted under {vault_id}' }, + // An unknown token names no field at all — raw value / empty, as before. + { field: 'stage', value: 'ghosted', summary: 'Ghosted {nosuchfield}' }, + ], +}; + +// --------------------------------------------------------------------------- +// A driver that COUNTS reads and returns COPIES +// --------------------------------------------------------------------------- + +/** + * Lifted from `audit-bound-previous.test.ts` (via `audit-lookup-summary.test.ts`), + * deliberately including its copy-returning rule: a driver that hands back live + * store references lets the engine's read path rewrite the store in place, after + * which measurements taken around a write describe a store that moved under + * them. Real drivers serialise; so does this one. + */ +function makeCountingDriver() { + const stores = new Map>>(); + const reads = { findOneOn: {} as Record, findOn: {} as Record }; + const storeFor = (o: string) => { + let s = stores.get(o); + if (!s) { s = new Map(); stores.set(o, s); } + return s; + }; + let nextId = 0; + const copy = (r: T): T => (r == null ? r : JSON.parse(JSON.stringify(r))); + const matches = (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((sub) => matches(row, sub))) return false; + continue; + } + if (k.startsWith('$')) continue; + if (v && typeof v === 'object' && '$in' in v) { + if (!(v.$in as unknown[]).includes(row[k])) return false; + continue; + } + const expected = (v && typeof v === 'object' && '$eq' in v) ? v.$eq : v; + if ((row[k] ?? null) !== (expected ?? null)) 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) { + reads.findOn[object] = (reads.findOn[object] ?? 0) + 1; + return Array.from(storeFor(object).values()).filter((r) => matches(r, ast?.where)).map(copy); + }, + async findOne(object: string, ast: any) { + reads.findOneOn[object] = (reads.findOneOn[object] ?? 0) + 1; + for (const r of storeFor(object).values()) if (matches(r, ast?.where)) return copy(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 copy(row); + }, + async update(object: string, id: string, data: Record) { + const s = storeFor(object); + const cur = s.get(id); + if (!cur) return null; + const updated = { ...cur, ...data, id }; + s.set(id, updated); + return copy(updated); + }, + async upsert(object: string, data: Record) { + const id = data.id as string | undefined; + return id && storeFor(object).has(id) ? this.update(object, id, data) : 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 updateMany(object: string, ast: any, data: Record) { + const rows = await this.find(object, ast); + const s = storeFor(object); + for (const r of rows) s.set(r.id as string, { ...s.get(r.id as string), ...data, id: r.id }); + return rows.length; + }, + async deleteMany(object: string, ast: any) { + const rows = await this.find(object, ast); + for (const r of rows) storeFor(object).delete(r.id as string); + return rows.length; + }, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, reads, storeFor }; +} + +/** + * Deterministic, reversible stand-in for a KMS. `crm_vault.api_key` is a real + * `secret` field and the engine is fail-closed about persisting one without a + * provider, so the masked-title case below runs against a genuinely encrypted + * column rather than a `secret`-typed field that happens to be empty. + */ +const cryptoProvider: any = { + calls: 0, + async encrypt(plain: string) { + cryptoProvider.calls += 1; + return { + id: `sec_${cryptoProvider.calls}`, kmsKeyId: 'test-key', alg: 'test', + version: 1, ciphertext: Buffer.from(plain).toString('base64'), + }; + }, + async decrypt(handle: any) { return Buffer.from(handle.ciphertext, 'base64').toString(); }, + async rotateKey(handle: any) { return handle; }, + digest(plain: string) { return plain; }, +}; + +const OWNER_PACKAGE = 'com.objectstack.test.audit-milestone-summary'; + +/** + * A REAL locale bundle in the shape the shipped bundles carry + * (`objects..fields..label`), not a stub translator that would + * answer whatever key it was handed. It is here to pin a NON-change: a + * milestone template is the author's own sentence and is not translated, so + * turning a real locale on must leave the milestone summary alone while the + * generic verbs around it localize. + */ +function makeI18n() { + const i18n = createMemoryI18n(); + i18n.loadTranslations('zh-CN', { + objects: { + biz_deal: { + label: '商机', + fields: { stage: { label: '阶段' }, owner: { label: '负责人' } }, + }, + }, + // `{{param}}` — the shipped bundles' interpolation syntax + // (`translations/messages.ts`: `Updated {{object}} "{{label}}"`), which is + // deliberately NOT the milestone template's single-brace `{token}`. Getting + // this wrong is exactly what a stub translator would have hidden. + messages: { activityUpdated: '已更新{{object}}"{{label}}"' }, + }); + return i18n; +} + +async function boot(opts: { locale?: string; i18n?: unknown } = {}) { + const engine = new ObjectQL(); + const stub = makeCountingDriver(); + engine.registerDriver(stub.driver, true); + await engine.init(); + engine.setCryptoProvider(cryptoProvider); + for (const o of [sysAuditLog, sysActivity, crmAccount, crmRegion, sysUserObj, crmVault, bizDeal]) { + engine.registry.registerObject(o as any, OWNER_PACKAGE); + } + installAuditWriters(engine as any, 'test.audit', { + getI18n: () => opts.i18n as any, + getLocale: async () => opts.locale, + }); + return { engine, ...stub }; +} + +const lastSummary = (storeFor: (o: string) => Map>) => { + const all = Array.from(storeFor('sys_activity').values()); + return all.length > 0 ? String(all[all.length - 1].summary) : undefined; +}; + +const lastType = (storeFor: (o: string) => Map>) => { + const all = Array.from(storeFor('sys_activity').values()); + return all.length > 0 ? String(all[all.length - 1].type) : undefined; +}; + +/** + * Seed the reference targets and TWO deals: + * + * - `deal_1` holds NO references — the `∅`-side and empty-token cases. + * - `deal_2` already holds EVERY reference field. That is load-bearing, not + * incidental: a zero-read guard only guards on a row that HAS references to + * read. Measured on PR #7291, hot-path cases run against a reference-free row + * stay green under the very regression they exist to catch, because there is + * nothing on the row to read. + * + * Every zero-read case below therefore runs against `deal_2` — with ONE + * deliberate exception, written down here rather than left for the next reader + * to trip over. `a reference token whose value is EMPTY pays ZERO reads` must + * run against `deal_1`, because an empty reference is the thing that case + * measures and `deal_2` has none to offer. It is therefore NOT a placement + * guard: on a row with nothing to read, EVERY placement pays zero, so it cannot + * tell one from another. That is measured rather than reasoned — it was the + * single case in this file that stayed GREEN under the mutation moving the read + * before `matchMilestone`, the placement the other zero-read cases exist to + * reject. It is retained for the empty-value rule alone; the placement is + * guarded by its neighbours, all of which run against `deal_2`. + */ +async function seed(engine: any) { + await engine.insert('crm_account', { id: 'acc_1', name: 'Acme Corp' }); + await engine.insert('crm_account', { id: 'acc_2', name: 'Globex' }); + await engine.insert('crm_region', { id: 'reg_1', title: 'APAC' }); + await engine.insert('sys_user', { id: 'usr_1', name: '张伟', email: 'z@example.com' }); + await engine.insert('crm_vault', { id: 'vlt_1', api_key: 'sk-live-do-not-leak' }); + await engine.insert('biz_deal', { + id: 'deal_2', title: 'Already linked', stage: 'open', priority: 'p1', + owner: 'usr_1', account_id: 'acc_1', partner_id: 'acc_2', + region_id: 'reg_1', parent_id: 'acc_1', vault_id: 'vlt_1', + }); + return engine.insert('biz_deal', { id: 'deal_1', title: 'Ship it', stage: 'open' }); +} + +const moveStage = (engine: any, id: string, stage: string) => + engine.update('biz_deal', { stage }, { where: { id } } as any); + +// --------------------------------------------------------------------------- +// 1. The defect — a reference token renders a title, not a raw id +// --------------------------------------------------------------------------- + +describe('[#7290] milestone `{token}` interpolation resolves reference titles', () => { + it('renders a `user` token as the referenced record title (the reported declaration)', async () => { + const { engine, storeFor } = await boot(); + await seed(engine); + + await moveStage(engine, 'deal_2', 'closed_won'); + + // Before this change: `Deal won by usr_1` — in production, a 32-char id. + expect(lastSummary(storeFor)).toBe('Deal won by 张伟'); + expect(lastType(storeFor)).toBe('completed'); + }); + + it('resolves a token on an UNTRACKED field — the milestone branch is not gated on `trackHistory`', async () => { + const { engine, storeFor } = await boot(); + await seed(engine); + + // `owner` carries no `trackHistory`, so #7230's diff-keyed gate can never + // see it. If this renders a raw id, the milestone branch has been wired to + // the tracked-change gate and the reported defect is still shipping. + expect((bizDeal.fields as any).owner.trackHistory).toBeUndefined(); + await moveStage(engine, 'deal_2', 'closed_won'); + + expect(lastSummary(storeFor)).toBe('Deal won by 张伟'); + }); + + it('resolves TWO tokens pointing at the same target object', async () => { + const { engine, storeFor } = await boot(); + await seed(engine); + + await moveStage(engine, 'deal_2', 'handover'); + + expect(lastSummary(storeFor)).toBe('Handed to Acme Corp via Globex'); + }); + + it('resolves tokens across TWO distinct target objects', async () => { + const { engine, storeFor } = await boot(); + await seed(engine); + + await moveStage(engine, 'deal_2', 'localized'); + + expect(lastSummary(storeFor)).toBe('Won by 张伟 for APAC'); + }); + + it('resolves a `master_detail` token', async () => { + const { engine, storeFor } = await boot(); + await seed(engine); + + await moveStage(engine, 'deal_2', 'merged'); + + expect(lastSummary(storeFor)).toBe('Merged into Acme Corp'); + }); + + it('resolves the SAME token at every occurrence', async () => { + const { engine, storeFor } = await boot(); + await seed(engine); + + await moveStage(engine, 'deal_2', 'escalated'); + + expect(lastSummary(storeFor)).toBe('Escalated by 张伟, notified 张伟'); + }); + + it("leaves the author's template wording — including non-token text — untouched", async () => { + const { engine, storeFor } = await boot(); + await seed(engine); + + await moveStage(engine, 'deal_2', 'archived'); + + // Only what a token RESOLVES TO is this card's business; the sentence, + // its punctuation and its emoji are the author's. + expect(lastSummary(storeFor)).toBe('✅ Archived: Already linked'); + }); + + it('leaves select option-label rendering exactly as it was (#7289 owns that surface)', async () => { + const { engine, storeFor } = await boot(); + await seed(engine); + + await moveStage(engine, 'deal_2', 'prioritized'); + + expect(lastSummary(storeFor)).toBe('Priority set to Highest'); + }); + + it("keeps the empty-value rule: an empty token renders '' , not '∅'", async () => { + const { engine, storeFor } = await boot(); + await seed(engine); + + // `deal_1` holds no `owner`. A milestone sentence is prose, not a diff row, + // so the pre-#7290 empty rendering is preserved verbatim. + await moveStage(engine, 'deal_1', 'unassigned'); + + expect(lastSummary(storeFor)).toBe('Unassigned deal:'); + }); + + it('renders the RAW ID when the referenced record cannot be resolved (restore-invariant)', async () => { + const { engine, storeFor } = await boot(); + await seed(engine); + + // A dangling reference cannot be authored — the engine's + // `assertReferencesResolve` refuses it on write — so the realistic + // unresolvable case is a reference that WAS valid and whose target has since + // gone away. Removed out of band here, which is also what a hard delete or + // an unregistered target object looks like from this function. + storeFor('sys_user').delete('usr_1'); + // A MIXED template on purpose (`'Won by {owner} for {region_id}'`): the + // unresolvable side must stay raw AND the resolvable side must still become + // a title. A single-token fallback assertion would survive the deletion of + // the render limb itself — measured on PR #7291's mutation B, which is why + // this case is written the harder way from the start. + await moveStage(engine, 'deal_2', 'localized'); + + expect(lastSummary(storeFor)).toBe('Won by usr_1 for APAC'); + }); + + it('leaves a token that names no field alone', async () => { + const { engine, storeFor } = await boot(); + await seed(engine); + + await moveStage(engine, 'deal_2', 'ghosted'); + + expect(lastSummary(storeFor)).toBe('Ghosted '); + }); +}); + +// --------------------------------------------------------------------------- +// 2. Precedence and locale — both are NON-changes, pinned so they stay that way +// --------------------------------------------------------------------------- + +describe('[#7290] precedence and locale behaviour are unchanged', () => { + it('the milestone still wins over the tracked field-change summary', async () => { + const { engine, storeFor } = await boot(); + await seed(engine); + + // `stage` and the tracked reference `account_id` move together. Without + // the milestone this would read `Stage: open → closed_won; Account: …`. + await engine.update( + 'biz_deal', + { stage: 'closed_won', account_id: 'acc_2' }, + { where: { id: 'deal_2' } } as any, + ); + + expect(lastSummary(storeFor)).toBe('Deal won by 张伟'); + }); + + it('a milestone template is the author’s sentence and is NOT translated', async () => { + const { engine, storeFor } = await boot({ locale: 'zh-CN', i18n: makeI18n() }); + await seed(engine); + + await moveStage(engine, 'deal_2', 'closed_won'); + + // Real zh-CN bundle loaded (`objects.biz_deal.fields.owner.label` = 负责人) + // and it must NOT leak into the milestone sentence: milestone summaries have + // never been localized, and this card does not change what a template says. + expect(lastSummary(storeFor)).toBe('Deal won by 张伟'); + }); + + it('the generic verb branch still localizes around it (the bundle really is live)', async () => { + const { engine, storeFor } = await boot({ locale: 'zh-CN', i18n: makeI18n() }); + await seed(engine); + + // A write that fires no milestone and moves no tracked field falls through + // to `messages.activityUpdated`. If this stayed English the bundle above + // would be inert and the previous case would prove nothing. + await engine.update('biz_deal', { priority: 'p2' }, { where: { id: 'deal_2' } } as any); + + expect(lastSummary(storeFor)).toBe('已更新商机"Already linked"'); + }); +}); + +// --------------------------------------------------------------------------- +// 3. Read cost — #6656 / PR #6977's retirement must survive this card +// --------------------------------------------------------------------------- + +/** Reads issued on the reference TARGETS during `body`, per object. */ +async function targetReads( + reads: { findOn: Record; findOneOn: Record }, + body: () => Promise, +): Promise> { + const objects = ['sys_user', 'crm_account', 'crm_region', 'crm_vault']; + const before: Record = {}; + for (const o of objects) before[o] = reads.findOn[o] ?? 0; + await body(); + const out: Record = {}; + for (const o of objects) out[o] = (reads.findOn[o] ?? 0) - before[o]; + return out; +} + +const ZERO = { sys_user: 0, crm_account: 0, crm_region: 0, crm_vault: 0 }; + +describe('[#7290] the milestone read is paid only on the transition, and only for what fired', () => { + it('an update of a milestone-declaring object that fires NOTHING pays ZERO reads', async () => { + const { engine, reads, storeFor } = await boot(); + await seed(engine); + + // THE guard for this card. `deal_2` holds every reference field, so a plan + // built BEFORE `matchMilestone` — the placement ruling A+ was obtained to + // remove — would read here, on a write that produces no milestone at all. + const delta = await targetReads(reads, () => moveStage(engine, 'deal_2', 'negotiation')); + + expect(delta).toEqual(ZERO); + expect(lastSummary(storeFor)).toBe('Stage: open → negotiation'); + }); + + it('a non-stage update of a milestone-declaring object pays ZERO reads', async () => { + const { engine, reads } = await boot(); + await seed(engine); + + const delta = await targetReads(reads, () => + engine.update('biz_deal', { priority: 'p2' }, { where: { id: 'deal_2' } } as any), + ); + + expect(delta).toEqual(ZERO); + }); + + it('a create on a milestone-declaring object pays ZERO reads (the branch is update-only)', async () => { + const { engine, reads } = await boot(); + await seed(engine); + + const delta = await targetReads(reads, () => + engine.insert('biz_deal', { + id: 'deal_3', title: 'Third', stage: 'closed_won', + owner: 'usr_1', account_id: 'acc_1', + }), + ); + + expect(delta).toEqual(ZERO); + }); + + it('a delete on a milestone-declaring object pays ZERO reads (the branch is update-only)', async () => { + const { engine, reads } = await boot(); + await seed(engine); + + const delta = await targetReads(reads, () => + engine.delete('biz_deal', { where: { id: 'deal_2' } } as any), + ); + + expect(delta).toEqual(ZERO); + }); + + it('a milestone that fires with NO reference token pays ZERO reads', async () => { + const { engine, reads, storeFor } = await boot(); + await seed(engine); + + // `'✅ Archived: {title}'` — the shipped showcase shape. The row still holds + // every reference; the template names none of them. + const delta = await targetReads(reads, () => moveStage(engine, 'deal_2', 'archived')); + + expect(delta).toEqual(ZERO); + expect(lastSummary(storeFor)).toBe('✅ Archived: Already linked'); + }); + + it('a reference token whose value is EMPTY pays ZERO reads', async () => { + const { engine, reads } = await boot(); + await seed(engine); + + // ⚠️ NOT a placement guard — and deliberately not counted as one. This case + // has to use `deal_1`, the reference-free row, because an empty reference is + // exactly what it measures; on a row with nothing to read, every candidate + // placement pays zero. Measured: this was the ONE case in this file that + // stayed GREEN when the read was moved before `matchMilestone` — the + // placement its neighbours exist to reject. Retained for the empty-value + // rule only. See the note in `seed()` above. + const delta = await targetReads(reads, () => moveStage(engine, 'deal_1', 'unassigned')); + + expect(delta).toEqual(ZERO); + }); + + it('re-firing is impossible, so a repeat write to the same value pays ZERO reads', async () => { + const { engine, reads } = await boot(); + await seed(engine); + await moveStage(engine, 'deal_2', 'closed_won'); + + // The milestone is a TRANSITION (`before !== value && after === value`). + // This is why "one read on the transition" is rare by construction rather + // than "one read per write on a won deal". + const delta = await targetReads(reads, () => + engine.update('biz_deal', { title: 'Renamed' }, { where: { id: 'deal_2' } } as any), + ); + + expect(delta).toEqual(ZERO); + }); + + it('ONE reference token costs exactly ONE read, on its target only', async () => { + const { engine, reads } = await boot(); + await seed(engine); + + const delta = await targetReads(reads, () => moveStage(engine, 'deal_2', 'closed_won')); + + expect(delta).toEqual({ ...ZERO, sys_user: 1 }); + }); + + it('TWO tokens onto the SAME target still cost ONE read (batched, not per token)', async () => { + const { engine, reads, storeFor } = await boot(); + await seed(engine); + + const delta = await targetReads(reads, () => moveStage(engine, 'deal_2', 'handover')); + + // Per-token resolution would measure 2 here. + expect(delta).toEqual({ ...ZERO, crm_account: 1 }); + expect(lastSummary(storeFor)).toBe('Handed to Acme Corp via Globex'); + }); + + it('TWO distinct targets cost exactly ONE read each', async () => { + const { engine, reads } = await boot(); + await seed(engine); + + const delta = await targetReads(reads, () => moveStage(engine, 'deal_2', 'localized')); + + expect(delta).toEqual({ ...ZERO, sys_user: 1, crm_region: 1 }); + }); + + it('the SAME token twice costs ONE read', async () => { + const { engine, reads } = await boot(); + await seed(engine); + + const delta = await targetReads(reads, () => moveStage(engine, 'deal_2', 'escalated')); + + expect(delta).toEqual({ ...ZERO, sys_user: 1 }); + }); + + it('a fired milestone pays for ITS tokens only — not for tracked references that also moved', async () => { + const { engine, reads } = await boot(); + await seed(engine); + + // `account_id` is a tracked reference and it moves, but the milestone wins, + // so #7230's tracked-change branch never runs and never reads. Pre-existing + // precedence, pinned so this card cannot quietly add a second read path. + const delta = await targetReads(reads, () => + engine.update( + 'biz_deal', + { stage: 'closed_won', account_id: 'acc_2' }, + { where: { id: 'deal_2' } } as any, + ), + ); + + expect(delta).toEqual({ ...ZERO, sys_user: 1 }); + }); + + it("re-asserts #6977's own count: a single-id update still pays exactly ONE pre-image read", async () => { + const { engine, reads } = await boot(); + await seed(engine); + + const before = reads.findOneOn['biz_deal'] ?? 0; + await moveStage(engine, 'deal_2', 'closed_won'); + + // The engine's one bound pre-image, and no plugin read of its own on the + // audited object — 2 → 1 stays 1, on the firing path too. + expect((reads.findOneOn['biz_deal'] ?? 0) - before).toBe(1); + }); +}); + +// --------------------------------------------------------------------------- +// 4. Masking — #6656's contract, on the new read path +// --------------------------------------------------------------------------- + +describe('[#7290] a credential title field is never read and never rendered', () => { + it('skips the read entirely when the target designates a masked field as its title', async () => { + const { engine, reads, storeFor } = await boot(); + await seed(engine); + + const delta = await targetReads(reads, () => moveStage(engine, 'deal_2', 'vaulted')); + + // Not "read it and mask it afterwards" — not read at all. `crm_vault` + // declares `nameField: 'api_key'` and `api_key` is `secret`, which is the + // same `collectMaskedReadFields` predicate `ledgerView` masks with. + expect(delta).toEqual(ZERO); + expect(lastSummary(storeFor)).toBe('Vaulted under vlt_1'); + expect(lastSummary(storeFor)).not.toContain('sk-live'); + }); +}); diff --git a/packages/plugins/plugin-audit/src/audit-writers.ts b/packages/plugins/plugin-audit/src/audit-writers.ts index aad1380fca..962516eb3a 100644 --- a/packages/plugins/plugin-audit/src/audit-writers.ts +++ b/packages/plugins/plugin-audit/src/audit-writers.ts @@ -355,10 +355,12 @@ function referenceIdsOf(value: any): string[] { * over the raw stored value. Empty/missing → "∅". * * [#7230] `titlesFor` is the pre-resolved id → title map for THIS field's - * target object (see `resolveTrackedLookupTitles`). It is a lookup into an - * already-batched result, never a read: this function stays synchronous, and - * the callers that have no reference to resolve (`matchMilestone`'s `{token}` - * interpolation) pass nothing and keep today's behaviour exactly. + * target object (see `resolveLookupTitles`). It is a lookup into an + * already-batched result, never a read: this function stays synchronous, and a + * caller with nothing to resolve passes nothing and keeps today's behaviour + * exactly. [#7290] Both summary branches now feed it: the tracked-change diff + * (`renderTrackedChangeSummary`) and the fired-milestone template + * (`renderMilestoneSummary`), each from its own read plan. * * An id with no entry in the map — record deleted, object unregistered, title * field unresolvable — falls back to the raw id, which is what this function @@ -386,7 +388,8 @@ function displayFieldValue(field: any, value: any, titlesFor?: Map 0 ? parts.join('; ') : null; } +/** + * [#7290] The `{token}` pattern of an `activityMilestones` summary template. + * + * A FRESH instance per call, deliberately: a module-level `/g` regex carries + * `lastIndex` between callers, and this one is read twice per fired milestone + * (once to plan the reads, once to render). One definition, no shared cursor. + */ +function milestoneTokenRe(): RegExp { + return /\{(\w+)\}/g; +} + /** * ADR-0052 §5b.2 — declarative semantic milestones. If a watched field * transitioned INTO a configured `value` on this update (before ≠ value, after = - * value), return the interpolated summary (and optional activity type). `{token}` - * in the template is replaced by the record's field value, resolving select - * option labels when possible. Returns null when no milestone fired (needs the - * `before` snapshot to detect a transition). Takes precedence over the raw - * field-change summary. + * value), return the matched summary TEMPLATE (and optional activity type). + * Returns null when no milestone fired (needs the `before` snapshot to detect a + * transition). Takes precedence over the raw field-change summary. + * + * [#7290] Detection only — the `{token}` interpolation moved out to + * {@link renderMilestoneSummary}, with {@link planMilestoneTokenReads} between + * them. Splitting it is what keeps the reference read OFF the write hot path + * and this function synchronous: + * + * match (sync, no I/O) → plan from the MATCHED template's tokens → one read + * per distinct target object → render (sync) + * + * A read placed BEFORE this function would have to speculate on the whole + * after-row, and would therefore fire on every update of a milestone-declaring + * object — including the overwhelming majority where no milestone fires at all. + * That is the shape #6656 / PR #6977's ruling was obtained to remove from this + * file, so the plan is built from what actually matched, or not at all. */ function matchMilestone( objectDef: any, - fields: Record | null, before: Record | null, after: Record | null, -): { summary: string; type?: string } | null { +): { template: string; type?: string } | null { const milestones = objectDef && Array.isArray(objectDef.activityMilestones) ? objectDef.activityMilestones : null; if (!milestones || !after || !before) return null; for (const m of milestones) { if (!m || typeof m.field !== 'string' || typeof m.summary !== 'string') continue; if (after[m.field] === m.value && before[m.field] !== m.value) { - const summary = m.summary.replace(/\{(\w+)\}/g, (_match: string, key: string) => { - const v = after[key]; - if (v === null || v === undefined || v === '') return ''; - const field = fields ? fields[key] : undefined; - return field ? displayFieldValue(field, v) : String(v); - }); - return { summary, type: typeof m.type === 'string' ? m.type : undefined }; + return { template: m.summary, type: typeof m.type === 'string' ? m.type : undefined }; } } return null; } +/** + * [#7290] The read plan for one FIRED milestone template — the referenced ids + * its `{token}`s actually name, grouped by TARGET OBJECT. + * + * Keyed on the matched template rather than on the row: a template naming no + * reference field (`'Task completed: {title}'` — the shipped showcase example) + * produces an empty plan and therefore no read, and a token whose value is + * empty contributes nothing because `referenceIdsOf` yields no ids for it. + * Grouping by target object gives the same batching as + * {@link planTrackedLookupReads}: N reference tokens pointing at one object are + * answered by ONE `id: { $in: [...] }`. + * + * Unlike the tracked-change plan this one does NOT filter on `trackHistory`. + * The token is whatever the milestone author wrote, and `trackHistory` governs + * the field-change summary — a different branch, which a fired milestone + * replaces outright. Requiring it here would silently render raw ids for the + * exact templates authors write (`'Deal won by {owner}'` on an untracked + * `owner`), which is the defect, not a guard against it. + * + * Only the AFTER row is read, because that is the only side the interpolation + * consults — a milestone summary describes the state the record arrived in. + */ +function planMilestoneTokenReads( + template: string, + fields: Record | undefined | null, + after: Record | null, +): Map> { + const byObject = new Map>(); + if (!fields || !after) return byObject; + for (const match of template.matchAll(milestoneTokenRe())) { + const key = match[1]; + const field = fields[key]; + if (!field || typeof field.type !== 'string' || !REFERENCE_FIELD_TYPES.has(field.type)) continue; + const reference = typeof field.reference === 'string' ? field.reference.trim() : ''; + if (!reference) continue; + for (const id of referenceIdsOf(after[key])) { + let set = byObject.get(reference); + if (!set) { set = new Set(); byObject.set(reference, set); } + set.add(id); + } + } + return byObject; +} + +/** + * [#7290] Interpolate a fired milestone's `{token}`s from the after-row. + * + * The author's template wording is theirs and is not touched — only what a + * token RESOLVES TO changes: a `lookup` / `master_detail` / `user` token now + * renders the referenced record's title instead of the raw 32-char id + * (`'Deal won by oBK25…'` → `'Deal won by 张伟'`), using the already-batched + * `lookupTitles` from {@link planMilestoneTokenReads}. Everything else is + * byte-identical to the pre-#7290 behaviour, including the empty-value rule: + * a token whose value is null/undefined/'' renders as the EMPTY STRING here, + * not as `displayFieldValue`'s `∅` — a milestone sentence is prose, not a + * diff row. + * + * Restore-invariant for the same reason `displayFieldValue`'s branch is: an id + * with no resolved title renders exactly as it did before. + */ +function renderMilestoneSummary( + template: string, + fields: Record | undefined | null, + after: Record | null, + lookupTitles?: Map>, +): string { + return template.replace(milestoneTokenRe(), (_match: string, key: string) => { + const v = after ? after[key] : undefined; + if (v === null || v === undefined || v === '') return ''; + const field = fields ? fields[key] : undefined; + if (!field) return String(v); + const reference = typeof field.reference === 'string' ? field.reference : undefined; + const titlesFor = reference ? lookupTitles?.get(reference) : undefined; + return displayFieldValue(field, v, titlesFor); + }); +} + /** * Install audit + activity writers on the given engine. Idempotent per * `packageId` — calling twice with the same id replaces the previous @@ -693,8 +789,16 @@ export function installAuditWriters( }; /** - * [#7230] Referenced record TITLES for the tracked-change summary — id → - * title, per target object. + * [#7230, #7290] Referenced record TITLES for a summary — id → title, per + * target object — resolved from a read PLAN its caller built. + * + * Taking the plan as a PARAMETER rather than building it here is what keeps + * the two summary branches one implementation: `planTrackedLookupReads` (the + * diff, #7230) and `planMilestoneTokenReads` (a fired milestone's tokens, + * #7290) both end here. The title-field resolution, the masking skip below + * and the `['id', titleField]` projection are therefore shared by + * construction — a second copy for the milestone branch would be a second + * de-facto contract, and the mask is precisely where that must not happen. * * ## Why this is shaped as a plan + one read per target object * @@ -710,6 +814,11 @@ export function installAuditWriters( * type actually changed in this write. Every other write — the overwhelming * majority, including every create and delete — pays nothing, and * `#6977`'s counts are untouched. Pinned in `audit-lookup-summary.test.ts`. + * [#7290] `planMilestoneTokenReads` is empty on the same default: it is + * only ever called once a milestone has ALREADY matched, i.e. on the + * transition `before !== value && after === value`, and it is empty again + * whenever that template names no reference token. Pinned in + * `audit-milestone-summary.test.ts`. * - **One read per distinct target object**, never per field and never per * value: N tracked reference fields pointing at one object are answered by * a single `id: { $in: [...] }`. Same batching shape `plugin-approvals` @@ -738,13 +847,10 @@ export function installAuditWriters( * failing driver leaves the id unresolved, and `displayFieldValue` renders the * raw id exactly as it did before this branch existed. */ - const resolveTrackedLookupTitles = async ( + const resolveLookupTitles = async ( api: any, - fields: Record | null, - oldVals: Record | null, - newVals: Record | null, + plan: Map>, ): Promise> | undefined> => { - const plan = planTrackedLookupReads(fields, oldVals, newVals); if (plan.size === 0) return undefined; const sys = api.sudo(); const out = new Map>(); @@ -1060,33 +1166,49 @@ export function installAuditWriters( // ADR-0052 §5b — declarative activity, precedence: a configured semantic // milestone (§5b.2) wins; else a tracked field-change diff ("Stage: // Proposal → Closed Won", §5b.1); else the generic fallback. - // [#6656] Masked views, not the raw pair. `matchMilestone` interpolates - // `{field}` from the after-row straight into a summary the record feed - // and Setup dashboards render verbatim, so it must never see a raw - // credential value. Computed fields are kept (a milestone may key on a + // [#6656] Masked views, not the raw pair. The milestone branch + // interpolates `{field}` from the after-row straight into a summary the + // record feed and Setup dashboards render verbatim, so it must never see + // a raw credential value. Computed fields are kept (a milestone may key on a // formula); detection is unaffected for every other class, since the // mask touches credential fields only — and a milestone whose declared // `value` is a secret's plaintext would be a leak in the metadata // itself, not a case worth preserving. - const milestone = matchMilestone( - getObjectDef(ctx.object), - getFieldDefs(ctx.object), - ledgerView(ctx.object, before, { dropComputed: false }), - ledgerView(ctx.object, after, { dropComputed: false }), - ); + const summaryFields = getFieldDefs(ctx.object); + const beforeView = ledgerView(ctx.object, before, { dropComputed: false }); + const afterView = ledgerView(ctx.object, after, { dropComputed: false }); + const milestone = matchMilestone(getObjectDef(ctx.object), beforeView, afterView); if (milestone) { - summary = milestone.summary; + // [#7290] The read is keyed on the tokens of the template that ACTUALLY + // FIRED, and is reached only on the transition itself — rare by + // construction, since a milestone fires on `before !== value && after + // === value` and never again while the record sits at that value. An + // update of a milestone-declaring object that fires NOTHING pays zero, + // which is what keeps #6656 / PR #6977's retirement in place on this + // branch too; a plan built before the match would have paid on every + // such update, which is the shape that ruling was obtained to remove. + // + // The plan reads the same MASKED after-view the interpolation renders + // from, so a credential field can no more become a read key here than + // it can reach the summary text. + const lookupTitles = await resolveLookupTitles( + api, + planMilestoneTokenReads(milestone.template, summaryFields, afterView), + ); + summary = renderMilestoneSummary(milestone.template, summaryFields, afterView, lookupTitles); if (milestone.type) activityType = milestone.type; } else { // [#7230] The read plan is built from the SAME masked views the summary // renders, and only reached once a milestone has declined — a write // that changes no tracked reference field issues no read at all. - const trackedFields = getFieldDefs(ctx.object); - const lookupTitles = await resolveTrackedLookupTitles(api, trackedFields, oldValue, newValue); + const lookupTitles = await resolveLookupTitles( + api, + planTrackedLookupReads(summaryFields, oldValue, newValue), + ); summary = renderTrackedChangeSummary( ctx.object, - trackedFields, + summaryFields, oldValue, newValue, translate,