diff --git a/.changeset/22387-plugin-approvals-viewer-attached-on-read.md b/.changeset/22387-plugin-approvals-viewer-attached-on-read.md new file mode 100644 index 00000000000..a082786106c --- /dev/null +++ b/.changeset/22387-plugin-approvals-viewer-attached-on-read.md @@ -0,0 +1,11 @@ +--- +"@objectstack/plugin-approvals": patch +--- + +fix(plugin-approvals): `sys_approval_request` declares its per-caller `viewer` block under `attachedOnRead`, so the shared validator accepts the object's own action predicates (#22211, #22387) + +`sys_approval_request` ships eight action `visible` predicates that read `record.viewer.can_act`, `record.viewer.can_override` or `record.viewer.is_submitter`. `viewer` is the block the approvals service attaches to every request it serves through `listRequests` and `getRequest`, computed from the caller. The object never declared it, so `os build` / `os validate` refused all eight with ``unknown field `viewer` on `sys_approval_request` ``, and the object save door's authoring gate refused a save of the shipped body the same way. + +The object now declares `attachedOnRead: { viewer: { can_act: 'boolean', can_override: 'boolean', is_submitter: 'boolean' } }`. The eight predicates pass the build and the save door's gate. A misspelt leaf (`record.viewer.can_actt`) is still refused, and the refusal names the three declared leaves. + +Unchanged: who sees which decision button. `viewer` is computed as before and served on the same two reads only; the generic data door, a record-change flow's `record` and a flow action's subject row do not carry it. The served object definition gains the `attachedOnRead` key. The exported `SysApprovalRequest` type resolves to the same type. diff --git a/packages/plugins/plugin-approvals/package.json b/packages/plugins/plugin-approvals/package.json index cee65a289b5..55b9e9dfb73 100644 --- a/packages/plugins/plugin-approvals/package.json +++ b/packages/plugins/plugin-approvals/package.json @@ -29,6 +29,7 @@ }, "devDependencies": { "@objectstack/driver-sql": "workspace:*", + "@objectstack/lint": "workspace:*", "@objectstack/metadata-protocol": "workspace:*", "@objectstack/objectql": "workspace:*", "@objectstack/service-automation": "workspace:*", diff --git a/packages/plugins/plugin-approvals/src/sys-approval-request-attached-on-read.test.ts b/packages/plugins/plugin-approvals/src/sys-approval-request-attached-on-read.test.ts new file mode 100644 index 00000000000..f44648b6de1 --- /dev/null +++ b/packages/plugins/plugin-approvals/src/sys-approval-request-attached-on-read.test.ts @@ -0,0 +1,125 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#22211 ruling A, #22387] `sys_approval_request`'s own action predicates pass + * the shared validator, because the object declares the block they read. + * + * Eight shipped `visible` predicates gate on `record.viewer.*` — the per-caller + * block `ApprovalService.attachViewers` sets on every row it serves (#3310). + * Until the object declared that block under `attachedOnRead`, the validator + * `os build` / `os validate` run refused all eight as naming an unknown field, + * and so did the object save door once it judged action predicates. + * + * The object is judged here the way those commands judge it: the stack is + * normalized and parsed (`normalizeStackInput`, `ObjectStackDefinitionSchema`), + * then read by `validateStackExpressions` and `runAuthoringRules('build')`; + * the save door is `runRuntimeAuthoringRules({ type: 'object' })`. The rule + * under test is the expression rule (`expression-invalid`); the object's other + * build findings are not this file's question. + * + * Every assertion reads the object's own predicates and its own declaration — + * no predicate or leaf list is copied here. Two controls keep the green honest: + * the same call refuses a misspelt leaf, naming the leaves the object declares, + * and refuses every `viewer` read once the declaration is taken away. + */ + +import { describe, it, expect } from 'vitest'; +import { ObjectStackDefinitionSchema, normalizeStackInput } from '@objectstack/spec'; +import { + EXPRESSION_INVALID, + runAuthoringRules, + runRuntimeAuthoringRules, + validateStackExpressions, +} from '@objectstack/lint'; +import { SysApprovalRequest } from './sys-approval-request.object.js'; + +const MANIFEST = { + id: 'com.objectstack.test.approvals-attached-on-read', + name: 'approvals_attached_on_read', + version: '1.0.0', + type: 'app', +} as const; + +type Action = { name: string; visible?: unknown }; + +/** An action predicate's source text — `ObjectSchema.create` stores the CEL envelope. */ +const sourceOf = (visible: unknown): string => + typeof visible === 'string' ? visible : String((visible as { source?: unknown } | undefined)?.source ?? ''); + +const ACTIONS = ((SysApprovalRequest as { actions?: Action[] }).actions ?? []); +/** The shipped predicates that read the block: the population this file judges. */ +const VIEWER_READERS = ACTIONS.filter((a) => sourceOf(a.visible).includes('record.viewer.')); +const DECLARED_LEAVES = Object.keys(SysApprovalRequest.attachedOnRead?.viewer ?? {}); + +const whereOf = (action: string) => `object 'sys_approval_request' · action '${action}' visible`; + +/** The object, judged by the build pair and by the object save door. */ +function judge(object: Record) { + const normalized = normalizeStackInput({ manifest: MANIFEST, objects: [object] }) as Record; + const result = ObjectStackDefinitionSchema.safeParse(normalized); + if (!result.success) { + throw new Error( + 'fixture is not spec-valid: ' + + result.error.issues.map((i) => `${i.path.join('.') || '(root)'}: ${i.message}`).join('; '), + ); + } + const parsed = result.data as Record; + return { + expressions: validateStackExpressions(parsed), + build: runAuthoringRules('build', { normalized, parsed }).filter((f) => f.rule === EXPRESSION_INVALID), + door: runRuntimeAuthoringRules({ type: 'object', item: object }).errors.filter((f) => f.rule === EXPRESSION_INVALID), + }; +} + +/** The object with one action's predicate rewritten. */ +function withPredicate(action: string, rewrite: (source: string) => string): Record { + return { + ...SysApprovalRequest, + actions: ACTIONS.map((a) => (a.name === action ? { ...a, visible: rewrite(sourceOf(a.visible)) } : a)), + }; +} + +describe('sys_approval_request — its action predicates against its attachedOnRead declaration', () => { + it('the population: shipped predicates read the block, and the object declares its leaves', () => { + expect(VIEWER_READERS.length).toBeGreaterThan(0); + expect(DECLARED_LEAVES.length).toBeGreaterThan(0); + }); + + it('every predicate passes the build pair and the object save door', () => { + const { expressions, build, door } = judge(SysApprovalRequest as Record); + expect(expressions, JSON.stringify(expressions, null, 2)).toEqual([]); + expect(build, JSON.stringify(build, null, 2)).toEqual([]); + expect(door, JSON.stringify(door, null, 2)).toEqual([]); + }); + + it('a misspelt leaf is still refused at all three, naming the leaves the object declares', () => { + const target = VIEWER_READERS[0].name; + const leaf = DECLARED_LEAVES[0]; + const misspelt = `${leaf}_x`; + const { expressions, build, door } = judge( + withPredicate(target, (s) => s.replaceAll(`record.viewer.${leaf}`, `record.viewer.${misspelt}`)), + ); + for (const [surface, found] of [['validateStackExpressions', expressions], ['build', build], ['door', door]] as const) { + expect(found, `${surface}: one refusal`).toHaveLength(1); + const [issue] = found as Array<{ message: string; where?: string; path?: string; severity?: string }>; + expect(issue.severity ?? 'error', surface).toBe('error'); + // The subject is the misspelt path, and the remedy names every declared leaf. + expect(issue.message, surface).toContain(`viewer.${misspelt}`); + for (const declared of DECLARED_LEAVES) expect(issue.message, `${surface} names \`${declared}\``).toContain(`\`${declared}\``); + } + expect(expressions[0].where).toBe(whereOf(target)); + expect(build[0].where).toBe(whereOf(target)); + }); + + it('control: without the declaration, every predicate that reads the block is refused again', () => { + const undeclared = Object.fromEntries( + Object.entries(SysApprovalRequest).filter(([key]) => key !== 'attachedOnRead'), + ); + const { expressions, build, door } = judge(undeclared); + const readers = VIEWER_READERS.map((a) => whereOf(a.name)).sort(); + expect(expressions.map((i) => i.where).sort()).toEqual(readers); + expect(build.map((f) => f.where).sort()).toEqual(readers); + expect(door).toHaveLength(VIEWER_READERS.length); + for (const issue of [...expressions, ...build, ...door]) expect(issue.message).toContain('`viewer`'); + }); +}); diff --git a/packages/plugins/plugin-approvals/src/sys-approval-request-viewer.conformance.test.ts b/packages/plugins/plugin-approvals/src/sys-approval-request-viewer.conformance.test.ts new file mode 100644 index 00000000000..c3dbd865494 --- /dev/null +++ b/packages/plugins/plugin-approvals/src/sys-approval-request-viewer.conformance.test.ts @@ -0,0 +1,219 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#22211 ruling A, #22387] The `viewer` block `ApprovalService` serves is the + * block `sys_approval_request` declares under `attachedOnRead`. + * + * The declaration's second reader (ADR-0049: a declaration has a reader from + * landing day). The shared build validator reads the declared leaf NAMES to + * judge the object's action predicates; nothing there reads the declared leaf + * TYPES. This file reads both: for every caller shape `attachViewers` serves, + * through both doors that call it (`listRequests` and `getRequest`), the keys + * of the served `viewer` equal the declared leaves, and each served value's + * runtime type is the type its leaf declares. + * + * The expected set is read from the object, never written here: a leaf the + * service starts emitting without a declaration, a leaf declared that the + * service never emits, or a value of another type is red with no edit to + * this file. + * + * Driven through the real `ApprovalService` on a real `ObjectQL` over + * `SqlDriver` (better-sqlite3, in-memory, real DDL), with the package's own + * object definitions registered: no engine double stands between the service + * and the rows it decorates. + * + * Each caller shape is held to being the shape it names, by the one leaf that + * marks it (or by none, for the caller who holds no capability), and the + * battery as a whole must observe both values of every boolean leaf, so a + * type check that only ever saw `false` cannot pass for conformance. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import type { ServiceObject } from '@objectstack/spec/data'; +import { ADMIN_FULL_ACCESS } from '@objectstack/spec/identity'; +import { ApprovalService } from './approval-service.js'; +import { ApprovalsServicePlugin } from './approvals-plugin.js'; +import { SysApprovalRequest } from './sys-approval-request.object.js'; + +/** The value types a read-attachment leaf may declare (the spec's closed set). */ +type LeafType = NonNullable[string][string]; + +/** + * The runtime test for each declared value type. Typed against the spec's + * vocabulary, so a fifth type added there fails to compile here until it has + * a test of its own. + */ +const HOLDS: Record boolean> = { + boolean: (v) => typeof v === 'boolean', + number: (v) => typeof v === 'number' && Number.isFinite(v), + text: (v) => typeof v === 'string', + date: (v) => (v instanceof Date && !Number.isNaN(v.getTime())) + || (typeof v === 'string' && !Number.isNaN(Date.parse(v))), +}; + +/** The block under test, read from the object the plugin ships. */ +const DECLARED: Readonly> = SysApprovalRequest.attachedOnRead?.viewer ?? {}; + +const SYSTEM = { isSystem: true, positions: [], permissions: [] } as any; +const asUser = (userId: string, permissions: string[] = []) => + ({ userId, positions: [], permissions }) as any; + +const sysUser = { + name: 'sys_user', + label: 'User', + fields: { + id: { name: 'id', label: 'Id', type: 'text' as const, primaryKey: true }, + name: { name: 'name', label: 'Name', type: 'text' as const }, + email: { name: 'email', label: 'Email', type: 'text' as const }, + }, +}; +const deal = { + name: 'crm_deal', + label: 'Deal', + fields: { + id: { name: 'id', label: 'Id', type: 'text' as const, primaryKey: true }, + title: { name: 'title', label: 'Title', type: 'text' as const }, + }, +}; + +/** The plugin's own object definitions, as its manifest registers them. */ +async function packageObjects(): Promise { + let manifest: any; + const ctx: any = { + getService: (name: string) => { + if (name === 'manifest') return { register: (m: any) => { manifest = m; } }; + throw new Error(`no service '${name}'`); + }, + logger: { info: () => {}, warn: () => {}, error: () => {}, debug: () => {} }, + }; + await new ApprovalsServicePlugin().init(ctx); + return manifest.objects; +} + +/** + * A caller shape `attachViewers` serves: who is reading, which request, and + * the one declared leaf that marks the shape (`null` — the caller who reads + * the request but holds none of the three capabilities). + */ +interface Shape { + label: string; + context: () => any; + request: () => string; + marks: string | null; +} + +describe('sys_approval_request — the served viewer block conforms to attachedOnRead.viewer', () => { + let engine: ObjectQL; + let svc: ApprovalService; + const ids: Record = {}; + let now = Date.parse('2026-10-09T09:00:00.000Z'); + + const SHAPES: Shape[] = [ + { label: 'the submitter, on a pending request', context: () => asUser('u_sub'), request: () => ids.pending, marks: 'is_submitter' }, + { label: 'a current pending approver', context: () => asUser('u_app'), request: () => ids.pending, marks: 'can_act' }, + { + label: 'an override actor (platform admin), on a pending request', + context: () => asUser('u_root', [ADMIN_FULL_ACCESS]), + request: () => ids.pending, + marks: 'can_override', + }, + { label: 'a system context, on a pending request', context: () => SYSTEM, request: () => ids.pending, marks: 'can_override' }, + { label: 'the approver who decided, on the finalized request', context: () => asUser('u_app'), request: () => ids.decided, marks: null }, + ]; + + beforeAll(async () => { + engine = new ObjectQL(); + engine.registerDriver(new SqlDriver({ + client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true, + }), true); + await engine.init(); + for (const def of [sysUser, deal, ...(await packageObjects())]) { + engine.registry.registerObject(def as any, 'approvals-test', 'approvals-test'); + } + await engine.syncSchemas(); + for (const id of ['u_sub', 'u_app', 'u_root']) { + await engine.insert('sys_user', { id, name: id, email: `${id}@example.com` }, { context: SYSTEM } as any); + } + svc = new ApprovalService({ + engine: engine as any, + clock: { now: () => new Date(now += 1000) }, + automation: { + resume: async () => ({ status: 'completed' }), + cancelRun: async () => undefined, + getRun: async (runId: string) => ({ id: runId, status: 'paused' }), + getFlow: async () => ({ name: 'deal_review', nodes: [{ id: 'review', type: 'approval' }], edges: [] }), + } as any, + logger: { warn: () => {}, info: () => {}, error: () => {}, debug: () => {} } as any, + }); + const open = async (key: string) => { + await engine.insert('crm_deal', { id: `D_${key}`, title: key }, { context: SYSTEM } as any); + const row: any = await svc.openNodeRequest({ + object: 'crm_deal', recordId: `D_${key}`, runId: `run_${key}`, nodeId: 'review', flowName: 'deal_review', + config: { approvers: [{ type: 'user', value: 'u_app' }], behavior: 'first_response' } as any, + submitterId: 'u_sub', record: { id: `D_${key}`, title: key }, + }, asUser('u_sub')); + ids[key] = row.id; + }; + await open('pending'); + await open('decided'); + await svc.decideNode(ids.decided, { decision: 'approve' } as any, asUser('u_app')); + }); + + afterAll(async () => { + try { await engine?.destroy(); } catch { /* noop */ } + }); + + it('the object declares the block this battery reads', () => { + // The battery below compares against DECLARED; an empty declaration would + // make every key comparison trivially about nothing. + expect(Object.keys(DECLARED).length).toBeGreaterThan(0); + }); + + /** Both doors that attach the block, for one caller shape. */ + async function served(shape: Shape): Promise> { + const id = shape.request(); + const one = await svc.getRequest(id, shape.context()); + const listed = (await svc.listRequests({ object: 'crm_deal' }, shape.context())).find((r) => r.id === id); + expect(one, `${shape.label}: getRequest serves the request`).not.toBeNull(); + expect(listed, `${shape.label}: listRequests serves the request`).toBeDefined(); + return [ + { door: 'getRequest', viewer: (one as any)?.viewer }, + { door: 'listRequests', viewer: (listed as any)?.viewer }, + ]; + } + + it.each(SHAPES)('$label: the served keys are the declared leaves, each value of its declared type', async (shape) => { + for (const { door, viewer } of await served(shape)) { + const where = `${shape.label} via ${door}`; + expect(viewer, `${where}: a viewer block is attached`).toBeTypeOf('object'); + const block = viewer as Record; + expect(Object.keys(block).sort(), `${where}: emitted keys`).toEqual(Object.keys(DECLARED).sort()); + for (const [leaf, type] of Object.entries(DECLARED)) { + expect(HOLDS[type](block[leaf]), `${where}: \`${leaf}\` declared ${type}, served ${JSON.stringify(block[leaf])}`) + .toBe(true); + } + // The shape is the shape it names: its marking leaf is the only true one. + for (const leaf of Object.keys(DECLARED)) { + expect(block[leaf], `${where}: \`${leaf}\``).toBe(leaf === shape.marks); + } + } + }); + + it('the battery observes both values of every boolean leaf (the type check is not vacuous)', async () => { + const seen = new Map>(); + for (const shape of SHAPES) { + for (const { viewer } of await served(shape)) { + for (const [leaf, value] of Object.entries(viewer as Record)) { + if (!seen.has(leaf)) seen.set(leaf, new Set()); + seen.get(leaf)!.add(value); + } + } + } + for (const [leaf, type] of Object.entries(DECLARED)) { + if (type !== 'boolean') continue; + expect([...(seen.get(leaf) ?? [])].sort(), `\`${leaf}\` across the battery`).toEqual([false, true]); + } + }); +}); diff --git a/packages/plugins/plugin-approvals/src/sys-approval-request.object.ts b/packages/plugins/plugin-approvals/src/sys-approval-request.object.ts index 1cd4e332f9d..c4b45c28b39 100644 --- a/packages/plugins/plugin-approvals/src/sys-approval-request.object.ts +++ b/packages/plugins/plugin-approvals/src/sys-approval-request.object.ts @@ -365,6 +365,25 @@ export const SysApprovalRequest = ObjectSchema.create({ updated_at: Field.datetime({ label: 'Updated At', required: false, group: 'System' }), }, + // [#22211 ruling A] The per-caller `viewer` block (#3310), declared as a read + // attachment: `ApprovalService.attachViewers` computes it from the CALLER's + // identity and sets it on every row `listRequests` / `getRequest` serve. It + // is not a field — no column, and never stored — so the rows the data engine + // itself returns (the generic data door, a record-change flow's `record`, a + // flow action's subject load) never carry it. + // + // Two readers hold this declaration to the truth: + // - the shared build validator judges the `visible` predicates below + // against it: `record.viewer` resolves, and `record.viewer.` only + // to a leaf declared here (`sys-approval-request-attached-on-read.test.ts`); + // - the conformance test pins that the keys `attachViewers` emits, and the + // runtime type of each value, equal this block, for every caller shape it + // serves (`sys-approval-request-viewer.conformance.test.ts`). A leaf added + // to the service without this block, or here without the service, is red. + attachedOnRead: { + viewer: { can_act: 'boolean', can_override: 'boolean', is_submitter: 'boolean' }, + }, + indexes: [ // Look up "is there a pending request for this record?" — common // guard on submit and on edit-while-locked checks. @@ -398,7 +417,8 @@ export const SysApprovalRequest = ObjectSchema.create({ // (the lever that releases the record without recording a decision nobody // made). `viewer` is // attached by getRequest/listRequests; where it is absent the predicate fails - // closed. + // closed. Its leaves are declared under `attachedOnRead` above, which is what + // the build validator judges these predicates against. // // Every predicate below is guarded for the SPARSE action face (#8990). This // binding is a list row or a record read carrying only what the caller diff --git a/packages/plugins/plugin-approvals/vitest.config.ts b/packages/plugins/plugin-approvals/vitest.config.ts index 213f2997bc5..3276ecb5927 100644 --- a/packages/plugins/plugin-approvals/vitest.config.ts +++ b/packages/plugins/plugin-approvals/vitest.config.ts @@ -30,6 +30,17 @@ export default defineConfig({ find: /^@objectstack\/metadata-protocol$/, replacement: path.resolve(__dirname, '../../metadata-protocol/src/index.ts'), }, + // [#22387] `sys-approval-request-attached-on-read.test.ts` runs this + // package's shipped action predicates through the shared validator + // (`validateStackExpressions`, `runAuthoringRules('build')`, the object + // save door), so the verdict must be the CURRENT rule's, not a `lint/dist` + // build's — same #7668/#7778 reason as the two entries above. Anchored: + // `@objectstack/lint` also exports `./runtime` and `./rule-explanations`, + // which a prefix match would resolve through this FILE (`ENOTDIR`). + { + find: /^@objectstack\/lint$/, + replacement: path.resolve(__dirname, '../../lint/src/index.ts'), + }, ], }, test: { diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index ec05c06f590..4791dd10e10 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1597,6 +1597,9 @@ importers: '@objectstack/driver-sql': specifier: workspace:* version: link:../../drivers/driver-sql + '@objectstack/lint': + specifier: workspace:* + version: link:../../lint '@objectstack/metadata-protocol': specifier: workspace:* version: link:../../metadata-protocol