From 34bd70115c70a69b3c57e3c8ed43494bc45be9ea Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 07:13:42 +0000 Subject: [PATCH 1/3] feat(spec)+fix(approvals): publish approver value sources, order the type enum for authors, surface dead approver slots (#3508 / #3807) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four follow-ups from browser-verifying the #3508 approver work end to end. 1. `APPROVER_VALUE_SOURCES` (spec) — `xRef.map` named a picker KIND but never where its rows come from, so the designer kept its own copy of the data contract and the first copy pointed every directory kind at the metadata REGISTRY, which cannot list `sys_user` / `sys_team` / `sys_business_unit` / `sys_position` rows (#3508). The binding now ships on the published JSON schema as `xRef.sources`, derived from `APPROVER_VALUE_BINDINGS` so the two cannot drift and a new `ApproverType` with no declared source stays a compile error. Presentation stays a renderer decision. 2. `ApproverType` order (spec) — the Studio picker derives from this enum via the published schema, not from objectui's own options array, so the indirect-bindings-first order objectui#2834 argued for never took effect. The enum now carries it: manager, position, department, team, field, expression, org_membership_level, user. Deprecated `role`/`queue` still parse and stay out of every picker via `xEnumDeprecated`. 3. Dead approver slots (plugin-approvals) — `queue` already warned; every other graph type (`team`, `department`, `position`, `org_membership_level`, `manager`) fell back to the same unactionable `type:value` literal in total silence. That is what let #3807 hide: an empty slate, no log line, and a permanently stuck approval as the first symptom (#3424). The fallback stays; it now names the type, value and organization that produced it. `user` and `field` stay quiet — they never had an "expanded to nobody" state. 4. plugin-sharing's identical org scope (tests only) — `BusinessUnitGraphService` .orgScope carries the same strict equality #3807 fixed in approvals. It is unreachable today (every materialized `sys_sharing_rule` is null-org, so the filter is skipped) and it is an authorization path, so it is pinned by tests rather than widened blind: the reachable paths and the divergence itself are now executable facts. Tests: spec 6742 pass (+6 new: sources exhaustive + on the wire, enum order, non-authorable spellings); plugin-approvals 258 pass (+6: each graph type warns with type/value/org, resolvable expansions and `user` stay quiet); plugin-sharing 107 pass (+6 new BU-graph cases); lint 471 pass; repo lint clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BVRVgSvmyCwmDfTmKmCZoH --- ...ver-value-sources-and-dead-slot-warning.md | 53 ++++++ .../src/approval-service.test.ts | 60 +++++++ .../plugin-approvals/src/approval-service.ts | 30 ++++ .../src/business-unit-graph.test.ts | 162 ++++++++++++++++++ packages/spec/src/automation/approval.test.ts | 61 +++++++ packages/spec/src/automation/approval.zod.ts | 98 +++++++++-- 6 files changed, 446 insertions(+), 18 deletions(-) create mode 100644 .changeset/approver-value-sources-and-dead-slot-warning.md create mode 100644 packages/plugins/plugin-sharing/src/business-unit-graph.test.ts diff --git a/.changeset/approver-value-sources-and-dead-slot-warning.md b/.changeset/approver-value-sources-and-dead-slot-warning.md new file mode 100644 index 0000000000..920f2a061f --- /dev/null +++ b/.changeset/approver-value-sources-and-dead-slot-warning.md @@ -0,0 +1,53 @@ +--- +"@objectstack/spec": minor +"@objectstack/plugin-approvals": patch +--- + +feat(spec)+fix(approvals): publish approver value data sources, order the type enum for authors, stop silent dead approver slots (#3508 / #3807 follow-ups) + +Four follow-ups from browser-verifying the #3508 approver work end to end. + +**`APPROVER_VALUE_SOURCES` — the designer stops guessing where candidates live.** +`xRef.map` only ever named a picker KIND (`'team'`), never where that picker's +rows come from, so the designer carried its own copy of the data contract — and +the first copy was wrong: every directory kind was wired to `GET +/api/v1/meta/:type`, the metadata REGISTRY, which does not hold `sys_user` / +`sys_team` / `sys_business_unit` / `sys_position` rows. Candidates came back +empty and the control degraded to free text (#3508). The binding is now +projected onto the published JSON schema as `xRef.sources` — `{ source: 'data', +object, valueField }` for the record-backed kinds, the closed enum inline for +`org_membership_level` — derived from `APPROVER_VALUE_BINDINGS` so the two +cannot drift, and inheriting its `satisfies` exhaustiveness (a new +`ApproverType` member that declares no source is a compile error). Presentation +— which field to show, whether to open a people-picker, what subtitle to use — +stays a renderer decision. + +**`ApproverType` declaration order is now the authoring recommendation.** +objectui#2834 argued for leading with indirect bindings and shipped that order +in its own options array — which the Studio inspector never reads: it derives +the picker from this enum via the published schema, so `user` still came first. +The intent only takes effect if the enum carries it, so the enum now reads +`manager, position, department, team, field, expression, org_membership_level, +user` (deprecated `role` / `queue` still parse and stay out of every picker via +`xEnumDeprecated`). Binding one specific person is the least portable choice an +author can make — it breaks when the flow moves to another environment (that id +does not exist there) and again when that person leaves. + +**A graph approver that expands to nobody no longer does it in silence.** +`queue` already warned (#3508); every OTHER graph type — `team`, `department`, +`position`, `org_membership_level`, `manager` — fell back to the same +unactionable `type:value` literal without a word. That silence is what let +#3807 hide for as long as it did: the request opened with an empty slate and +the first symptom was a permanently stuck approval (#3424). The fallback stays +(15.x slots and substring fixtures depend on it); it now logs the type, value +and organization that produced it. `user` / `field` stay quiet — they take the +id they were given and never had an "expanded to nobody" state. + +**`plugin-sharing`'s identical org scope is pinned by tests.** +`BusinessUnitGraphService.orgScope` has the same strict `organization_id` +equality #3807 fixed in approvals. It is unreachable today — every materialized +`sys_sharing_rule` carries `organization_id = null`, so the filter is skipped — +and widening an authorization path on a defect that cannot currently fire is +not a change to make blind. New tests lock both the reachable paths and the +divergence itself, so if sharing ever adopts the null-org=env-wide reading it +is a deliberate edit to a named test rather than a silent behaviour change. diff --git a/packages/plugins/plugin-approvals/src/approval-service.test.ts b/packages/plugins/plugin-approvals/src/approval-service.test.ts index 0523b0a39e..a0dc124d76 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.test.ts @@ -2331,6 +2331,66 @@ describe('ApprovalService — queue approver is unresolved (#3508)', () => { }); }); +// #3807 follow-up: `queue` was not the only way to end up with a slot nobody +// can act on — every GRAPH approver type falls back to the same literal when +// its lookup finds no one, and that fallback used to happen in total silence. +// A stuck approval was the first symptom; the log said nothing. The literal +// stays (15.x slots and substring fixtures depend on it) — it just announces +// itself now. +describe('ApprovalService — a graph approver that expands to nobody warns (#3807)', () => { + const svcWithWarnings = (engine: any) => { + const warnings: any[] = []; + let n = 0; + const svc = new ApprovalService({ + engine, + clock: { now: () => new Date(1757000000000 + (n++) * 1000) }, + logger: { warn: (msg: any, meta: any) => warnings.push([msg, meta]) }, + }); + return { svc, warnings }; + }; + + const approverInput = (type: string, value: string) => ({ + ...openInput([]), + config: { approvers: [{ type, value }], behavior: 'first_response' as const, lockRecord: false }, + }); + + it.each([ + ['team', 'team_gone'], + ['department', 'bu_gone'], + ['position', 'nobody_holds_this'], + ['org_membership_level', 'member'], + ])('%s: the dead literal is logged with its type, value and org', async (type, value) => { + const engine = makeFakeEngine(); + const { svc, warnings } = svcWithWarnings(engine); + const req = await svc.openNodeRequest(approverInput(type, value), CTX); + + expect(req.pending_approvers).toEqual([`${type}:${value}`]); + const hit = warnings.find(([msg]) => String(msg).includes('expanded to nobody')); + expect(hit, `no warning for ${type}`).toBeTruthy(); + expect(String(hit[0])).toContain('#3807'); + expect(hit[1]).toMatchObject({ type, value, organizationId: 't1' }); + }); + + it('stays quiet when the graph DOES resolve someone', async () => { + const engine = makeFakeEngine(); + engine._tables['sys_team_member'] = [{ id: 'tm1', team_id: 'team_ok', user_id: 'u5' }]; + const { svc, warnings } = svcWithWarnings(engine); + const req = await svc.openNodeRequest(approverInput('team', 'team_ok'), CTX); + + expect(req.pending_approvers).toEqual(['u5']); + expect(warnings.filter(([msg]) => String(msg).includes('expanded to nobody'))).toEqual([]); + }); + + it('stays quiet for `user` — a literal id was never a lookup that could come back empty', async () => { + const engine = makeFakeEngine(); + const { svc, warnings } = svcWithWarnings(engine); + const req = await svc.openNodeRequest(approverInput('user', 'u_unknown'), CTX); + + expect(req.pending_approvers).toEqual(['u_unknown']); + expect(warnings.filter(([msg]) => String(msg).includes('expanded to nobody'))).toEqual([]); + }); +}); + // ── File-access delegate (ADR-0104 D3 wave 2) ──────────────────────── // // A decision attachment is OWNED by its `sys_approval_action` row, so the diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index 1ca3ebde7d..dc591e072d 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -177,6 +177,21 @@ function actingUserId(context: SharingExecutionContext | undefined): string | nu */ const OOO_MAX_CHAIN = 8; +/** + * Approver types resolved by QUERYING a graph rather than by taking `value` + * literally (#3807). Each can legitimately come back empty — an unstaffed + * position, an emptied team, a mis-pointed unit — and the caller then falls + * back to a `type:value` literal that no user can act on. They are listed here + * so that dead end gets one warning instead of passing in silence. + * + * `user` / `field` are deliberately absent: they resolve to the id they were + * given without a lookup, so there is no "expanded to nobody" state to report. + * `business_unit` / `bu` are the accepted dialects of `department`. + */ +const GRAPH_APPROVER_TYPES: ReadonlySet = new Set([ + 'team', 'department', 'business_unit', 'bu', 'position', 'org_membership_level', 'manager', +]); + /** One OOO delegation hop applied while resolving an approver (#1322 M1/M4). */ interface OooSubstitution { /** The approver who was skipped (out of office). */ @@ -757,6 +772,21 @@ export class ApprovalService implements IApprovalService { `[approvals] approver type 'queue' is not implemented — the slot resolves to nobody (#3508)`, { value: a.value }, ); + } else if (GRAPH_APPROVER_TYPES.has(type)) { + // #3807 follow-up — every OTHER way to land here is a graph type whose + // lookup produced nobody, and the literal below is a slot no user can + // ever act on. That silence is what let #3807 hide: a `department` + // approver pointing at a seeded (env-wide) unit resolved to + // `department:` on every request, the request opened with an empty + // slate, and nothing in the logs said so — the first symptom was a + // permanently stuck approval (#3424). The fallback itself stays (a + // literal keeps 15.x slots and substring fixtures working); it just + // stops being invisible. + this.logger?.warn?.( + `[approvals] approver '${type}:${a.value}' expanded to nobody — the slot routes to no one ` + + `and the request cannot advance until someone is added or the approver is re-pointed (#3807)`, + { type, value: a.value, organizationId: organizationId ?? null }, + ); } return [`${a.type}:${a.value}`]; } diff --git a/packages/plugins/plugin-sharing/src/business-unit-graph.test.ts b/packages/plugins/plugin-sharing/src/business-unit-graph.test.ts new file mode 100644 index 0000000000..d61546128c --- /dev/null +++ b/packages/plugins/plugin-sharing/src/business-unit-graph.test.ts @@ -0,0 +1,162 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * BusinessUnitGraphService — org scoping of the unit tree. + * + * These pin the ORG-SCOPE behaviour specifically, because it is the exact + * shape that broke approvals in #3807: `orgScope()` AND-composes a strict + * `organization_id = ` equality, so a unit written with no + * organization at all (a seeded / file-layer / bootstrap row — a seed cannot + * know the org id the runtime mints at boot) matches nothing, the seed check + * fails, and the expansion returns zero members. In approvals that produced a + * dead `department:` approver slot; here it would produce a sharing rule + * that silently grants nobody. + * + * It is NOT reachable today: every materialized `sys_sharing_rule` row carries + * `organization_id = null` (verified on a live showcase stack), so + * `expandRecipient` passes `null` and `orgScope` is skipped entirely. The + * moment rules start carrying an org — a multi-tenant deployment — a BU + * subtree rule against a seeded unit stops granting, and the symptom is + * "the right people cannot see the record", which is far quieter than a stuck + * approval. + * + * So this file locks BOTH sides down: + * - the reachable paths (null-org rule) keep working, and + * - the divergence from approvals is written down as an executable fact + * rather than a comment, so flipping it is a deliberate edit to a named + * test and never a silent behaviour change. + * + * If the platform decides null-org means "env-wide, visible to every org" for + * sharing too — the way `plugin-approvals` and `sys_metadata` already read it — + * the test named `[divergence]` below is the one to flip, and `orgScope` grows + * the same `$or: [{ organization_id }, { organization_id: null }]` predicate. + */ + +import { describe, it, expect } from 'vitest'; +import { BusinessUnitGraphService } from './business-unit-graph.js'; + +interface UnitRow { + id: string; + parent_business_unit_id?: string | null; + organization_id?: string | null; + active?: boolean; +} +interface MemberRow { business_unit_id: string; user_id: string } + +/** + * Minimal engine over `sys_business_unit` + `sys_business_unit_member`. + * Mirrors the real filter surface the service uses: plain equality, `$ne`, + * `$in`, and `$or` (so a widened predicate would actually be exercised here + * rather than silently ignored by the stub). + */ +function makeEngine(units: UnitRow[], members: MemberRow[]) { + const matches = (row: any, filter: any): boolean => { + if (!filter || typeof filter !== 'object') return true; + for (const [k, v] of Object.entries(filter)) { + if (k === '$or') { + if (!(v as any[]).some((sub) => matches(row, sub))) return false; + continue; + } + const rv = row[k]; + if (v && typeof v === 'object' && '$ne' in (v as any)) { + if (rv === (v as any).$ne) return false; + continue; + } + if (v && typeof v === 'object' && '$in' in (v as any)) { + if (!(v as any).$in.includes(rv)) return false; + continue; + } + // `organization_id: null` must also match a row that simply omits the + // column — that is what a NULL column reads back as. + if (v === null) { + if (rv != null) return false; + continue; + } + if (rv !== v) return false; + } + return true; + }; + + return { + async find(object: string, options: any) { + const filter = options?.where ?? options?.filter ?? {}; + if (object === 'sys_business_unit') return units.filter((u) => matches(u, filter)); + if (object === 'sys_business_unit_member') return members.filter((m) => matches(m, filter)); + return []; + }, + } as any; +} + +/** A seeded org tree: rows written before any organization existed. */ +const SEEDED_UNITS: UnitRow[] = [ + { id: 'bu_root', organization_id: null, active: true }, + { id: 'bu_child', parent_business_unit_id: 'bu_root', organization_id: null, active: true }, +]; +const SEEDED_MEMBERS: MemberRow[] = [ + { business_unit_id: 'bu_root', user_id: 'u_root' }, + { business_unit_id: 'bu_child', user_id: 'u_child' }, +]; + +describe('BusinessUnitGraphService — subtree expansion', () => { + it('expands the unit and every descendant', async () => { + const g = new BusinessUnitGraphService({ engine: makeEngine(SEEDED_UNITS, SEEDED_MEMBERS) }); + expect((await g.expandUsers('bu_root')).sort()).toEqual(['u_child', 'u_root']); + }); + + it('an inactive unit contributes nobody and stops the descent', async () => { + const units: UnitRow[] = [ + { id: 'bu_root', organization_id: null, active: false }, + { id: 'bu_child', parent_business_unit_id: 'bu_root', organization_id: null, active: true }, + ]; + const g = new BusinessUnitGraphService({ engine: makeEngine(units, SEEDED_MEMBERS) }); + expect(await g.expandUsers('bu_root')).toEqual([]); + }); +}); + +describe('BusinessUnitGraphService — org scoping (#3807)', () => { + it('an org-less rule (today’s materialized shape) expands seeded units fine', async () => { + // `expandRecipient` passes `rule.organization_id ?? null`, and every + // materialized sharing rule carries null — so `orgScope` adds nothing and + // the seeded tree resolves. This is the only path that runs in practice. + const g = new BusinessUnitGraphService({ + engine: makeEngine(SEEDED_UNITS, SEEDED_MEMBERS), + organizationId: null, + }); + expect((await g.expandUsers('bu_root')).sort()).toEqual(['u_child', 'u_root']); + }); + + it('an org-scoped rule expands units belonging to that org', async () => { + const units: UnitRow[] = [ + { id: 'bu_root', organization_id: 'org_a', active: true }, + { id: 'bu_child', parent_business_unit_id: 'bu_root', organization_id: 'org_a', active: true }, + ]; + const g = new BusinessUnitGraphService({ + engine: makeEngine(units, SEEDED_MEMBERS), + organizationId: 'org_a', + }); + expect((await g.expandUsers('bu_root')).sort()).toEqual(['u_child', 'u_root']); + }); + + it('an org-scoped rule never reaches another org’s unit', async () => { + const units: UnitRow[] = [{ id: 'bu_root', organization_id: 'org_b', active: true }]; + const g = new BusinessUnitGraphService({ + engine: makeEngine(units, SEEDED_MEMBERS), + organizationId: 'org_a', + }); + expect(await g.expandUsers('bu_root')).toEqual([]); + }); + + it('[divergence] an org-scoped rule does NOT see an env-wide (null-org) unit — approvals does (#3807)', async () => { + // Same inputs that #3807 fixed on the approvals side. Sharing still reads + // a null-org unit as "belongs to no org, therefore not mine" and grants + // nobody. Unreachable today (rules are null-org), deliberate until the + // platform rules on null-org semantics for AUTHORIZATION paths — widening + // who can SEE a record is not a change to make on a defect that cannot + // currently fire. + const g = new BusinessUnitGraphService({ + engine: makeEngine(SEEDED_UNITS, SEEDED_MEMBERS), + organizationId: 'org_a', + }); + expect(await g.expandUsers('bu_root')).toEqual([]); + }); +}); diff --git a/packages/spec/src/automation/approval.test.ts b/packages/spec/src/automation/approval.test.ts index d6111ea496..618b0edce3 100644 --- a/packages/spec/src/automation/approval.test.ts +++ b/packages/spec/src/automation/approval.test.ts @@ -4,6 +4,7 @@ import { DEPRECATED_APPROVER_TYPES, NON_AUTHORABLE_APPROVER_TYPES, APPROVER_VALUE_BINDINGS, + APPROVER_VALUE_SOURCES, ORG_MEMBERSHIP_LEVELS, canonicalApproverType, APPROVAL_NODE_TYPE, @@ -116,6 +117,66 @@ describe('APPROVER_VALUE_BINDINGS (#3508)', () => { }); }); +// #3508 follow-up: `xRef.map` names a picker KIND but never said where that +// picker's candidates live — which is how the designer came to query the +// metadata registry for data records. The data contract now ships on the wire. +describe('APPROVER_VALUE_SOURCES (#3508 follow-up)', () => { + it('covers every ApproverType member, exactly like the bindings it projects', () => { + for (const t of ApproverType.options) { + expect(APPROVER_VALUE_SOURCES[t], `no source published for '${t}'`).toBeDefined(); + expect(APPROVER_VALUE_SOURCES[t].source).toBe( + APPROVER_VALUE_BINDINGS[t].source === 'record' ? 'data' : APPROVER_VALUE_BINDINGS[t].source, + ); + } + }); + + it('routes the directory kinds to the DATA API, naming the object and the committed column', () => { + // `data`, not `meta`: these are rows in system objects, and the metadata + // registry (`GET /api/v1/meta/:type`) cannot list them (#3508). + expect(APPROVER_VALUE_SOURCES.user).toEqual({ source: 'data', object: 'sys_user', valueField: 'id' }); + expect(APPROVER_VALUE_SOURCES.team).toEqual({ source: 'data', object: 'sys_team', valueField: 'id' }); + expect(APPROVER_VALUE_SOURCES.department).toEqual({ source: 'data', object: 'sys_business_unit', valueField: 'id' }); + // The one kind that commits something other than the primary key. + expect(APPROVER_VALUE_SOURCES.position).toEqual({ source: 'data', object: 'sys_position', valueField: 'name' }); + }); + + it('keeps the non-record kinds off the lookup path and carries the closed enum inline', () => { + expect(APPROVER_VALUE_SOURCES.org_membership_level).toEqual({ source: 'enum', values: [...ORG_MEMBERSHIP_LEVELS] }); + expect(APPROVER_VALUE_SOURCES.role).toEqual(APPROVER_VALUE_SOURCES.org_membership_level); + expect(APPROVER_VALUE_SOURCES.manager).toEqual({ source: 'auto' }); + expect(APPROVER_VALUE_SOURCES.field).toEqual({ source: 'trigger-field' }); + expect(APPROVER_VALUE_SOURCES.expression).toEqual({ source: 'expression' }); + expect(APPROVER_VALUE_SOURCES.queue).toEqual({ source: 'unsupported' }); + }); + + it('ships on the published JSON schema, where the designer reads it', () => { + const schema = getApprovalNodeConfigJsonSchema() as any; + const sources = schema?.properties?.approvers?.items?.properties?.value?.xRef?.sources; + expect(sources).toBeDefined(); + expect(sources.department).toEqual({ source: 'data', object: 'sys_business_unit', valueField: 'id' }); + expect(sources.position).toEqual({ source: 'data', object: 'sys_position', valueField: 'name' }); + }); +}); + +// objectui#2834 argued for leading with indirect bindings, but shipped the +// order in its own options array — which the Studio inspector never reads (it +// derives the picker from this enum via the published JSON schema). The intent +// only takes effect if the enum itself carries it, so pin it here. +describe('ApproverType declaration order is the authoring recommendation', () => { + const authorable = ApproverType.options.filter((t) => !NON_AUTHORABLE_APPROVER_TYPES.includes(t)); + + it('leads with indirect bindings and puts the literal `user` last', () => { + expect(authorable).toEqual([ + 'manager', 'position', 'department', 'team', 'field', 'expression', 'org_membership_level', 'user', + ]); + }); + + it('never offers a non-authorable spelling, wherever it sits in the enum', () => { + expect(authorable).not.toContain('role'); + expect(authorable).not.toContain('queue'); + }); +}); + describe('Approval node constants (ADR-0019)', () => { it('exposes the canonical node type and decision branch labels', () => { expect(APPROVAL_NODE_TYPE).toBe('approval'); diff --git a/packages/spec/src/automation/approval.zod.ts b/packages/spec/src/automation/approval.zod.ts index 09a876de3c..c36b56669c 100644 --- a/packages/spec/src/automation/approval.zod.ts +++ b/packages/spec/src/automation/approval.zod.ts @@ -5,9 +5,38 @@ import { lazySchema } from '../shared/lazy-schema'; /** * Approval Step Approver Type + * + * **Declaration order is author-facing.** A designer that derives its + * approver-type picker from this enum (the Studio flow inspector does, via the + * published JSON schema minus `xEnumDeprecated`) offers the members in exactly + * this order, so the order is the platform's recommendation, not an accident of + * when each member was added. + * + * INDIRECT bindings lead; the literal `user` binding comes last. Naming one + * specific person is the least portable choice an author can make — it breaks + * when the flow is deployed to another environment (that id does not exist + * there) and again when that person changes team or leaves, silently routing + * approvals to someone who should no longer see them. `manager` / `position` / + * `department` / `team` all survive both. Offering `user` first taught the + * opposite (objectui#2834). */ export const ApproverType = z.enum([ - 'user', // Specific user(s) + 'manager', // Submitter's manager (sys_user.manager_id) + 'position', // Holders of a position (sys_user_position, ADR-0090 D3) + 'department', // Members of a department + all descendant departments (sys_business_unit) + 'team', // Members of a flat collaboration team (sys_team) + 'field', // User ID defined in a record field + /** + * #3447 P2: a CEL expression resolved AT NODE ENTRY against three explicit + * roots — `current.*` (the record's live state), `trigger.*` (the submit-time + * snapshot) and `vars.*` (flow variables, incl. upstream node outputs). The + * result (a user-id string, CSV, or string array — or intermediate ids + * re-expanded per `resolveAs`) becomes the approver slate. `record` and bare + * field names are deliberately NOT available: `record` means "the record at + * event time" everywhere else on the platform (flow conditions, hooks), and + * reusing it here would silently alias one of the two times. + */ + 'expression', // The better-auth ORG-MEMBERSHIP TIER (sys_member.role: owner / admin / // member), spelled with the projection name ADR-0057 D7 mandates and // ADR-0090 D3 assumes ("relabelled `org_membership_level` … its UI label is @@ -19,13 +48,11 @@ export const ApproverType = z.enum([ // surfaces; its exception covers better-auth's own `sys_member.role` column, // NOT this enum (which is ours). Accepted for one deprecation window: the // runtime resolves it identically and warns, `os lint` prescribes the - // rewrite. Removed in the next major. + // rewrite. Removed in the next major. Kept adjacent to the spelling that + // replaced it; `xEnumDeprecated` keeps it out of every picker regardless of + // where it sits. 'role', - 'position', // Holders of a position (sys_user_position, ADR-0090 D3) - 'team', // Members of a flat collaboration team (sys_team) - 'department', // Members of a department + all descendant departments (sys_business_unit) - 'manager', // Submitter's manager (sys_user.manager_id) - 'field', // User ID defined in a record field + 'user', // A specific user id — least portable; see the note above // @deprecated #3508 — declared-but-unenforced: `resolveApproverSpec` in // `plugin-approvals` has no queue branch, so a queue approver resolves to // nobody (the dead `queue:` fallback literal). The sharing engine @@ -34,17 +61,6 @@ export const ApproverType = z.enum([ // (see NON_AUTHORABLE_APPROVER_TYPES). Re-admit only together with a real // ownership-queue implementation (queue entity + membership + claim). 'queue', - /** - * #3447 P2: a CEL expression resolved AT NODE ENTRY against three explicit - * roots — `current.*` (the record's live state), `trigger.*` (the submit-time - * snapshot) and `vars.*` (flow variables, incl. upstream node outputs). The - * result (a user-id string, CSV, or string array — or intermediate ids - * re-expanded per `resolveAs`) becomes the approver slate. `record` and bare - * field names are deliberately NOT available: `record` means "the record at - * event time" everywhere else on the platform (flow conditions, hooks), and - * reusing it here would silently alias one of the two times. - */ - 'expression' ]); /** @@ -141,6 +157,47 @@ export const APPROVER_VALUE_BINDINGS = { expression: { source: 'expression', roots: APPROVER_EXPRESSION_ROOTS }, } as const satisfies Record, ApproverValueBinding>; +/** + * {@link APPROVER_VALUE_BINDINGS}, projected onto the wire for the designer + * (#3508 follow-up). + * + * `xRef.map` only ever said which PICKER KIND to render — a name like `'team'` + * — and never where that picker's candidates come from. So the designer had to + * carry its own copy of the data contract, and the first copy was wrong: every + * directory kind was wired to `GET /api/v1/meta/:type`, the metadata registry, + * which does not hold `sys_user` / `sys_team` / `sys_business_unit` / + * `sys_position` ROWS. Candidates came back empty and the control degraded to a + * free-text box (#3508). + * + * Publishing the binding closes that gap at the source: a renderer reads which + * object to query and which column to commit off the schema instead of + * re-deriving it, and a new {@link ApproverType} member cannot leave a stale + * mirror behind — `satisfies` above already makes an undeclared member a + * compile error, and this projection inherits that guarantee. + * + * Presentation stays with the renderer: which field to SHOW, whether to open a + * people-picker, what subtitle to put under a row are objectui's calls. This + * carries only the data contract — where the candidates live and what the + * committed value is. + */ +export const APPROVER_VALUE_SOURCES = Object.fromEntries( + Object.entries(APPROVER_VALUE_BINDINGS).map(([type, binding]) => [ + type, + binding.source === 'record' + // `data` names the DATA API, in contrast to the metadata registry the + // designer used to query — the whole point of the annotation. + ? { source: 'data', object: binding.object, valueField: binding.valueField } + : binding.source === 'enum' + ? { source: 'enum', values: [...binding.values] } + : { source: binding.source }, + ]), +) as Record< + z.infer, + | { source: 'data'; object: string; valueField: string } + | { source: 'enum'; values: string[] } + | { source: 'auto' | 'trigger-field' | 'expression' | 'unsupported' } +>; + // ========================================================================== // Approval as a Flow Node (ADR-0019, canonical) // ========================================================================== @@ -247,6 +304,11 @@ export const ApprovalNodeApproverSchema = lazySchema(() => z.object({ field: 'object-field', queue: 'queue', }, + // Where each kind's candidates actually live, and what the picker + // commits — see {@link APPROVER_VALUE_SOURCES}. `map` alone named a + // picker but never its data source, which is how the designer ended up + // querying the metadata registry for data records (#3508). + sources: APPROVER_VALUE_SOURCES, }, }), /** From 3c74aa2a999389ba25d24f9a52fb247a2bc6c426 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 07:20:14 +0000 Subject: [PATCH 2/3] chore(spec): regenerate the reference docs and API surface for the approver changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `check:docs` and `check:api-surface` are both generated-artifact gates: - `content/docs/references/automation/approval.mdx` — the `ApproverType` table and its Allowed Values list follow the enum's declaration order, which this branch reordered. Also reflowed the enum's own TSDoc: the doc generator emits one paragraph per source line, so the long ordering rationale now lives in `//` comments above the block and the JSDoc keeps a two-line summary. - `packages/spec/api-surface.json` — picks up the new `APPROVER_VALUE_SOURCES` export. NOTE: regenerating the API surface also absorbs drift that PREDATES this branch — `MEMBERSHIP_ROLE_NAME_MIN_LENGTH` / `MEMBERSHIP_ROLE_NAME_PATTERN`, `AppTranslationBundle*`, `ObjectTranslationNode*` added, and `LEGACY_OBJECT_FIRST_KEYS` / `LegacyObjectFirstKey` / `TranslationItem*` / `defineTranslation` removed. Those come from already-merged PRs that did not re-run the generator; `check:api-surface` fails on main without any of this branch's changes (verified by stashing them and re-running the gate). The snapshot exists to mirror the real export surface, so it is brought fully in sync rather than hand-patched to only this branch's line. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BVRVgSvmyCwmDfTmKmCZoH --- .../docs/references/automation/approval.mdx | 22 ++++++++----- packages/spec/api-surface.json | 14 +++++--- packages/spec/src/automation/approval.zod.ts | 32 +++++++++++-------- 3 files changed, 42 insertions(+), 26 deletions(-) diff --git a/content/docs/references/automation/approval.mdx b/content/docs/references/automation/approval.mdx index c6436ee165..7521b9b912 100644 --- a/content/docs/references/automation/approval.mdx +++ b/content/docs/references/automation/approval.mdx @@ -7,6 +7,12 @@ description: Approval protocol schemas Approval Step Approver Type +Declaration order is author-facing: designers derive their picker from this + +enum, and it leads with the portable indirect bindings (`manager`, + +`position`, `department`, `team`) — a literal `user` id comes last. + **Source:** `packages/spec/src/automation/approval.zod.ts` @@ -54,7 +60,7 @@ const result = ApprovalDecision.parse(data); | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **type** | `Enum<'user' \| 'org_membership_level' \| 'role' \| 'position' \| 'team' \| 'department' \| 'manager' \| 'field' \| 'queue' \| 'expression'>` | ✅ | | +| **type** | `Enum<'manager' \| 'position' \| 'department' \| 'team' \| 'field' \| 'expression' \| 'org_membership_level' \| 'role' \| 'user' \| 'queue'>` | ✅ | | | **value** | `string` | optional | User id / membership tier / position / team / department / field — per `type`; for `expression`, a CEL expression over `current.*` / `trigger.*` / `vars.*` | | **resolveAs** | `Enum<'user' \| 'department' \| 'position' \| 'team'>` | optional | How an `expression` result is expanded into approvers (default 'user') | | **group** | `string` | optional | Group label for per_group sign-off (e.g. "legal", "finance") | @@ -68,7 +74,7 @@ const result = ApprovalDecision.parse(data); | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **approvers** | `{ type: Enum<'user' \| 'org_membership_level' \| 'role' \| 'position' \| 'team' \| 'department' \| 'manager' \| 'field' \| 'queue' \| 'expression'>; value?: string; resolveAs?: Enum<'user' \| 'department' \| 'position' \| 'team'>; group?: string }[]` | ✅ | Allowed approvers for this node | +| **approvers** | `{ type: Enum<'manager' \| 'position' \| 'department' \| 'team' \| 'field' \| 'expression' \| 'org_membership_level' \| 'role' \| 'user' \| 'queue'>; value?: string; resolveAs?: Enum<'user' \| 'department' \| 'position' \| 'team'>; group?: string }[]` | ✅ | Allowed approvers for this node | | **behavior** | `Enum<'first_response' \| 'unanimous' \| 'quorum' \| 'per_group'>` | ✅ | How to combine multiple approvers | | **minApprovals** | `integer` | optional | Approvals required — total (quorum) or per group (per_group). Default 1 | | **lockRecord** | `boolean` | ✅ | Lock the record from editing while pending | @@ -85,16 +91,16 @@ const result = ApprovalDecision.parse(data); ### Allowed Values -* `user` -* `org_membership_level` -* `role` +* `manager` * `position` -* `team` * `department` -* `manager` +* `team` * `field` -* `queue` * `expression` +* `org_membership_level` +* `role` +* `user` +* `queue` --- diff --git a/packages/spec/api-surface.json b/packages/spec/api-surface.json index acfb19e2e9..b78c75d541 100644 --- a/packages/spec/api-surface.json +++ b/packages/spec/api-surface.json @@ -54,6 +54,8 @@ "MEMBERSHIP_ROLE_ADMIN (const)", "MEMBERSHIP_ROLE_DELEGATED_ADMIN (const)", "MEMBERSHIP_ROLE_MEMBER (const)", + "MEMBERSHIP_ROLE_NAME_MIN_LENGTH (const)", + "MEMBERSHIP_ROLE_NAME_PATTERN (const)", "MEMBERSHIP_ROLE_OWNER (const)", "METADATA_ALIASES (const)", "MIGRATIONS_BY_MAJOR (const)", @@ -614,6 +616,8 @@ "AppLike (interface)", "AppManifest (type)", "AppManifestSchema (const)", + "AppTranslationBundle (type)", + "AppTranslationBundleSchema (const)", "AudienceBook (interface)", "AudienceCaller (interface)", "AuthConfig (type)", @@ -871,10 +875,8 @@ "KeyRotationPolicy (type)", "KeyRotationPolicyInput (type)", "KeyRotationPolicySchema (const)", - "LEGACY_OBJECT_FIRST_KEYS (const)", "LWWRegister (type)", "LWWRegisterSchema (const)", - "LegacyObjectFirstKey (type)", "License (type)", "LicenseMetricType (type)", "LicenseSchema (const)", @@ -1007,6 +1009,8 @@ "ObjectStorageConfigSchema (const)", "ObjectTranslationData (type)", "ObjectTranslationDataSchema (const)", + "ObjectTranslationNode (type)", + "ObjectTranslationNodeSchema (const)", "OidcProviderConfig (type)", "OidcProviderConfigSchema (const)", "OidcProvidersConfig (type)", @@ -1257,8 +1261,6 @@ "TranslationDiffItemSchema (const)", "TranslationDiffStatus (type)", "TranslationDiffStatusSchema (const)", - "TranslationItem (type)", - "TranslationItemSchema (const)", "UserActivityStatus (type)", "VectorClock (type)", "VectorClockSchema (const)", @@ -1274,7 +1276,6 @@ "defineBook (function)", "defineEmailTemplateDefinition (function)", "defineJob (function)", - "defineTranslation (function)", "defineTranslationBundle (function)", "deriveImplicitPackageBook (function)", "docAudienceAllows (function)", @@ -1982,6 +1983,7 @@ "APPROVAL_NODE_TYPE (const)", "APPROVER_EXPRESSION_ROOTS (const)", "APPROVER_VALUE_BINDINGS (const)", + "APPROVER_VALUE_SOURCES (const)", "ActionCategory (type)", "ActionCategorySchema (const)", "ActionDescriptor (type)", @@ -4269,6 +4271,8 @@ "MEMBERSHIP_ROLE_ADMIN (const)", "MEMBERSHIP_ROLE_DELEGATED_ADMIN (const)", "MEMBERSHIP_ROLE_MEMBER (const)", + "MEMBERSHIP_ROLE_NAME_MIN_LENGTH (const)", + "MEMBERSHIP_ROLE_NAME_PATTERN (const)", "MEMBERSHIP_ROLE_OWNER (const)", "Member (type)", "MemberSchema (const)", diff --git a/packages/spec/src/automation/approval.zod.ts b/packages/spec/src/automation/approval.zod.ts index c36b56669c..7008ef17f5 100644 --- a/packages/spec/src/automation/approval.zod.ts +++ b/packages/spec/src/automation/approval.zod.ts @@ -3,22 +3,28 @@ import { z } from 'zod'; import { lazySchema } from '../shared/lazy-schema'; +// Why the members sit in THIS order (the generated reference renders the JSDoc +// below; this rationale stays in source): +// +// A designer that derives its approver-type picker from this enum — the Studio +// flow inspector does, via the published JSON schema minus `xEnumDeprecated` — +// offers the members in exactly this order. So the order is a recommendation +// the platform makes to every author, not an accident of when each member was +// added, and it belongs here rather than in any one renderer's options array +// (objectui#2834 put it there, where the inspector never read it). +// +// Indirect bindings therefore lead and the literal `user` binding comes last. +// Naming one specific person is the least portable choice an author can make: +// it breaks when the flow is deployed to another environment (that id does not +// exist there), and again when that person changes team or leaves — silently +// routing approvals to someone who should no longer see them. `manager` / +// `position` / `department` / `team` survive both. /** * Approval Step Approver Type * - * **Declaration order is author-facing.** A designer that derives its - * approver-type picker from this enum (the Studio flow inspector does, via the - * published JSON schema minus `xEnumDeprecated`) offers the members in exactly - * this order, so the order is the platform's recommendation, not an accident of - * when each member was added. - * - * INDIRECT bindings lead; the literal `user` binding comes last. Naming one - * specific person is the least portable choice an author can make — it breaks - * when the flow is deployed to another environment (that id does not exist - * there) and again when that person changes team or leaves, silently routing - * approvals to someone who should no longer see them. `manager` / `position` / - * `department` / `team` all survive both. Offering `user` first taught the - * opposite (objectui#2834). + * Declaration order is author-facing: designers derive their picker from this + * enum, and it leads with the portable indirect bindings (`manager`, + * `position`, `department`, `team`) — a literal `user` id comes last. */ export const ApproverType = z.enum([ 'manager', // Submitter's manager (sys_user.manager_id) From c60d16c7300daf8f4cd29314796051aa0147553f Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 07:31:36 +0000 Subject: [PATCH 3/3] =?UTF-8?q?fix(spec):=20regenerate=20api-surface.json?= =?UTF-8?q?=20from=20a=20clean=20build=20=E2=80=94=20only=20the=20new=20ex?= =?UTF-8?q?port?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit regenerated the snapshot against a `dist/` that turbo had restored from another checkout's cache, so it recorded exports this branch's source does not have (`AppTranslationBundle*`, `ObjectTranslationNode*`, `MEMBERSHIP_ROLE_NAME_*`) and dropped ones it does (`defineTranslation`, `TranslationItem*`, `LEGACY_OBJECT_FIRST_KEYS`). The generator reads the BUILT `.d.ts` from the exports map, so a stale dist silently produces a wrong snapshot — and it read as 8 breaking removals, which is exactly the alarm the gate exists to raise. `rm -rf packages/spec/dist && pnpm --filter @objectstack/spec build` first, then regenerate: the snapshot now differs from main by the single line this branch actually adds, `APPROVER_VALUE_SOURCES (const)`, and `check:api-surface` reports the surface unchanged. Every other spec gate (docs, skill-refs, skill-docs, react-blocks, spec-changes, upgrade-guide) re-verified green off the clean build. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BVRVgSvmyCwmDfTmKmCZoH --- packages/spec/api-surface.json | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/packages/spec/api-surface.json b/packages/spec/api-surface.json index b78c75d541..377492022c 100644 --- a/packages/spec/api-surface.json +++ b/packages/spec/api-surface.json @@ -54,8 +54,6 @@ "MEMBERSHIP_ROLE_ADMIN (const)", "MEMBERSHIP_ROLE_DELEGATED_ADMIN (const)", "MEMBERSHIP_ROLE_MEMBER (const)", - "MEMBERSHIP_ROLE_NAME_MIN_LENGTH (const)", - "MEMBERSHIP_ROLE_NAME_PATTERN (const)", "MEMBERSHIP_ROLE_OWNER (const)", "METADATA_ALIASES (const)", "MIGRATIONS_BY_MAJOR (const)", @@ -616,8 +614,6 @@ "AppLike (interface)", "AppManifest (type)", "AppManifestSchema (const)", - "AppTranslationBundle (type)", - "AppTranslationBundleSchema (const)", "AudienceBook (interface)", "AudienceCaller (interface)", "AuthConfig (type)", @@ -875,8 +871,10 @@ "KeyRotationPolicy (type)", "KeyRotationPolicyInput (type)", "KeyRotationPolicySchema (const)", + "LEGACY_OBJECT_FIRST_KEYS (const)", "LWWRegister (type)", "LWWRegisterSchema (const)", + "LegacyObjectFirstKey (type)", "License (type)", "LicenseMetricType (type)", "LicenseSchema (const)", @@ -1009,8 +1007,6 @@ "ObjectStorageConfigSchema (const)", "ObjectTranslationData (type)", "ObjectTranslationDataSchema (const)", - "ObjectTranslationNode (type)", - "ObjectTranslationNodeSchema (const)", "OidcProviderConfig (type)", "OidcProviderConfigSchema (const)", "OidcProvidersConfig (type)", @@ -1261,6 +1257,8 @@ "TranslationDiffItemSchema (const)", "TranslationDiffStatus (type)", "TranslationDiffStatusSchema (const)", + "TranslationItem (type)", + "TranslationItemSchema (const)", "UserActivityStatus (type)", "VectorClock (type)", "VectorClockSchema (const)", @@ -1276,6 +1274,7 @@ "defineBook (function)", "defineEmailTemplateDefinition (function)", "defineJob (function)", + "defineTranslation (function)", "defineTranslationBundle (function)", "deriveImplicitPackageBook (function)", "docAudienceAllows (function)", @@ -4271,8 +4270,6 @@ "MEMBERSHIP_ROLE_ADMIN (const)", "MEMBERSHIP_ROLE_DELEGATED_ADMIN (const)", "MEMBERSHIP_ROLE_MEMBER (const)", - "MEMBERSHIP_ROLE_NAME_MIN_LENGTH (const)", - "MEMBERSHIP_ROLE_NAME_PATTERN (const)", "MEMBERSHIP_ROLE_OWNER (const)", "Member (type)", "MemberSchema (const)",