fix(lint): flag an approval slate that is entirely manager rungs

.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.

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<string, unknown>) => {
+    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)', () => {