diff --git a/.changeset/approval-approvers-manager-rung-may-resolve-empty.md b/.changeset/approval-approvers-manager-rung-may-resolve-empty.md new file mode 100644 index 0000000000..e4f6b467f8 --- /dev/null +++ b/.changeset/approval-approvers-manager-rung-may-resolve-empty.md @@ -0,0 +1,18 @@ +--- +"@objectstack/lint": minor +--- + +`approval-approvers-may-resolve-empty` now covers the `manager` rung, not just the group-routed ones. + +The rule exists for the empty-slate dead-end (#3424): an approver slate that resolves to nobody, with `lockRecord` turning that into a stranded record. It reasoned about `position` / `team` / `department` and said nothing about `{ type: 'manager' }` — which has the same failure shape and a strictly worse cause. A `position` rung resolves empty because the position is unstaffed, and an operator can staff it. A `manager` rung resolves empty because `sys_user.manager_id` is unset, and an operator **cannot** set it: the managed-update whitelist for `sys_user` is exactly `{name, image, locale}` (ADR-0092), the auth admin endpoints do not accept the column, and the Console renders no field for it. So the rule warned about the rung an author can rescue and stayed silent on the one they cannot — and `manager` is the canonical first rung of a tiered approval ladder, so the silent case was also the common one. + +- **What fires.** A node whose approver slate is made up ENTIRELY of `{ type: 'manager' }` rungs now draws one `approval-approvers-may-resolve-empty` finding, at the same `info` tier as its `position` sibling. `manager` resolves through `sys_user.manager_id` of the record's owner and yields nobody when that column is unset; when nothing else is on the node, the request waits forever, and under the default `lockRecord` the record stays locked. +- **What it does not claim.** The message states in as many words that this is a static check which cannot read the column, and that it does not assert the slate IS empty — it reports that nothing else on the node can approve if it is. A lint rule must not claim a runtime fact it did not read. +- **The remedy it prescribes, with the routes graded rather than listed.** An exact diagnosis whose prescription cannot be carried out is worse than no prescription, so the hint separates what this platform provides from what it does not. A **seed, or any other system-context write**, populates the column here — both write guards gate on `isUserContextWrite` (`userId && !isSystem`), so a system-context write bypasses the managed-update whitelist by construction. **SCIM provisioning and directory sync** are named too, because a deployment running a real one may well populate the column through it — but named as a path the deployment itself supplies: this repo declares the SCIM Enterprise `manager` attribute without projecting it onto the column, and the admin bulk import does not write it either (`SYS_USER_IMPORT_UPDATE_FIELDS` is `{name, image, locale}` plus `phone_number` and `role`, and `manager_id` is listed there among the admin-surface-only columns). Editing the user in the Console is explicitly ruled out, since it cannot write the column at all. And the escape that depends on none of this stays on offer: add a fallback approver that cannot resolve empty, such as `{ type: 'org_membership_level', value: 'owner' }`. +- **When it stays quiet — and on which surface.** A stack whose own seed data wires `sys_user.manager_id` on any seeded row has shown the linter that it populates the column, and the advisory is suppressed. Seed rows are the only manager-chain evidence a stack can carry, so that is the whole of what this check reads on the question. ⚠️ That suppression is **CLI-side only**. The runtime publish gate hands rules a `RuntimeStackContext` whose collections are fixed — `objects`, `permissions`, `books`, `datasets`, `pages`, and no `data` — so a Studio publish of a manager-only flow carries no seeds to read and draws the advisory however the tenant's users are wired. That is a surface asymmetry, not a broken suppressor: an `info` finding never blocks a publish, it rides the 2xx `advisories`. Noted here so a reader who seeds correctly and still sees it fire on publish does not go looking for a bug in the rule. + +Existing verdicts are unchanged. The new arm is scoped to slates that are entirely `manager` rungs, which keeps it disjoint from the group-routed arm by construction — no node can draw both findings — and leaves every `position` verdict exactly as it was, mixed slates included: a `[position, manager]` node stays silent, as it is pinned to. + +This is a purely additive widening of a published package's public surface — the rule begins covering a case it was silent on — so it is graded `minor`, the floor that act carries regardless of the commit type. + +No severity moved. The finding is `info`, so it lands in the advisory channel on every consumer: `os lint` renders it as a suggestion and its exit code is unchanged (a suggestion does not fail a run even under `--strict`), and the runtime publish gate returns it on the 2xx `advisories` array rather than refusing the write. What changes is the report, not any verdict. diff --git a/packages/lint/src/validate-approval-approvers.test.ts b/packages/lint/src/validate-approval-approvers.test.ts index b603426c19..9fe0c774e4 100644 --- a/packages/lint/src/validate-approval-approvers.test.ts +++ b/packages/lint/src/validate-approval-approvers.test.ts @@ -220,6 +220,187 @@ describe('validateApprovalApprovers', () => { }); }); +// ── unset-manager dead-end (#16748) ────────────────────────────────────── +// +// The #3424 arm above reasons about `position`/`team`/`department` and was +// silent on `{ type: 'manager' }` — measured on the parent commit as +// `git grep -c "'manager'"` = 0 against `'position'` = 4 in the rule file, and +// confirmed behaviourally: a manager-only node returned `[]`. +// +// Every count here carries its controls, because an implementation that simply +// always fires satisfies the positive case on its own and is indistinguishable +// without them. + +describe('unset-manager dead-end (#16748)', () => { + const managerOnly = () => stackWithApprovers([{ type: 'manager' }]); + + /** A stack that demonstrably wires `sys_user.manager_id` in its own seeds. */ + const withSeededManagerChain = (stack: Record) => { + stack.data = [{ + object: 'sys_user', + mode: 'upsert', + externalId: 'name', + records: [ + { name: 'ceo' }, // top of the chain — no manager, correctly + { name: 'ic', manager_id: 'ceo' }, + ], + }]; + return stack; + }; + + it('FIRES on a node whose whole slate is { type: manager }, at info', () => { + const findings = validateApprovalApprovers(managerOnly()); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY); + // ⛔ Tier boundary: the same advisory tier as its `position` sibling. An + // `error` here would red every stack that authors a manager rung today. + expect(findings[0].severity).toBe('info'); + expect(findings[0].path).toBe('flows[0].nodes[1].config.approvers'); + expect(findings[0].message).toContain('locked'); // lockRecord defaults true + }); + + it('names the REAL remedy — provisioning, not the Console', () => { + const [finding] = validateApprovalApprovers(managerOnly()); + // The prescription an operator can actually carry out (#16678: the column + // has no product write surface). + expect(finding.hint).toContain('SCIM'); + expect(finding.hint).toContain('import'); + expect(finding.hint).toContain('directory sync'); + expect(finding.hint).toContain('no product write surface'); + // ⛔ And it must not send them to a surface that cannot write it. The word + // "Console" appears only inside that denial, never as an instruction. + expect(finding.hint).toContain('never populated by editing the user in the Console'); + expect(finding.hint).not.toMatch(/[Ee]dit .{0,40}in the Console\b(?!.*NOT)/); + // It still offers the escape that does not depend on #16678 at all. + expect(finding.hint).toContain("org_membership_level', value: 'owner'"); + }); + + it('GRADES the routes — an exact diagnosis whose remedy cannot be carried out is worse than none', () => { + // A remedy that names a route with no writer is the #17037 shape. The three + // routes are measured against this tree, so the hint must SEPARATE the one + // that works here from the ones that need the deployment's own provisioning. + const [finding] = validateApprovalApprovers(managerOnly()); + + // The route with a demonstrated writer: a system-context write bypasses the + // managed-update whitelist (`isUserContextWrite` is `userId && !isSystem`). + expect(finding.hint).toContain('written by a seed, or by any other system-context write'); + expect(finding.hint).toContain('bypasses the managed-update whitelist'); + + // ⛔ The two that are NOT this repo's to offer must be marked as the + // deployment's own, and the hint must say WHY rather than merely hedging. + expect(finding.hint).toContain('a provisioning path your own deployment supplies'); + expect(finding.hint).toContain("declares the SCIM 'manager' attribute without projecting it"); + expect(finding.hint).toContain('admin bulk import does not write it either'); + + // ⛔ And they must not be deleted: a deployment running a real directory + // sync may well populate the column, and the defect was presenting all + // three as equally available, never naming them at all. + expect(finding.hint).toContain('SCIM provisioning and directory sync can populate it'); + }); + + it('does not claim a runtime fact it did not read', () => { + const [finding] = validateApprovalApprovers(managerOnly()); + expect(finding.message).toContain('a static check cannot read that column'); + expect(finding.message).toContain('does not assert the slate IS empty'); + }); + + // ── negative controls ────────────────────────────────────────────────── + + it('NEGATIVE: a populated manager chain in the stack emits nothing', () => { + expect(validateApprovalApprovers(withSeededManagerChain(managerOnly()))).toEqual([]); + }); + + it('NEGATIVE: a stack authoring neither rung emits nothing', () => { + expect(validateApprovalApprovers(stackWithApprovers([{ type: 'user', value: 'u1' }]))).toEqual([]); + expect(validateApprovalApprovers({ flows: [] })).toEqual([]); + expect(validateApprovalApprovers({})).toEqual([]); + }); + + it('NEGATIVE: a fallback that cannot resolve empty silences it', () => { + expect(validateApprovalApprovers(stackWithApprovers([ + { type: 'manager' }, + { type: 'org_membership_level', value: 'owner' }, + ]))).toEqual([]); + expect(validateApprovalApprovers(stackWithApprovers([ + { type: 'manager' }, + { type: 'user', value: 'u1' }, + ]))).toEqual([]); + }); + + // ── the suppressor's own controls ────────────────────────────────────── + + it('the seed suppressor discriminates: it reads sys_user.manager_id and only that', () => { + // FIRING control — sys_user rows that carry no manager_id suppress nothing. + const noChain = managerOnly(); + noChain.data = [{ object: 'sys_user', mode: 'upsert', records: [{ name: 'ic' }] }]; + expect(validateApprovalApprovers(noChain)).toHaveLength(1); + + // NONSENSE control — the same column on some OTHER object is not evidence + // about `sys_user`, and an empty / malformed `data` is not evidence either. + const wrongObject = managerOnly(); + wrongObject.data = [{ object: 'sys_team', mode: 'upsert', records: [{ name: 't', manager_id: 'ceo' }] }]; + expect(validateApprovalApprovers(wrongObject)).toHaveLength(1); + + const junk = managerOnly(); + junk.data = ['garbage', null, { object: 'sys_user' }, { object: 'sys_user', records: 'oops' }]; + expect(validateApprovalApprovers(junk)).toHaveLength(1); + + // And a blank string is not a populated chain. + const blank = managerOnly(); + blank.data = [{ object: 'sys_user', records: [{ name: 'ic', manager_id: ' ' }] }]; + expect(validateApprovalApprovers(blank)).toHaveLength(1); + }); + + // ── regression controls: the `position` arm is untouched ─────────────── + + it('REGRESSION: the position arm keeps its own verdict and its own message', () => { + const positionOnly = validateApprovalApprovers(stackWithApprovers([ + { type: 'position', value: 'exec' }, + ])); + expect(positionOnly).toHaveLength(1); + expect(positionOnly[0].message).toContain('routes to a group (position/team/department)'); + expect(positionOnly[0].message).not.toContain('manager_id'); + + // The mixed slate this package has always pinned as silent stays silent — + // this arm is scoped to slates that are ENTIRELY manager rungs. + expect(validateApprovalApprovers(stackWithApprovers([ + { type: 'position', value: 'exec' }, + { type: 'manager' }, + ]))).toEqual([]); + }); + + it('the two arms are disjoint — no node ever draws both findings', () => { + for (const approvers of [ + [{ type: 'manager' }], + [{ type: 'manager' }, { type: 'manager', value: 'requested_by' }], + [{ type: 'position', value: 'exec' }], + [{ type: 'position', value: 'exec' }, { type: 'team', value: 't1' }], + [{ type: 'position', value: 'exec' }, { type: 'manager' }], + ]) { + const hits = validateApprovalApprovers(stackWithApprovers(approvers)) + .filter((f) => f.rule === APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY); + expect(hits.length).toBeLessThanOrEqual(1); + } + }); + + it('a multi-rung manager ladder is one finding, not one per rung', () => { + const findings = validateApprovalApprovers(stackWithApprovers([ + { type: 'manager' }, + { type: 'manager', value: 'requested_by' }, + ])); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY); + }); + + it('drops the record-lock clause when lockRecord is false', () => { + const stack = managerOnly(); + (stack.flows as any)[0].nodes[1].config.lockRecord = false; + const findings = validateApprovalApprovers(stack); + expect(findings).toHaveLength(1); + expect(findings[0].message).not.toContain('locked'); + }); +}); + // ── #3447 P2: expression approvers / decision outputs ───────────────────── describe('expression approvers (#3447 P2)', () => { diff --git a/packages/lint/src/validate-approval-approvers.ts b/packages/lint/src/validate-approval-approvers.ts index 7b4ebfdc99..220014cf7a 100644 --- a/packages/lint/src/validate-approval-approvers.ts +++ b/packages/lint/src/validate-approval-approvers.ts @@ -21,6 +21,7 @@ * | approval-approver-type-unknown | warning | contract-first (PD #12) | * | approval-escalation-reassign-no-target | warning | silent notify degradation | * | approval-approvers-may-resolve-empty | info | empty-position dead-end (#3424) | + * | approval-approvers-may-resolve-empty | info | unset-manager dead-end (#16748) | * | approval-expression-invalid | error/info | #3447 P2 closed-root expressions | * | approval-expression-no-empty-policy | info | #3447 P2 empty-slate policy | * | approval-decision-outputs-reserved | error | #3447 P2 resume envelope | @@ -84,9 +85,81 @@ const RESERVED_OUTPUT_KEYS = new Set(['decision', 'requestId']); * tiers, and the opaque `queue` are deliberately excluded: any of them present * signals the author has a non-group route, so the node isn't purely * group-gated. + * + * ⛔ `manager` stays OUT of this set and is judged by its own arm below + * ({@link MANAGER_ONLY_REMEDY}), for two reasons that are not interchangeable: + * the group message ("routes to a group (position/team/department)") would be + * false about it, and the REMEDY differs — an unstaffed position is staffed + * in-product, an unset `sys_user.manager_id` is not (#16748). Folding `manager` + * in here would also change a `position` verdict this package pins: a + * `[position, manager]` node is silent today, and that pin is the existing + * arm's contract, not an oversight to sweep up. */ const GROUP_ROUTED_TYPES = new Set(['position', 'team', 'department']); +/** + * How an operator actually populates `sys_user.manager_id` (#16748 / #16678). + * + * ⚠️ NOT "edit the user in the Console". The column has NO product write + * surface today: `getManagedUpdateWhitelist('sys_user')` is exactly + * `{name, image, locale}` (ADR-0092 platform-object write narrowing), the auth + * admin endpoints do not accept it, and the Console renders no field for it. + * + * ⛔ AND a remedy naming a route that does not exist is worse than no remedy — + * an exact diagnosis whose prescription cannot be carried out (#17037). So the + * routes are GRADED here rather than listed, and each grade is a measurement + * against this tree with a discriminating control beside it: + * + * - Seed, or any other system-context write — AVAILABLE HERE. Both write + * guards gate on `isUserContextWrite`, spelled identically in each: + * `Boolean(userId) && isSystem !== true` (plugin-security + * `system-write-guard.ts`, plugin-auth `identity-write-guard.ts`). A + * system-context write therefore bypasses the managed-update whitelist by + * construction. This is the one route with a demonstrated writer in-repo. + * - SCIM — NOT here. The Enterprise `manager` attribute IS declared + * (`scim.zod.ts`), but no non-test file under `packages/plugins` or + * `packages/runtime` projects it onto the column: measured 0, against a + * control (`SysScimGroup`) the same scan does find, so the scan + * discriminates. + * - Admin bulk import — NOT here. `admin-import-users.ts` matches + * `manager_id` 0 times against a control of `phone_number` 8, and + * `SYS_USER_IMPORT_UPDATE_FIELDS` is `{name, image, locale}` plus + * `phone_number` and `role`. `sys-user-writable-fields.ts` lists + * `manager_id` among the admin-surface-only columns, so the omission is + * deliberate and ⛔ not an oversight to route around. + * + * ⇒ SCIM and directory sync stay NAMED, because a deployment running a real + * one may well populate the column through it — but named as something the + * operator's own provisioning supplies, ⛔ never as something this repo gives + * them. + * + * ⛔ DEPENDENCY — #16678 holds the open question of whether `manager_id` should + * GAIN a product write surface. If it ever does, these strings are the lines + * that go stale: it would then be wrong to tell an author their only route is + * a system-context write. Update them in the same change that opens the write + * surface — and re-take the three grades above, which are readings of this + * tree, not standing facts. + */ +// ⛔ 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 +// generated surfaces, none of whom can resolve `#NNNN`. The reader who can +// resolve it is reading this source. +const MANAGER_ONLY_REMEDY = + `sys_user.manager_id has no product write surface — the data API's managed-update whitelist is ` + + `{name, image, locale}, the auth admin endpoints do not accept the column and the Console ` + + `renders no field for it, so it is never populated by editing the user in the Console.`; + +/** + * The routes an operator can actually take, GRADED — the measurement behind + * each grade is in {@link MANAGER_ONLY_REMEDY}'s docblock. + */ +const MANAGER_ONLY_ROUTES = + `On this platform the column is written by a seed, or by any other system-context write, which ` + + `bypasses the managed-update whitelist. SCIM provisioning and directory sync can populate it ` + + `too, but only through a provisioning path your own deployment supplies: this platform declares ` + + `the SCIM 'manager' attribute without projecting it onto the column, and its admin bulk import ` + + `does not write it either.`; + export type ApprovalApproverSeverity = 'error' | 'warning' | 'info'; export interface ApprovalApproverFinding { @@ -129,6 +202,53 @@ const TYPE_FIX: Record = { bu: 'department', }; +/** + * Does this stack itself show that it wires `sys_user.manager_id`? + * + * The ONLY manager-chain evidence a stack can carry is its own seed rows + * (`stack.data` — `SeedSchema[]`, `{ object, records, … }`), so this is the + * whole of what a static check may read on the question. It reads it and says + * no more than it read: a stack that seeds a `sys_user` row carrying a + * non-empty `manager_id` demonstrably knows about the column and populates it, + * so the advisory below would be noise there. A stack that seeds none has given + * the linter nothing, which is not the same as the column being unset at + * runtime — hence the advisory's wording, which flags the SHAPE and explicitly + * does not assert the slate is empty. + * + * ⛔ This is deliberately NOT "every seeded user has a manager": the top of any + * real reporting chain legitimately has none, so an all-rows test would fire on + * a correctly-seeded stack forever. Nor does a seeded chain prove anything + * about users who arrive later by sign-up or SCIM — which is exactly why the + * finding it suppresses is `info`, not an error. + * + * ⚠️ SURFACE ASYMMETRY, so nobody reads a correct suppressor as broken: this + * silences the advisory on the CLI only. The runtime publish gate hands rules a + * `RuntimeStackContext`, whose collections are fixed by + * `CONTEXT_STACK_KEY_ORDER` in `runtime-gate.ts` — `objects`, `permissions`, + * `books`, `datasets`, `pages`, and NO `data`. So a Studio publish of a + * manager-only flow carries no seeds to read, `stack.data` is absent, and the + * advisory is drawn no matter how the tenant's users are wired. That is + * acceptable rather than a bug: an `info` finding never blocks a publish, it + * rides the 2xx `advisories`. ⛔ Do not "fix" it by weakening the arm — the + * repair, if one is ever wanted, is a context collection the gate does not + * carry today, which is a runtime-gate decision and not this rule's. + */ +function stackWiresManagerChain(stack: AnyRec): boolean { + const seeds = Array.isArray(stack.data) ? (stack.data as AnyRec[]) : []; + for (const seed of seeds) { + if (!seed || typeof seed !== 'object') continue; + if (seed.object !== 'sys_user') continue; + const rows = Array.isArray(seed.records) ? (seed.records as unknown[]) : []; + for (const row of rows) { + if (!row || typeof row !== 'object') continue; + const managerId = (row as AnyRec).manager_id; + if (typeof managerId === 'string' && managerId.trim() !== '') return true; + if (typeof managerId === 'number') return true; + } + } + return false; +} + /** * Validate the approvers of every Approval node in the stack's flows. * Returns findings (empty = clean). @@ -139,6 +259,9 @@ export function validateApprovalApprovers(stack: AnyRec): ApprovalApproverFindin const flows = recordsOf(stack.flows); const validTypes = new Set(ApproverType.options); + // Stack-level, so it is read once and every node is judged on the same + // evidence — a per-node re-read would let two identical nodes disagree. + const managerChainWired = stackWiresManagerChain(stack); for (let fi = 0; fi < flows.length; fi++) { const flow = flows[fi]; @@ -361,6 +484,56 @@ export function validateApprovalApprovers(stack: AnyRec): ApprovalApproverFindin }); } + // Unset-manager dead-end (#16748) — the empty-slate rule's other half. + // + // `{ type: 'manager' }` resolves through `lookupManager`: it reads + // `sys_user.manager_id` of `record[value] ?? record.owner_id` and returns + // NULL when the column is unset, so the approver contributes nobody. When + // the whole slate is manager rungs that is the same empty-slate dead-end + // the #3424 arm above flags for positions — with a strictly WORSE cause. + // An unstaffed position is an operator's to fix in-product; an unset + // `manager_id` is not (see {@link MANAGER_ONLY_REMEDY}). So the rule that + // exists for the recoverable rung was, until this arm, silent on the + // unrecoverable one — and `manager` is the canonical first rung of a + // tiered ladder, so the silent case was also the common one. + // + // ⛔ Scoped to slates that are ENTIRELY manager rungs, which keeps it + // disjoint from the arm above by construction (`manager` is not in + // GROUP_ROUTED_TYPES, so that arm's `every` is false whenever this one's + // is true — no node can draw both findings) and leaves every `position` + // verdict, mixed slates included, exactly as it was. + // + // Advisory (`info`), the same tier as its sibling: this reads the SHAPE. + // It does not read row data, and the message says so rather than + // asserting a slate is empty — a lint rule must not claim a runtime fact + // it did not read. `stackWiresManagerChain` is the one manager-chain fact + // a stack CAN put in front of it, and it silences the advisory. + if ( + !managerChainWired && + routable.length > 0 && + routable.every((a) => canonicalApproverType(String((a as AnyRec).type)) === 'manager') + ) { + const locks = (cfg as AnyRec).lockRecord !== false; // default true + findings.push({ + severity: 'info', + rule: APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY, + where, + path: `${nodePath}.config.approvers`, + message: + `every approver on this node is { type: 'manager' }, resolved at runtime from ` + + `sys_user.manager_id of the record's owner — a static check cannot read that column, ` + + `so this does not assert the slate IS empty; it reports that nothing else on the node ` + + `can approve if it is. Where manager_id is unset the expansion returns nobody, the ` + + `request resolves to an empty slate and waits forever` + + (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.`, + }); + } + // #3447 P2: a node with an `expression` approver resolves people from // runtime data — an empty result is far likelier than for static types // (a mid-flow field nobody wrote yet, an upstream output that came back