From 41cab60fde30a83d4d3093e9246c87f7928d673b Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 16 Sep 2026 19:07:04 +0000 Subject: [PATCH 1/5] feat(spec): approval onEmptyApprovers gains 'fallback' with a sibling fallbackApprovers The node-level empty-slate policy can now NAME people. `fallbackApprovers` reuses `ApprovalNodeApproverSchema` (not a second approver shape) and is paired with the policy in BOTH directions by a `superRefine`: required when the policy is 'fallback', refused under any other policy so a list nothing reads can never ship. Claude-Session: https://claude.ai/code/session_01KB5PFtxuy1x3dcR5gxudx6 Co-authored-by: Claude --- packages/spec/src/automation/approval.test.ts | 129 ++++++++++++++++++ packages/spec/src/automation/approval.zod.ts | 111 ++++++++++++++- 2 files changed, 237 insertions(+), 3 deletions(-) diff --git a/packages/spec/src/automation/approval.test.ts b/packages/spec/src/automation/approval.test.ts index fe1b4375c24..fb3736c7770 100644 --- a/packages/spec/src/automation/approval.test.ts +++ b/packages/spec/src/automation/approval.test.ts @@ -318,6 +318,135 @@ describe('ApprovalNodeConfigSchema', () => { }); }); +// The node-level empty-slate policy that NAMES people. `manager` is the card's +// proving rung, but nothing here is manager-specific: all five graph approver +// types fall back to the same `type:value` literal when their lookup finds +// nobody, and the node is where that is decided. +describe("ApprovalNodeConfigSchema — onEmptyApprovers: 'fallback' + fallbackApprovers", () => { + const minimal = { approvers: [{ type: 'manager' }] }; + const owner = { type: 'org_membership_level', value: 'owner' }; + + const issuesOf = (input: unknown) => { + const r = ApprovalNodeConfigSchema.safeParse(input); + expect(r.success, 'expected this config to be REFUSED').toBe(false); + return r.error!.issues; + }; + + it("accepts 'fallback' with a sibling fallbackApprovers, and keeps the list verbatim", () => { + const cfg = ApprovalNodeConfigSchema.parse({ + ...minimal, + onEmptyApprovers: 'fallback', + fallbackApprovers: [owner, { type: 'user', value: 'u_backstop', group: 'backstop' }], + }); + expect(cfg.onEmptyApprovers).toBe('fallback'); + expect(cfg.fallbackApprovers).toEqual([ + { type: 'org_membership_level', value: 'owner' }, + { type: 'user', value: 'u_backstop', group: 'backstop' }, + ]); + }); + + // The whole point of reusing ApprovalNodeApproverSchema rather than declaring + // a second approver shape: the fallback is not a dialect. An entry that the + // primary list would refuse is refused here too, by the SAME surface error. + it('resolves entries through the approver shape `approvers` uses, not a second one', () => { + expect(ApprovalNodeConfigSchema.shape.fallbackApprovers).toBeDefined(); + const bad = issuesOf({ + ...minimal, + onEmptyApprovers: 'fallback', + fallbackApprovers: [{ type: 'not_a_type', value: 'x' }], + }); + expect(bad.some((i) => i.path.join('.') === 'fallbackApprovers.0.type')).toBe(true); + + // …and an unknown KEY inside an entry is rejected by the approver surface's + // own strict error, the same one an `approvers` entry would raise. + const unknownKey = issuesOf({ + ...minimal, + onEmptyApprovers: 'fallback', + fallbackApprovers: [{ type: 'user', value: 'u1', escalateTo: 'u2' }], + }); + expect(unknownKey.some((i) => i.code === 'unrecognized_keys')).toBe(true); + }); + + it("refuses 'fallback' with no fallbackApprovers, naming BOTH keys", () => { + const issues = issuesOf({ ...minimal, onEmptyApprovers: 'fallback' }); + const hit = issues.find((i) => i.path.join('.') === 'fallbackApprovers'); + expect(hit, 'no issue on fallbackApprovers').toBeTruthy(); + expect(hit!.message).toContain('onEmptyApprovers'); + expect(hit!.message).toContain('fallbackApprovers'); + // The remedy offers both edits, because only the author knows which + // they meant. + expect(hit!.message).toContain('admin_rescue'); + }); + + it('refuses an empty fallbackApprovers array — a named slate that names nobody', () => { + const issues = issuesOf({ ...minimal, onEmptyApprovers: 'fallback', fallbackApprovers: [] }); + expect(issues.some((i) => i.path.join('.').startsWith('fallbackApprovers'))).toBe(true); + }); + + // The reverse arm, and the one that earns its place: `admin_rescue` is the + // DEFAULT, so "added the list, forgot the policy" is the likeliest slip and + // the one that would otherwise ship a node silently ignoring half its config. + it('refuses fallbackApprovers under the DEFAULT policy, and says the key is omitted', () => { + const issues = issuesOf({ ...minimal, fallbackApprovers: [owner] }); + const hit = issues.find((i) => i.path.join('.') === 'onEmptyApprovers'); + expect(hit, 'no issue on onEmptyApprovers').toBeTruthy(); + expect(hit!.message).toContain('fallbackApprovers'); + expect(hit!.message).toContain("'fallback'"); + expect(hit!.message).toContain('admin_rescue'); + expect(hit!.message).toContain('default'); + }); + + it.each(['admin_rescue', 'fail', 'auto_approve'])( + "refuses fallbackApprovers under '%s', naming that policy", + (policy) => { + const issues = issuesOf({ ...minimal, onEmptyApprovers: policy, fallbackApprovers: [owner] }); + const hit = issues.find((i) => i.path.join('.') === 'onEmptyApprovers'); + expect(hit, `no issue on onEmptyApprovers for ${policy}`).toBeTruthy(); + expect(hit!.message).toContain(`'${policy}'`); + expect(hit!.message).toContain('fallbackApprovers'); + }, + ); + + // Negative controls: the three pre-existing policies are untouched by this + // widening — they still parse with no sibling key, and the default still + // resolves to admin_rescue. + it.each(['admin_rescue', 'fail', 'auto_approve'])('leaves %s parsing exactly as before', (policy) => { + const cfg = ApprovalNodeConfigSchema.parse({ ...minimal, onEmptyApprovers: policy }); + expect(cfg.onEmptyApprovers).toBe(policy); + expect(cfg.fallbackApprovers).toBeUndefined(); + }); + + it('leaves the omitted policy defaulting to admin_rescue', () => { + expect(ApprovalNodeConfigSchema.parse(minimal).onEmptyApprovers).toBe('admin_rescue'); + }); + + // The refinement must not cost the shape its strictness or its class: a + // ZodEffects wrapper would silently drop `.shape`, `additionalProperties: + // false` on the published JSON schema, and the whole #4001 unknown-key error. + it('keeps the strict unknown-key contract the refinement is attached to', () => { + const issues = issuesOf({ ...minimal, onEmptyApprover: 'fallback' }); + expect(issues.some((i) => i.code === 'unrecognized_keys')).toBe(true); + }); + + // The designer renders the property form from this JSON schema, so a member + // that never reaches it is a member no author can pick. + it('publishes the fourth member and the sibling key on the JSON schema the designer reads', () => { + const js = getApprovalNodeConfigJsonSchema() as any; + expect(js.properties.onEmptyApprovers.enum).toEqual( + ['admin_rescue', 'fail', 'auto_approve', 'fallback'], + ); + expect(js.properties.fallbackApprovers).toBeDefined(); + expect(js.properties.fallbackApprovers.type).toBe('array'); + expect(js.additionalProperties).toBe(false); + }); + + it("documents the sibling key on the policy's own description", () => { + const doc = ApprovalNodeConfigSchema.shape.onEmptyApprovers.description ?? ''; + expect(doc).toContain('fallbackApprovers'); + expect(doc).toContain("'fallback'"); + }); +}); + describe('ApprovalEscalationSchema', () => { it('defaults action to notify and requires a positive timeout', () => { const result = ApprovalEscalationSchema.parse({ enabled: true, timeoutHours: 24 }); diff --git a/packages/spec/src/automation/approval.zod.ts b/packages/spec/src/automation/approval.zod.ts index 56026b83c53..3750696713e 100644 --- a/packages/spec/src/automation/approval.zod.ts +++ b/packages/spec/src/automation/approval.zod.ts @@ -691,6 +691,74 @@ export type ApprovalEscalationParsed = z.infer; * first-class engine-adjacent state owned by `plugin-approvals`; this config * only describes how the node behaves. */ +/** + * `onEmptyApprovers: 'fallback'` ↔ `fallbackApprovers` must be declared as one + * unit, in both directions. + * + * ## Why a refinement and not a discriminated union + * + * `automation/ApprovalNodeConfig` is a published JSON-Schema def — the Studio + * property form and `registerFlow()`'s per-node config validation both read it + * (see {@link getApprovalNodeConfigJsonSchema}). A union would fan that one def + * into four branches re-declaring the same eleven keys, and the designer would + * render whichever branch it happened to pick. A refinement adds no structure, + * keeps the `ZodObject` class and `.shape` intact under Zod 4, and emits ONE + * issue on the key the author has to edit. + * + * ## Why BOTH arms + * + * The forward arm is obvious: a policy that names people and names nobody + * cannot run. The reverse arm is the one that earns its place — a + * `fallbackApprovers` list under `admin_rescue` (the DEFAULT, so the arm fires + * on the likeliest authoring slip: adding the list and forgetting the policy) + * is read by nothing at all. Accepting it would ship a node that declares a + * rescue slate and silently ignores it, which is exactly the failure this + * module's `.strict()` shapes exist to make audible. + * + * Both messages name BOTH keys, because either one of them is a valid edit and + * only the author knows which they meant. + */ +function checkFallbackApproversPairing( + cfg: { onEmptyApprovers?: unknown; fallbackApprovers?: unknown }, + ctx: z.RefinementCtx, +): void { + // `.default('admin_rescue')` has already been applied by the time a + // refinement runs, so an omitted key and an explicit `admin_rescue` are + // INDISTINGUISHABLE here. The message says so rather than guessing: on the + // default value it names both readings, because "added the list, forgot the + // key" is the slip this arm exists for and an author told flatly that their + // policy is `admin_rescue` would go looking for a key they never wrote. + const policy = typeof cfg.onEmptyApprovers === 'string' ? cfg.onEmptyApprovers : 'admin_rescue'; + const declared = Array.isArray(cfg.fallbackApprovers) && cfg.fallbackApprovers.length > 0; + + if (policy === 'fallback') { + if (declared) return; + ctx.addIssue({ + code: 'custom', + path: ['fallbackApprovers'], + message: + "onEmptyApprovers: 'fallback' needs a sibling fallbackApprovers naming who takes the " + + 'request when the primary slate resolves to nobody — it is the one empty-slate policy ' + + 'that names people. Either add fallbackApprovers (same shape as approvers, e.g. ' + + "[{ type: 'org_membership_level', value: 'owner' }]), or choose a policy that names " + + "nobody: 'admin_rescue', 'fail' or 'auto_approve'.", + }); + return; + } + + if (!declared) return; + ctx.addIssue({ + code: 'custom', + path: ['onEmptyApprovers'], + message: + `fallbackApprovers is only read when onEmptyApprovers is 'fallback', and this node's ` + + `policy is '${policy}'` + + (policy === 'admin_rescue' ? ' (either declared, or the default that applies when the key is omitted)' : '') + + ', so the list would be silently ignored. Either set ' + + "onEmptyApprovers: 'fallback' to make it live, or remove fallbackApprovers.", + }); +} + export const ApprovalNodeConfigSchema = lazySchema(() => strictObject( { surface: "this approval node's config", @@ -790,9 +858,46 @@ export const ApprovalNodeConfigSchema = lazySchema(() => strictObject( * - `auto_approve` — skip the request and continue down the `approve` edge * with `output.autoApproved = true`. The DingTalk/Feishu default; opt-in * here because it silently waves the record through. + * - `fallback` — open the request on {@link ApprovalNodeConfig.fallbackApprovers} + * instead. The only policy that NAMES people, so it is the only one that + * turns an empty slate into a request someone can actually decide rather + * than a state an operator has to recover from. Measured elsewhere as + * Entra's "Add fallback" and Odoo's "If empty, the approval is done by an + * Administrator or Approver" — taken here at the NODE, not on the + * `manager` rung: all five graph approver types share the same dead end + * (a lookup that finds nobody leaves a `type:value` literal no user can + * act on), and the node is already where emptiness is decided. + * + * The fallback slate is resolved by the SAME resolver as `approvers`, so + * every approver type, OOO delegation and `per_group` tagging behaves + * identically on it. A fallback that itself resolves to nobody degrades to + * `admin_rescue` — the run is never killed and the record is never waved + * through by a policy that only asked for different people. + */ + onEmptyApprovers: z.enum(['admin_rescue', 'fail', 'auto_approve', 'fallback']).default('admin_rescue') + .describe( + 'Behavior when no concrete approver resolves at node entry — ' + + "'fallback' opens the request on fallbackApprovers instead", + ), + + /** + * Who takes the request when `onEmptyApprovers` is `'fallback'` and the + * primary slate resolved to nobody. Same shape as `approvers` — + * {@link ApprovalNodeApproverSchema}, deliberately NOT a second approver + * shape: a fallback that could not express `{ type: 'position' }` or a + * `group` would be a dialect of the key it backs up. + * + * Required iff the policy is `'fallback'`, and refused otherwise — both + * arms are enforced by {@link checkFallbackApproversPairing}. The second arm + * is the load-bearing one: a `fallbackApprovers` list under any other policy + * is read by nothing, and a node that quietly ignores half its config is the + * exact ADR-0078 trap this shape was closed against. + * + * Prefer a target that cannot itself resolve empty — a literal `user`, or + * `{ type: 'org_membership_level', value: 'owner' }`. */ - onEmptyApprovers: z.enum(['admin_rescue', 'fail', 'auto_approve']).default('admin_rescue') - .describe('Behavior when no concrete approver resolves at node entry'), + fallbackApprovers: z.array(ApprovalNodeApproverSchema).min(1).optional() + .describe("Approvers the request opens on when onEmptyApprovers is 'fallback'"), /** * #3447 P2: keys a decision may carry as structured outputs @@ -832,7 +937,7 @@ export const ApprovalNodeConfigSchema = lazySchema(() => strictObject( */ maxRevisions: z.number().int().min(0).default(3) .describe('Max send-backs for revision before auto-reject (0 = send-back disabled)'), -})); +}).superRefine(checkFallbackApproversPairing)); export type ApprovalNodeConfig = z.input; /** Post-parse shape of {@link ApprovalNodeConfig} — defaults applied, transforms run (ADR-0122). */ export type ApprovalNodeConfigParsed = z.infer; From 27eb5139f4a5ccfaed44a247c34e67bb2b73f43f Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 16 Sep 2026 19:15:43 +0000 Subject: [PATCH 2/5] feat(approvals): openNodeRequest opens on fallbackApprovers when the slate is empty MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `fallback` empty-slate policy resolves the node's declared `fallbackApprovers` through the SAME expansion as `approvers` and opens the request on those ids — never on the `manager:undefined` literal an empty manager rung leaves behind. A fallback that itself resolves to nobody, or a stored node declaring the policy with no list, degrades to admin_rescue. Adds the missing `manager` row to the graph-approver warning table. Claude-Session: https://claude.ai/code/session_01KB5PFtxuy1x3dcR5gxudx6 Co-authored-by: Claude --- .../src/approval-service.test.ts | 191 +++++++++++++++++- .../plugin-approvals/src/approval-service.ts | 95 ++++++++- .../spec/authorable-surface/automation.json | 1 + 3 files changed, 278 insertions(+), 9 deletions(-) diff --git a/packages/plugins/plugin-approvals/src/approval-service.test.ts b/packages/plugins/plugin-approvals/src/approval-service.test.ts index 8ddb4febbab..d36db2fc81b 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.test.ts @@ -3140,9 +3140,16 @@ describe('ApprovalService — a graph approver that expands to nobody warns (#38 return { svc, warnings }; }; - const approverInput = (type: string, value: string) => ({ + // `value` is OPTIONAL because `manager` omits it (it is resolved from the + // submitter's `sys_user.manager_id`, never authored) — and the literal it + // falls back to interpolates that absence, which is the row below. + const approverInput = (type: string, value?: string) => ({ ...openInput([]), - config: { approvers: [{ type, value }], behavior: 'first_response' as const, lockRecord: false }, + config: { + approvers: [value === undefined ? { type } : { type, value }], + behavior: 'first_response' as const, + lockRecord: false, + }, }); it.each([ @@ -3150,6 +3157,12 @@ describe('ApprovalService — a graph approver that expands to nobody warns (#38 ['department', 'bu_gone'], ['position', 'nobody_holds_this'], ['org_membership_level', 'member'], + // `manager` is in GRAPH_APPROVER_TYPES like the four above, and the row was + // missing from this table. It is the worst of the set, not the mildest: it + // authors no `value`, so the dead literal it leaves is `manager:undefined` + // — a slot that names a person who does not exist AND reads like a bug in + // the platform to whoever finds it in `pending_approvers`. + ['manager', undefined], ])('%s: the dead literal is logged with its type, value and org', async (type, value) => { const engine = makeFakeEngine(); const { svc, warnings } = svcWithWarnings(engine); @@ -3182,6 +3195,180 @@ describe('ApprovalService — a graph approver that expands to nobody warns (#38 }); }); +// The empty-slate policy that NAMES people. Before it, an empty +// `{ type: 'manager' }` rung had three endings and all three were bad: open on +// `manager:undefined` and wait for an admin (`admin_rescue`), kill the run +// (`fail`), or wave the record through (`auto_approve`). `fallback` adds the +// fourth — open on someone who can actually decide. +// +// The rung is not special. Every graph approver type ends at the same literal; +// `manager` is just the one that reaches it without anybody authoring a wrong +// value, which is why it is the proving case here. +describe("ApprovalService — onEmptyApprovers: 'fallback' (node-level named rescue)", () => { + 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 }; + }; + + /** A node whose only approver is an unset manager rung — the empty slate. */ + const managerNode = (configExtra: Record = {}) => ({ + ...openInput([]), + config: { + approvers: [{ type: 'manager' as const }], + behavior: 'first_response' as const, + lockRecord: false, + ...configExtra, + }, + }); + + const OWNER_BACKSTOP = { type: 'user' as const, value: 'u_backstop' }; + const said = (warnings: any[], needle: string) => + warnings.some(([msg]) => String(msg).includes(needle)); + + it('opens the request on the declared fallback, never on the manager:undefined literal', async () => { + const engine = makeFakeEngine(); + const { svc, warnings } = svcWithWarnings(engine); + const req = await svc.openNodeRequest(managerNode({ + onEmptyApprovers: 'fallback', + fallbackApprovers: [OWNER_BACKSTOP], + }), CTX) as any; + + expect(req.status).toBe('pending'); + expect(req.pending_approvers).toEqual(['u_backstop']); + expect(req.pending_approvers).not.toContain('manager:undefined'); + expect(said(warnings, "onEmptyApprovers: 'fallback'")).toBe(true); + // The request is decidable by a person, so the admin-takeover warning must + // NOT fire — it would be false about this request. + expect(said(warnings, 'decidable only by a privileged admin')).toBe(false); + }); + + // "the same resolver as ordinary approver entries" is the ruled requirement, + // so the fallback must expand a GRAPH type, not just literal ids. + it('resolves the fallback through the same expansion approvers use', async () => { + const engine = makeFakeEngine(); + engine._tables['sys_team_member'] = [{ id: 'tm1', team_id: 'team_ok', user_id: 'u5' }]; + const { svc } = svcWithWarnings(engine); + const req = await svc.openNodeRequest(managerNode({ + onEmptyApprovers: 'fallback', + fallbackApprovers: [{ type: 'team', value: 'team_ok' }], + }), CTX) as any; + + expect(req.pending_approvers).toEqual(['u5']); + }); + + it('namespaces the fallback resolution inputs in the audit snapshot, keeping the primary ones', async () => { + const engine = makeFakeEngine(); + const { svc } = svcWithWarnings(engine); + await svc.openNodeRequest({ + ...openInput([]), + variables: { primary: [], rescue: ['u7'] }, + config: { + approvers: [{ type: 'expression' as const, value: 'vars.primary' }], + behavior: 'first_response' as const, + lockRecord: false, + onEmptyApprovers: 'fallback', + fallbackApprovers: [{ type: 'expression' as const, value: 'vars.rescue' }], + }, + } as any, CTX); + + const snapshot = JSON.parse(engine._tables['sys_approval_request'][0].node_config_json); + // WHY the fallback fired, and WHO it picked — both answerable later. + expect(snapshot.__resolvedFrom).toEqual({ + 'expression#0': [], + 'fallback:expression#0': ['u7'], + }); + }); + + // A fallback is a rescue path: when it too finds nobody the run must not die + // and the record must not be waved through. It degrades to the default. + it('degrades to admin_rescue when the fallback ITSELF resolves to nobody', async () => { + const engine = makeFakeEngine(); + const { svc, warnings } = svcWithWarnings(engine); + const req = await svc.openNodeRequest(managerNode({ + onEmptyApprovers: 'fallback', + fallbackApprovers: [{ type: 'team', value: 'team_also_gone' }], + }), CTX) as any; + + expect(req.status).toBe('pending'); + expect(req.pending_approvers).toEqual(['manager:undefined']); + expect(said(warnings, 'resolved to nobody either')).toBe(true); + expect(said(warnings, 'decidable only by a privileged admin')).toBe(true); + }); + + // The spec refuses this pairing at authoring time; a STORED node written + // before the policy existed can still reach the service. + it('degrades to admin_rescue when the policy is declared with no fallbackApprovers', async () => { + const engine = makeFakeEngine(); + const { svc, warnings } = svcWithWarnings(engine); + const req = await svc.openNodeRequest(managerNode({ onEmptyApprovers: 'fallback' }), CTX) as any; + + expect(req.status).toBe('pending'); + expect(req.pending_approvers).toEqual(['manager:undefined']); + expect(said(warnings, 'with no fallbackApprovers')).toBe(true); + expect(said(warnings, 'decidable only by a privileged admin')).toBe(true); + }); + + it('never consults the fallback when the primary slate resolved someone', async () => { + const engine = makeFakeEngine(); + const { svc, warnings } = svcWithWarnings(engine); + const req = await svc.openNodeRequest({ + ...openInput([]), + config: { + approvers: [{ type: 'user' as const, value: 'u9' }], + behavior: 'first_response' as const, + lockRecord: false, + onEmptyApprovers: 'fallback', + fallbackApprovers: [OWNER_BACKSTOP], + }, + } as any, CTX) as any; + + expect(req.pending_approvers).toEqual(['u9']); + expect(said(warnings, "onEmptyApprovers: 'fallback'")).toBe(false); + }); + + // ── negative controls ───────────────────────────────────────────── + // + // The other three policies are untouched by this change — including when a + // `fallbackApprovers` list is present on the row (the spec refuses that + // pairing at authoring time; the runtime simply does not read it). + + it("'fail' still fails loudly, fallbackApprovers on the row or not", async () => { + const engine = makeFakeEngine(); + const { svc } = svcWithWarnings(engine); + await expect(svc.openNodeRequest(managerNode({ + onEmptyApprovers: 'fail', fallbackApprovers: [OWNER_BACKSTOP], + }), CTX)).rejects.toThrow(/NO_APPROVERS/); + expect(engine._tables['sys_approval_request'] ?? []).toHaveLength(0); + }); + + it("'auto_approve' still waves through, fallbackApprovers on the row or not", async () => { + const engine = makeFakeEngine(); + const { svc } = svcWithWarnings(engine); + const outcome = await svc.openNodeRequest(managerNode({ + onEmptyApprovers: 'auto_approve', fallbackApprovers: [OWNER_BACKSTOP], + }), CTX); + expect(outcome).toEqual({ autoApproved: true, reason: 'empty_approvers' }); + expect(engine._tables['sys_approval_request'] ?? []).toHaveLength(0); + }); + + it("'admin_rescue' still opens on the dead literal and warns", async () => { + const engine = makeFakeEngine(); + const { svc, warnings } = svcWithWarnings(engine); + const req = await svc.openNodeRequest(managerNode({ + onEmptyApprovers: 'admin_rescue', fallbackApprovers: [OWNER_BACKSTOP], + }), CTX) as any; + expect(req.pending_approvers).toEqual(['manager:undefined']); + expect(said(warnings, 'decidable only by a privileged admin')).toBe(true); + expect(said(warnings, "onEmptyApprovers: 'fallback'")).toBe(false); + }); +}); + // ── 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 bbae6c8f215..280e7c8879e 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -2652,6 +2652,11 @@ export class ApprovalService implements IApprovalService { * #3447 P2: may instead return an {@link ApprovalNodeAutoOutcome} — no * request opened — when the slate resolves empty and the node's * `onEmptyApprovers` policy is `auto_approve`. + * + * An empty slate under `onEmptyApprovers: 'fallback'` opens a NORMAL request + * on the node's declared `fallbackApprovers` instead, resolved through the + * same expansion as `approvers`. The return type is unchanged for that + * policy: from the flow node's side it is an ordinary pending request. */ async openNodeRequest( input: { @@ -2732,10 +2737,13 @@ export class ApprovalService implements IApprovalService { const subjectOrg = this.recordOrgResolver.organizationOf(input.object, liveRecord, input.record); const requestOrg = subjectOrg ?? ctxOrg; const resolvedFrom: Record = {}; - const approvers = await this.expandApprovers( + const exprCtx = { trigger: input.record ?? null, vars: input.variables ?? null }; + // `let`, not `const`: the `fallback` empty-slate policy below REPLACES this + // slate with the node's declared fallback approvers. + let approvers = await this.expandApprovers( { approvers: input.config.approvers }, liveRecord, requestOrg, { now: nowDate.getTime(), substitutions, groups, - exprCtx: { trigger: input.record ?? null, vars: input.variables ?? null }, + exprCtx, resolvedFrom, }, ); @@ -2760,16 +2768,89 @@ export class ApprovalService implements IApprovalService { ); return { autoApproved: true, reason: 'empty_approvers' }; } + // `fallback`: the one policy that NAMES people. The declared + // `fallbackApprovers` go through the SAME resolver as `approvers` — not + // a parallel path — so every approver type, OOO delegation (#1322) and + // per_group tagging (#3266) behaves on the fallback exactly as it does on + // the primary slate. The request then opens on THOSE ids. + // + // ⛔ What must never happen here is opening on the abandoned slate: an + // empty `{ type: 'manager' }` rung leaves the literal `manager:undefined` + // (the type is in GRAPH_APPROVER_TYPES and `value` is omitted for + // `manager`, so the literal interpolates the missing value), and that + // slot is decidable by nobody. Replacing the slate is the whole point. + if (emptyPolicy === 'fallback') { + const declared = (input.config as any).fallbackApprovers; + // The spec refuses `fallback` without a non-empty list, so an empty one + // here means a stored node written before the policy existed, or a + // caller that bypassed the schema. Degrade to admin_rescue rather than + // throw: this is a rescue path, and killing the run is the one outcome + // the policy was chosen to avoid. + if (Array.isArray(declared) && declared.length) { + const fallbackResolvedFrom: Record = {}; + const fallbackApprovers = await this.expandApprovers( + { approvers: declared }, liveRecord, requestOrg, { + now: nowDate.getTime(), substitutions, groups, + exprCtx, + resolvedFrom: fallbackResolvedFrom, + }, + ); + // Keep the primary slate's resolution inputs (they explain WHY the + // fallback fired) and namespace the fallback's own, so neither + // clobbers the other in the `__resolvedFrom` audit snapshot. + for (const [k, v] of Object.entries(fallbackResolvedFrom)) { + resolvedFrom[`fallback:${k}`] = v; + } + if (fallbackApprovers.some(a => a && !a.includes(':'))) { + this.logger?.warn?.( + `[approvals] approval node '${input.nodeId}' on ${input.object}/${input.recordId} resolved to no ` + + `concrete approver — opening on the declared fallbackApprovers per onEmptyApprovers: 'fallback'.`, + { + object: input.object, recordId: input.recordId, node: input.nodeId, + resolved: approvers, fallback: fallbackApprovers, + }, + ); + // The abandoned slate's per_group tags describe ids that are no + // longer on this request; drop them so the snapshot the tally reads + // describes the slate the approvers actually see. + for (const slot of approvers) delete groups[slot]; + approvers = fallbackApprovers; + } else { + this.logger?.warn?.( + `[approvals] approval node '${input.nodeId}' on ${input.object}/${input.recordId} resolved to no ` + + `concrete approver AND its declared fallbackApprovers resolved to nobody either — falling back ` + + `to admin_rescue. Check that the fallback target(s) are staffed.`, + { + object: input.object, recordId: input.recordId, node: input.nodeId, + resolved: approvers, fallback: fallbackApprovers, + }, + ); + } + } else { + this.logger?.warn?.( + `[approvals] approval node '${input.nodeId}' on ${input.object}/${input.recordId} declares ` + + `onEmptyApprovers: 'fallback' with no fallbackApprovers — falling back to admin_rescue.`, + { object: input.object, recordId: input.recordId, node: input.nodeId, resolved: approvers }, + ); + } + } + // #3424 admin_rescue (default): the request is still opened (a privileged // admin can override it, and legacy 15.x literal slots stay queryable) — // the only option that neither waves the record through nor kills the // run — but warn loudly so the misconfiguration surfaces instead of // silently locking the record with no obvious cause. - this.logger?.warn?.( - `[approvals] approval node '${input.nodeId}' on ${input.object}/${input.recordId} resolved to no concrete approver` - + ' — the request is decidable only by a privileged admin. Check that the approver target(s) are staffed.', - { object: input.object, recordId: input.recordId, node: input.nodeId, resolved: approvers }, - ); + // + // Re-tested rather than assumed: a `fallback` that landed people above + // has already turned this into a normal request, and warning "decidable + // only by a privileged admin" about it would be false. + if (!approvers.some(a => a && !a.includes(':'))) { + this.logger?.warn?.( + `[approvals] approval node '${input.nodeId}' on ${input.object}/${input.recordId} resolved to no concrete approver` + + ' — the request is decidable only by a privileged admin. Check that the approver target(s) are staffed.', + { object: input.object, recordId: input.recordId, node: input.nodeId, resolved: approvers }, + ); + } } const now = nowDate.toISOString(); diff --git a/packages/spec/authorable-surface/automation.json b/packages/spec/authorable-surface/automation.json index 3db3517c88b..35396430b34 100644 --- a/packages/spec/authorable-surface/automation.json +++ b/packages/spec/authorable-surface/automation.json @@ -36,6 +36,7 @@ "automation/ApprovalNodeConfig:behavior", "automation/ApprovalNodeConfig:decisionOutputs", "automation/ApprovalNodeConfig:escalation", + "automation/ApprovalNodeConfig:fallbackApprovers", "automation/ApprovalNodeConfig:lockRecord", "automation/ApprovalNodeConfig:maxRevisions", "automation/ApprovalNodeConfig:minApprovals", From 3cb510d9393192cca33b9051abdb343209d91c10 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 16 Sep 2026 19:25:52 +0000 Subject: [PATCH 3/5] feat(lint,docs): name the node-level fallback escape and pin the shape-only verdict MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `approval-approvers-may-resolve-empty` now offers `onEmptyApprovers: 'fallback'` as a route that needs no write to `sys_user.manager_id`, and the empty-policy nudge enumerates all four members. The finding still fires when a fallback is declared — the rule reads shape, and a fallback list can resolve empty too. Claude-Session: https://claude.ai/code/session_01KB5PFtxuy1x3dcR5gxudx6 Co-authored-by: Claude --- ...7931-approval-onemptyapprovers-fallback.md | 23 ++++++++ content/docs/automation/approvals.mdx | 41 ++++++++++++--- .../src/validate-approval-approvers.test.ts | 52 +++++++++++++++++++ .../lint/src/validate-approval-approvers.ts | 30 ++++++++--- 4 files changed, 132 insertions(+), 14 deletions(-) create mode 100644 .changeset/17931-approval-onemptyapprovers-fallback.md diff --git a/.changeset/17931-approval-onemptyapprovers-fallback.md b/.changeset/17931-approval-onemptyapprovers-fallback.md new file mode 100644 index 00000000000..268f3cdf4e5 --- /dev/null +++ b/.changeset/17931-approval-onemptyapprovers-fallback.md @@ -0,0 +1,23 @@ +--- +"@objectstack/spec": minor +"@objectstack/plugin-approvals": minor +"@objectstack/lint": minor +--- + +Approval nodes gain a fourth empty-slate policy — `onEmptyApprovers: 'fallback'` with a sibling `fallbackApprovers` list — so a rung that expands to nobody opens the request on people you named instead of on a slot nobody can act on. + +Until now an approval node whose approvers resolved to nobody had three endings, and none of them named anyone: `admin_rescue` (the default — the request opens on a dead `type:value` slot and waits for a privileged admin), `fail` (the run dies) and `auto_approve` (the record is waved through). All five graph approver types reach that dead end, and `{ type: 'manager' }` reaches it without anybody authoring a wrong value: `manager` omits `value`, so the literal the expansion falls back to is `manager:undefined`. + +```ts +{ + approvers: [{ type: 'manager' }], + onEmptyApprovers: 'fallback', + fallbackApprovers: [{ type: 'org_membership_level', value: 'owner' }], +} +``` + +- **`fallbackApprovers` is the approver shape you already write** — the same entries as `approvers`, resolved by the same expansion, so every approver type, OOO delegation and `per_group` tagging behaves identically on it. It is not a second, reduced approver dialect. +- **The pairing is enforced in both directions.** `'fallback'` without a list is refused; a list under any other policy is refused too, because nothing would ever read it — a node that declares a rescue slate and silently ignores it is the failure this config shape is `.strict()` against. Both messages name both keys. +- **A fallback that itself resolves to nobody degrades to `admin_rescue`.** The run is never killed and the record is never waved through by a policy whose author only asked for different people; the log says both that the fallback fired and that it found nobody. +- **This is on the node, not on the `manager` rung** — the node is already where emptiness is decided, and a fallback is wanted for every approver type, not one of them. +- **`os lint` names the new escape and keeps firing without it.** `approval-approvers-may-resolve-empty` still reports a manager-only slate even when a fallback is declared: the rule reads shape, and a static check can no more prove a `fallbackApprovers` list resolves than it can read `sys_user.manager_id`. A seeded manager chain remains the one silencer. diff --git a/content/docs/automation/approvals.mdx b/content/docs/automation/approvals.mdx index 2f309ae0cf4..a3e279f6df3 100644 --- a/content/docs/automation/approvals.mdx +++ b/content/docs/automation/approvals.mdx @@ -94,10 +94,15 @@ write it either. Where an identity carries `source: 'idp_provisioned'` the admin Setting the column is still something you have to *do*. Where `manager_id` is unset the rung expands to nobody, the request opens on a slate no one can act on, and under the default `lockRecord: true` the record stays locked — recoverable only by a platform or tenant admin -override. So set it for everyone who submits this request, or add a fallback approver that cannot -resolve empty — e.g. `{ type: 'org_membership_level', value: 'owner' }`. `os lint` reports the -shape at authoring time (`approval-approvers-may-resolve-empty`, `info`) and carries the same -remedy text, so the two cannot drift apart. +override. So set it for everyone who submits this request, or take one of the two escapes that +need no write to the column at all: add a second approver entry that cannot resolve empty — e.g. +`{ type: 'org_membership_level', value: 'owner' }` — or declare `onEmptyApprovers: 'fallback'` +on the node with a `fallbackApprovers` list, so an empty manager rung opens the request on those +people instead. Neither makes `manager` resolve; they make the failure survivable, which is a +different and independently useful thing. `os lint` reports the shape at authoring time +(`approval-approvers-may-resolve-empty`, `info`) and carries the same remedy text, so the two +cannot drift apart — and it keeps reporting it even with a fallback declared, because a static +check cannot read the column and cannot know the fallback resolves either. Approving is itself a gated action — model "may approve" as a capability (`approve_invoice`) the approver's permission set grants, and gate the approve action's `requiredPermissions` on it so the gate is enforced on **both** the UI and the server (ADR-0066 D4). @@ -273,12 +278,36 @@ empty slate you can see than a task the tenancy wall hides. A result may legitimately be **empty** (a present-but-empty field or variable); the node-level `onEmptyApprovers` policy decides what that means — `admin_rescue` (default: the request opens, a privileged admin takes over via -Reassign), `fail` (the node fails: an empty slate is a config bug), or +Reassign), `fail` (the node fails: an empty slate is a config bug), `auto_approve` (skip the request and continue down `approve` with -`output.autoApproved = true`; opt-in, since it waves the record through). A +`output.autoApproved = true`; opt-in, since it waves the record through), or +`fallback` (open the request on the people named in `fallbackApprovers`). A **missing** key (`vars.never_written`) is a loud error instead — guard genuinely-optional inputs with `has(vars.x) ? vars.x : []`. +`fallback` is the only one of the four that **names people**, so it is the only +one that turns an empty slate into a request someone can actually decide: + +```ts +{ + approvers: [{ type: 'manager' }], + onEmptyApprovers: 'fallback', + fallbackApprovers: [{ type: 'org_membership_level', value: 'owner' }], +} +``` + +`fallbackApprovers` takes the same entries as `approvers` — same types, same +`group` / `resolveAs` / `organization` keys — and is resolved by the same +expansion, OOO delegation included. The pairing is enforced in both directions: +`fallback` without a `fallbackApprovers` list is refused at authoring time, and +a `fallbackApprovers` list under any other policy is refused too, because +nothing would ever read it. + +Prefer a fallback target that cannot itself resolve empty (a literal `user`, or +the `owner` membership tier). If it does resolve empty the node degrades to +`admin_rescue` — the request still opens and the run is never killed — and the +log says both that the fallback fired and that it found nobody. + The "previous approver picks the next step's approvers" loop needs no record field at all: declare `decisionOutputs: ['next_reviewers']` on node A, have the approver decide with `{ outputs: { next_reviewers: ['u2', 'u3'] } }`, and let diff --git a/packages/lint/src/validate-approval-approvers.test.ts b/packages/lint/src/validate-approval-approvers.test.ts index 3a6f8a5d137..9d20b266169 100644 --- a/packages/lint/src/validate-approval-approvers.test.ts +++ b/packages/lint/src/validate-approval-approvers.test.ts @@ -327,6 +327,37 @@ describe('unset-manager dead-end (#16748)', () => { expect(finding.message).toContain('does not assert the slate IS empty'); }); + it("names the node-level escape — onEmptyApprovers: 'fallback' with fallbackApprovers", () => { + const [finding] = validateApprovalApprovers(managerOnly()); + expect(finding.hint).toContain("onEmptyApprovers: 'fallback'"); + expect(finding.hint).toContain('fallbackApprovers'); + // It is offered as a route that needs NO write to the column — the point + // of naming it beside an endpoint the operator may not be able to reach. + expect(finding.hint).toContain('needs no write to the column'); + // ⛔ And the pre-existing routes are not displaced by it. + expect(finding.hint).toContain('/api/v1/auth/admin/set-user-manager'); + expect(finding.hint).toContain("org_membership_level', value: 'owner'"); + }); + + // ⛔ The load-bearing negative: declaring the policy must NOT silence the + // finding. This rule reads SHAPE, and a `fallbackApprovers` list can itself + // resolve to nobody at runtime — which a static check cannot see either. A + // silencer here would be a claim the rule is not allowed to make. + it('KEEPS FIRING when the node declares the fallback policy — the rule reads shape', () => { + const stack = managerOnly(); + const cfg = (stack.flows as any)[0].nodes[1].config; + cfg.onEmptyApprovers = 'fallback'; + cfg.fallbackApprovers = [{ type: 'org_membership_level', value: 'owner' }]; + + const findings = validateApprovalApprovers(stack); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY); + expect(findings[0].severity).toBe('info'); + + // The one silencer stays the one silencer. + expect(validateApprovalApprovers(withSeededManagerChain(stack))).toEqual([]); + }); + // ── negative controls ────────────────────────────────────────────────── it('NEGATIVE: a populated manager chain in the stack emits nothing', () => { @@ -512,6 +543,27 @@ describe('expression approvers (#3447 P2)', () => { expect(findings[0].hint).toContain('admin_rescue'); }); + // The nudge enumerates the vocabulary, so it goes stale the moment the + // vocabulary widens. All four members, or an author picks from three. + it('enumerates every empty-slate policy in the nudge, fallback included', () => { + const [finding] = validateApprovalApprovers(stackWithConfig({ + approvers: [{ type: 'expression', value: 'vars.picked' }], + })); + for (const member of ['admin_rescue', 'fail', 'auto_approve', 'fallback']) { + expect(finding.hint, `nudge omits ${member}`).toContain(member); + } + expect(finding.hint).toContain('fallbackApprovers'); + }); + + it("accepts 'fallback' as an explicit empty policy — no nudge", () => { + const findings = validateApprovalApprovers(stackWithConfig({ + approvers: [{ type: 'expression', value: 'vars.picked' }], + onEmptyApprovers: 'fallback', + fallbackApprovers: [{ type: 'user', value: 'u_backstop' }], + })); + expect(findings).toEqual([]); + }); + it('errors on reserved decisionOutputs keys', () => { const findings = validateApprovalApprovers(stackWithConfig({ approvers: [{ type: 'user', value: 'u1' }], diff --git a/packages/lint/src/validate-approval-approvers.ts b/packages/lint/src/validate-approval-approvers.ts index 00303597354..3f4ec8a6a3c 100644 --- a/packages/lint/src/validate-approval-approvers.ts +++ b/packages/lint/src/validate-approval-approvers.ts @@ -156,6 +156,15 @@ const GROUP_ROUTED_TYPES = new Set(['position', 'team', 'department']); * "an operator can". So the finding stays, `stackWiresManagerChain` stays the * silencer, and the remedy is what was rewritten (#16678). * + * ⛔ THE SAME READING APPLIES to the node-level `onEmptyApprovers: 'fallback'` + * policy this remedy now names. A fallback does not make `manager` resolve; it + * makes the failure SURVIVABLE, and the two are independent. So it is added to + * the remedy as one more route the author can take — ⛔ never as a second + * silencer beside `stackWiresManagerChain`. Silencing on it would be a + * different claim from the one this rule is allowed to make: the rule reads + * SHAPE, and a declared `fallbackApprovers` list can itself resolve to nobody + * at runtime, which a static check cannot see either. + * * ⛔ DEPENDENCY — one carrier of the old assertion is still stale and is ⛔ NOT * fixed here: `ApproverType`'s `.describe()` in * `packages/spec/src/automation/approval.zod.ts` (rendered verbatim into the @@ -191,7 +200,9 @@ const MANAGER_ONLY_ROUTES = `platform declares the SCIM 'manager' attribute without projecting it onto the column, and its ` + `admin bulk import does not write it either — and where an identity carries ` + `source 'idp_provisioned' the admin operation refuses, leaving that directory the one surface ` + - `that authors its manager.`; + `that authors its manager. Or declare onEmptyApprovers: 'fallback' on the node, with a ` + + `fallbackApprovers list: an empty manager rung then opens the request on those people instead ` + + `of on a slot nobody can act on, which needs no write to the column at all.`; export type ApprovalApproverSeverity = 'error' | 'warning' | 'info'; @@ -512,8 +523,10 @@ export function validateApprovalApprovers(stack: AnyRec): ApprovalApproverFindin (locks ? `, and (lockRecord) the record stays locked with no in-product recovery.` : `.`), hint: `Make sure at least one target is always staffed, or add a guaranteed-staffed ` + - `fallback approver, e.g. { type: 'org_membership_level', value: 'owner' }. A request ` + - `that still lands empty is recoverable only by a platform/tenant admin override (#3424).`, + `approver entry, e.g. { type: 'org_membership_level', value: 'owner' }, or declare ` + + `onEmptyApprovers: 'fallback' with a fallbackApprovers list for the node to open on ` + + `when the groups come back empty. A request that still lands empty is recoverable ` + + `only by a platform/tenant admin override (#3424).`, }); } @@ -561,9 +574,9 @@ export function validateApprovalApprovers(stack: AnyRec): ApprovalApproverFindin (locks ? `, and (lockRecord) the record stays locked with no in-product recovery.` : `.`), hint: `${MANAGER_ONLY_REMEDY} ${MANAGER_ONLY_ROUTES} Populate it for everyone who submits ` + - `this request, or take the escape that needs none of that: add a fallback approver ` + - `which cannot resolve empty, e.g. { type: 'org_membership_level', value: 'owner' }. A ` + - `request that still lands empty is recoverable only by a platform/tenant admin override.`, + `this request, or take the other escape that needs none of that: add a second approver ` + + `entry which cannot resolve empty, e.g. { type: 'org_membership_level', value: 'owner' }. ` + + `A request that still lands empty is recoverable only by a platform/tenant admin override.`, }); } @@ -587,8 +600,9 @@ export function validateApprovalApprovers(stack: AnyRec): ApprovalApproverFindin `privileged admin can act).`, hint: `Declare the empty-slate policy explicitly: onEmptyApprovers: 'admin_rescue' (hold for ` + - `admin takeover), 'fail' (fail the node — config bug), or 'auto_approve' (wave through, ` + - `output.autoApproved = true).`, + `admin takeover), 'fail' (fail the node — config bug), 'auto_approve' (wave through, ` + + `output.autoApproved = true), or 'fallback' with a sibling fallbackApprovers list (open ` + + `the request on those people instead).`, }); } From fa9ba24043c65dde332cb6590c3f8d5040bdc417 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 16 Sep 2026 19:29:10 +0000 Subject: [PATCH 4/5] fix(spec): the approver-type description no longer denies a write surface that exists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `sys_user.manager_id` gained a dedicated admin operation, and this string still said the column "has no product write surface" — a sentence the generated reference renders once per shape that reuses the approver entry, so this change's new `fallbackApprovers` shape would have shipped a third copy of it. The sentence now describes the column and keeps pointing at the one authoritative remedy instead of asserting a fact of its own. Claude-Session: https://claude.ai/code/session_01KB5PFtxuy1x3dcR5gxudx6 Co-authored-by: Claude --- .../docs/references/automation/approval.mdx | 13 ++++++++- .../lint/src/validate-approval-approvers.ts | 16 +++++------ packages/spec/src/automation/approval.test.ts | 27 +++++++++++++++++++ packages/spec/src/automation/approval.zod.ts | 23 ++++++++++------ 4 files changed, 61 insertions(+), 18 deletions(-) diff --git a/content/docs/references/automation/approval.mdx b/content/docs/references/automation/approval.mdx index f16e1cd79f0..51ddfee6d45 100644 --- a/content/docs/references/automation/approval.mdx +++ b/content/docs/references/automation/approval.mdx @@ -72,7 +72,8 @@ const result = ApprovalDecision.parse(data); | **minApprovals** | `integer` | optional | Approvals required — total (quorum) or per group (per_group). Omitted ⇒ all resolvable approvers for quorum, 1 per group for per_group | | **lockRecord** | `boolean` | optional (default: `true`) | Lock the record from editing while pending | | **approvalStatusField** | `string` | optional | Business-object field to mirror request status onto | -| **onEmptyApprovers** | `Enum<'admin_rescue' \| 'fail' \| 'auto_approve'>` | optional (default: `"admin_rescue"`) | Behavior when no concrete approver resolves at node entry | +| **onEmptyApprovers** | `Enum<'admin_rescue' \| 'fail' \| 'auto_approve' \| 'fallback'>` | optional (default: `"admin_rescue"`) | Behavior when no concrete approver resolves at node entry — 'fallback' opens the request on fallbackApprovers instead | +| **fallbackApprovers** | `{ type: Enum<'manager' \| 'position' \| 'department' \| 'team' \| 'field' \| 'expression' \| …>; value?: string; resolveAs?: Enum<'user' \| 'department' \| 'position' \| 'team'>; group?: string; … }[]` | optional | Approvers the request opens on when onEmptyApprovers is 'fallback' | | **decisionOutputs** | `(string \| { key: string; label?: string; type?: Enum<'text' \| 'user' \| 'department' \| 'position' \| 'team'>; multiple?: boolean; … })[]` | optional | Author-declared decision outputs — bare keys or typed `{ key, type, multiple }` declarations | | **escalation** | `{ enabled: boolean; timeoutHours: number; action: Enum<'reassign' \| 'auto_approve' \| 'auto_reject' \| 'notify'>; escalateTo?: string; … }` | optional | Per-node SLA escalation | | **maxRevisions** | `integer` | optional (default: `3`) | Max send-backs for revision before auto-reject (0 = send-back disabled) | @@ -87,6 +88,16 @@ const result = ApprovalDecision.parse(data); | **group** | `string` | optional | Group label for per_group sign-off (e.g. "legal", "finance") | | **organization** | `string` | optional | ADR-0105 D9 — organization whose directory resolves this approver: `$root` (group org), `$parent` (one level up), or an organization slug. Omitted = the request's own organization. | +### Nested Shape: `ApprovalNodeConfig.fallbackApprovers[number]` + +| Property | Type | Required | Description | +| :--- | :--- | :--- | :--- | +| **type** | `Enum<'manager' \| 'position' \| 'department' \| 'team' \| 'field' \| 'expression' \| …>` | ✅ | Approval step approver type. `manager` is a directory-sync dependency rather than something an author configures here: it resolves the submitter's `sys_user.manager_id` at runtime, and that column has no product write surface, so until an operator populates it from outside the product a manager step resolves to nobody and the request waits. `os lint` reports that at authoring time as `approval-approvers-may-resolve-empty` and carries the graded population routes and the full remedy; the Approvals guide states the same remedy in prose. | +| **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") | +| **organization** | `string` | optional | ADR-0105 D9 — organization whose directory resolves this approver: `$root` (group org), `$parent` (one level up), or an organization slug. Omitted = the request's own organization. | + ### Nested Shape: `ApprovalNodeConfig.decisionOutputs[number]` | Property | Type | Required | Description | diff --git a/packages/lint/src/validate-approval-approvers.ts b/packages/lint/src/validate-approval-approvers.ts index 3f4ec8a6a3c..cf7cb3a81a8 100644 --- a/packages/lint/src/validate-approval-approvers.ts +++ b/packages/lint/src/validate-approval-approvers.ts @@ -165,16 +165,14 @@ const GROUP_ROUTED_TYPES = new Set(['position', 'team', 'department']); * SHAPE, and a declared `fallbackApprovers` list can itself resolve to nobody * at runtime, which a static check cannot see either. * - * ⛔ DEPENDENCY — one carrier of the old assertion is still stale and is ⛔ NOT - * fixed here: `ApproverType`'s `.describe()` in + * ⛔ DEPENDENCY — the other two carriers of this assertion are now correct and + * must stay that way: `ApproverType`'s `.describe()` in * `packages/spec/src/automation/approval.zod.ts` (rendered verbatim into the - * generated `content/docs/references/automation/approval.mdx`) still says the - * column "has no product write surface". That edit is `packages/spec`, which - * this change is fenced out of; it is reported to the PM for the `domain:spec` - * seat. The third carrier, the `manager` callout in - * `content/docs/automation/approvals.mdx`, IS updated in this same change. - * Neither RESTATES the remedy — both point back here, which is why there is - * still exactly one copy to edit. + * generated `content/docs/references/automation/approval.mdx`, once per shape + * that reuses the approver entry — so a stale copy there multiplies) and the + * `manager` callout in `content/docs/automation/approvals.mdx`. Neither + * RESTATES the remedy — both point back here, which is why there is still + * exactly one copy to edit. */ // ⛔ The tracker ids stay in the comments above and never in this string: // `check:doc-authoring` Rule 3 — a runtime string reaches authors, operators and diff --git a/packages/spec/src/automation/approval.test.ts b/packages/spec/src/automation/approval.test.ts index fb3736c7770..a4554fe7ef5 100644 --- a/packages/spec/src/automation/approval.test.ts +++ b/packages/spec/src/automation/approval.test.ts @@ -83,6 +83,33 @@ describe('ApproverType', () => { // `delegated_admin` tier (ADR-0105 D8) could not be authored as an approver. // The list is now DERIVED from `BUILTIN_MEMBERSHIP_ROLES`; these pins keep the // derivation from ever being silently replaced by a copy again. +// The enum's `.describe()` reaches authors twice over: `os lint` is not its only +// reader, the generated reference renders it verbatim ONCE PER SHAPE that reuses +// the approver entry — so a stale sentence there multiplies rather than sits +// still. It is a POINTER at the remedy by design; what it must never do is make +// a claim of its own that the platform contradicts. +describe('ApproverType.describe() points at the remedy without asserting a stale fact', () => { + const doc = () => ApproverType.description ?? ''; + + it('⛔ does not deny a write surface that exists', () => { + // `sys_user.manager_id` gained a dedicated admin operation. An author who + // reads "no product write surface" stops looking for it. + expect(doc()).not.toContain('no product write surface'); + expect(doc()).not.toContain('from outside the product'); + }); + + it('still routes the reader to the one authoritative remedy', () => { + expect(doc()).toContain('approval-approvers-may-resolve-empty'); + expect(doc()).toContain('os lint'); + // ⛔ And still does not restate it — one copy, in the lint rule. + expect(doc()).not.toContain('/api/v1/auth/admin/set-user-manager'); + }); + + it('names the node-level escape this widening added', () => { + expect(doc()).toContain('onEmptyApprovers'); + }); +}); + describe('ORG_MEMBERSHIP_LEVELS derives from BUILTIN_MEMBERSHIP_ROLES', () => { it('is the same list, in the same display order, as the sys_member.role vocabulary', () => { expect([...ORG_MEMBERSHIP_LEVELS]).toEqual([...BUILTIN_MEMBERSHIP_ROLES]); diff --git a/packages/spec/src/automation/approval.zod.ts b/packages/spec/src/automation/approval.zod.ts index 3750696713e..d91ceb7ec50 100644 --- a/packages/spec/src/automation/approval.zod.ts +++ b/packages/spec/src/automation/approval.zod.ts @@ -83,19 +83,26 @@ export const ApproverType = z.enum([ // `packages/lint/src/validate-approval-approvers.ts`, mirrored for readers by // the Approvals guide; a third copy here would be one more line to keep in // step, and a pointer cannot drift into disagreement with what it points at. - // That file's `⛔ DEPENDENCY` docblock now names this string among the lines - // that go stale if `manager_id` ever gains a product write surface. + // ⛔ It said "that column has no product write surface" until the write + // surface landed — the exact staleness the pointer discipline above exists to + // make cheap to repair, and the reason the sentence now describes the COLUMN + // (not a profile field an author edits) rather than the platform's inventory + // of writers. An author who reads "no write surface" stops looking for the + // admin operation that does exist, which is worse than no sentence at all. + // The generated reference renders this string once per carrier, so a stale + // copy multiplies silently every time a new shape reuses the approver entry. // // ⛔ Tracker ids stay out of the string itself — it reaches authors and // generated surfaces, neither of whom can resolve one (`check:doc-authoring` // Rule 3). .describe( - 'Approval step approver type. `manager` is a directory-sync dependency rather than something ' + - "an author configures here: it resolves the submitter's `sys_user.manager_id` at runtime, and " + - 'that column has no product write surface, so until an operator populates it from outside the ' + - 'product a manager step resolves to nobody and the request waits. `os lint` reports that at ' + - 'authoring time as `approval-approvers-may-resolve-empty` and carries the graded population ' + - 'routes and the full remedy; the Approvals guide states the same remedy in prose.', + 'Approval step approver type. `manager` is resolved from the directory rather than configured ' + + "here: it reads the submitter's `sys_user.manager_id` at runtime, and that column is not a " + + 'profile field an author edits, so until an operator populates it through one of the routes ' + + '`os lint` names, a manager step resolves to nobody and the request waits. `os lint` reports ' + + 'that at authoring time as `approval-approvers-may-resolve-empty` and carries the graded ' + + 'population routes and the full remedy, the node-level `onEmptyApprovers` fallback included; ' + + 'the Approvals guide states the same remedy in prose.', ); export type ApproverType = z.input; From 6d3f5a7946ed0159944a6ed72a68367326f5d6c6 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 16 Sep 2026 19:35:23 +0000 Subject: [PATCH 5/5] chore(spec): regenerate the automation reference for the corrected approver-type text Claude-Session: https://claude.ai/code/session_01KB5PFtxuy1x3dcR5gxudx6 Co-authored-by: Claude --- content/docs/references/automation/approval.mdx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/content/docs/references/automation/approval.mdx b/content/docs/references/automation/approval.mdx index 51ddfee6d45..9694d9f72d1 100644 --- a/content/docs/references/automation/approval.mdx +++ b/content/docs/references/automation/approval.mdx @@ -52,7 +52,7 @@ const result = ApprovalDecision.parse(data); | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **type** | `Enum<'manager' \| 'position' \| 'department' \| 'team' \| 'field' \| 'expression' \| 'org_membership_level' \| 'role' \| 'user' \| 'queue'>` | ✅ | Approval step approver type. `manager` is a directory-sync dependency rather than something an author configures here: it resolves the submitter's `sys_user.manager_id` at runtime, and that column has no product write surface, so until an operator populates it from outside the product a manager step resolves to nobody and the request waits. `os lint` reports that at authoring time as `approval-approvers-may-resolve-empty` and carries the graded population routes and the full remedy; the Approvals guide states the same remedy in prose. | +| **type** | `Enum<'manager' \| 'position' \| 'department' \| 'team' \| 'field' \| 'expression' \| 'org_membership_level' \| 'role' \| 'user' \| 'queue'>` | ✅ | Approval step approver type. `manager` is resolved from the directory rather than configured here: it reads the submitter's `sys_user.manager_id` at runtime, and that column is not a profile field an author edits, so until an operator populates it through one of the routes `os lint` names, a manager step resolves to nobody and the request waits. `os lint` reports that at authoring time as `approval-approvers-may-resolve-empty` and carries the graded population routes and the full remedy, the node-level `onEmptyApprovers` fallback included; the Approvals guide states the same remedy in prose. | | **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") | @@ -82,7 +82,7 @@ const result = ApprovalDecision.parse(data); | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **type** | `Enum<'manager' \| 'position' \| 'department' \| 'team' \| 'field' \| 'expression' \| …>` | ✅ | Approval step approver type. `manager` is a directory-sync dependency rather than something an author configures here: it resolves the submitter's `sys_user.manager_id` at runtime, and that column has no product write surface, so until an operator populates it from outside the product a manager step resolves to nobody and the request waits. `os lint` reports that at authoring time as `approval-approvers-may-resolve-empty` and carries the graded population routes and the full remedy; the Approvals guide states the same remedy in prose. | +| **type** | `Enum<'manager' \| 'position' \| 'department' \| 'team' \| 'field' \| 'expression' \| …>` | ✅ | Approval step approver type. `manager` is resolved from the directory rather than configured here: it reads the submitter's `sys_user.manager_id` at runtime, and that column is not a profile field an author edits, so until an operator populates it through one of the routes `os lint` names, a manager step resolves to nobody and the request waits. `os lint` reports that at authoring time as `approval-approvers-may-resolve-empty` and carries the graded population routes and the full remedy, the node-level `onEmptyApprovers` fallback included; the Approvals guide states the same remedy in prose. | | **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") | @@ -92,7 +92,7 @@ const result = ApprovalDecision.parse(data); | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **type** | `Enum<'manager' \| 'position' \| 'department' \| 'team' \| 'field' \| 'expression' \| …>` | ✅ | Approval step approver type. `manager` is a directory-sync dependency rather than something an author configures here: it resolves the submitter's `sys_user.manager_id` at runtime, and that column has no product write surface, so until an operator populates it from outside the product a manager step resolves to nobody and the request waits. `os lint` reports that at authoring time as `approval-approvers-may-resolve-empty` and carries the graded population routes and the full remedy; the Approvals guide states the same remedy in prose. | +| **type** | `Enum<'manager' \| 'position' \| 'department' \| 'team' \| 'field' \| 'expression' \| …>` | ✅ | Approval step approver type. `manager` is resolved from the directory rather than configured here: it reads the submitter's `sys_user.manager_id` at runtime, and that column is not a profile field an author edits, so until an operator populates it through one of the routes `os lint` names, a manager step resolves to nobody and the request waits. `os lint` reports that at authoring time as `approval-approvers-may-resolve-empty` and carries the graded population routes and the full remedy, the node-level `onEmptyApprovers` fallback included; the Approvals guide states the same remedy in prose. | | **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") | @@ -123,7 +123,7 @@ const result = ApprovalDecision.parse(data); ## ApproverType -Approval step approver type. `manager` is a directory-sync dependency rather than something an author configures here: it resolves the submitter's `sys_user.manager_id` at runtime, and that column has no product write surface, so until an operator populates it from outside the product a manager step resolves to nobody and the request waits. `os lint` reports that at authoring time as `approval-approvers-may-resolve-empty` and carries the graded population routes and the full remedy; the Approvals guide states the same remedy in prose. +Approval step approver type. `manager` is resolved from the directory rather than configured here: it reads the submitter's `sys_user.manager_id` at runtime, and that column is not a profile field an author edits, so until an operator populates it through one of the routes `os lint` names, a manager step resolves to nobody and the request waits. `os lint` reports that at authoring time as `approval-approvers-may-resolve-empty` and carries the graded population routes and the full remedy, the node-level `onEmptyApprovers` fallback included; the Approvals guide states the same remedy in prose. ### Allowed Values