feat(spec)+fix(approvals): approver value sources on the wire, author-facing type order, no more silent dead slots (#3508 / #3807) (#3817)

os-zhuang · web-flow · commit 0f8ad0959679 · 2026-07-28T15:45:48.000+08:00
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 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. 2. `ApproverType` order (spec) — the Studio picker derives from this enum via the published schema, so the indirect-bindings-first order objectui#2834 argued for never took effect. The enum now carries it. 3. Dead approver slots (plugin-approvals) — every graph type that expands to nobody now says so; only `queue` used to. That silence is what let #3807 hide until an approval was permanently stuck. 4. plugin-sharing's identical org scope (tests only) — unreachable today and an authorization path, so it is pinned by tests rather than widened blind.
diff --git a/.changeset/approver-value-sources-and-dead-slot-warning.md 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/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.
+
 <Callout type="info">
 **Source:** `packages/spec/src/automation/approval.zod.ts`
 </Callout>
@@ -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/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
@@ -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<string> = 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:<id>` 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
@@ -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 = <rule org>` 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:<id>` 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/api-surface.json b/packages/spec/api-surface.json
@@ -1986,6 +1986,7 @@
     "APPROVAL_NODE_TYPE (const)",
     "APPROVER_EXPRESSION_ROOTS (const)",
     "APPROVER_VALUE_BINDINGS (const)",
+    "APPROVER_VALUE_SOURCES (const)",
     "ActionCategory (type)",
     "ActionCategorySchema (const)",
     "ActionDescriptor (type)",
diff --git a/packages/spec/src/automation/approval.test.ts b/packages/spec/src/automation/approval.test.ts
diff --git a/packages/spec/src/automation/approval.zod.ts b/packages/spec/src/automation/approval.zod.ts
