From d25b46d0be68735b199adf5e98e2981558cd3658 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 03:02:21 +0000 Subject: [PATCH 1/4] fix(lint): flag an approval slate that is entirely manager rungs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `approval-approvers-may-resolve-empty` reasoned only about group-routed rungs (position/team/department) and was silent on `{ type: 'manager' }`, which has the same empty-slate failure shape and a worse cause: an unstaffed position is an operator's to fix in-product, an unset `sys_user.manager_id` is not — the managed-update whitelist is {name, image, locale}, the auth admin endpoints refuse the column and the Console renders no field for it. Adds a second arm under the same rule id and the same `info` tier. It is scoped to slates that are ENTIRELY manager rungs, which keeps it disjoint from the existing arm by construction and leaves every `position` verdict — mixed slates included — byte-identical. The message says plainly that the check is static and does not assert the slate is empty; the hint prescribes SCIM / import / directory sync rather than a Console edit that is not possible. A stack whose own seeds wire `sys_user.manager_id` silences it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012GKcPZbMoGq7WPzKLfRBTU --- .../src/validate-approval-approvers.test.ts | 158 ++++++++++++++++++ .../lint/src/validate-approval-approvers.ts | 118 +++++++++++++ 2 files changed, 276 insertions(+) diff --git a/packages/lint/src/validate-approval-approvers.test.ts b/packages/lint/src/validate-approval-approvers.test.ts index b603426c19..aa185e135e 100644 --- a/packages/lint/src/validate-approval-approvers.test.ts +++ b/packages/lint/src/validate-approval-approvers.test.ts @@ -220,6 +220,164 @@ 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('NOT 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('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..504e748581 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,38 @@ 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. So + * the only honest prescription is the provisioning path. + * + * ⛔ DEPENDENCY — #16678 holds the open question of whether `manager_id` should + * GAIN a product write surface. If it ever does, this string is the line that + * goes stale: it would then be wrong to tell an author their only route is + * provisioning. Update it in the same change that opens the write surface. + */ +const MANAGER_ONLY_REMEDY = + `sys_user.manager_id has no product write surface today (#16678) — 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 populated by SCIM provisioning, a seed / bulk import ` + + `or directory sync, NOT by editing the user in the Console.`; + export type ApprovalApproverSeverity = 'error' | 'warning' | 'info'; export interface ApprovalApproverFinding { @@ -129,6 +159,41 @@ 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. + */ +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 +204,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 +429,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} Populate it for everyone who submits this request, or add a ` + + `fallback approver that 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 (#3424).`, + }); + } + // #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 From a173e673e2e9497b195125aebbbe4b59d902c02b Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 03:13:49 +0000 Subject: [PATCH 2/4] fix(lint): keep tracker ids out of the manager rung's runtime prose MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `check:doc-authoring` Rule 3: a runtime string reaches authors, operators and generated surfaces, none of whom can resolve a bare id. The anchors move to the adjacent comments, where the reader who can resolve them is already reading the source. Also adds the changeset — the new advisory prose reaches four published bundles (dist/index.{js,cjs}, dist/runtime.{js,cjs}), all inside the package's `files`. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012GKcPZbMoGq7WPzKLfRBTU --- ...l-approvers-manager-rung-may-resolve-empty.md | 16 ++++++++++++++++ packages/lint/src/validate-approval-approvers.ts | 8 ++++++-- 2 files changed, 22 insertions(+), 2 deletions(-) create mode 100644 .changeset/approval-approvers-manager-rung-may-resolve-empty.md 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..3dad63bfb5 --- /dev/null +++ b/.changeset/approval-approvers-manager-rung-may-resolve-empty.md @@ -0,0 +1,16 @@ +--- +"@objectstack/lint": patch +--- + +`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.** Populate `sys_user.manager_id` by SCIM provisioning, a seed / bulk import or directory sync — explicitly **not** by editing the user in the Console, which cannot write it — or add a fallback approver that cannot resolve empty, such as `{ type: 'org_membership_level', value: 'owner' }`. That second escape works today regardless of whether the column ever gains a write surface. +- **When it stays quiet.** 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. + +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. + +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.ts b/packages/lint/src/validate-approval-approvers.ts index 504e748581..4402d20290 100644 --- a/packages/lint/src/validate-approval-approvers.ts +++ b/packages/lint/src/validate-approval-approvers.ts @@ -111,8 +111,12 @@ const GROUP_ROUTED_TYPES = new Set(['position', 'team', 'department']); * goes stale: it would then be wrong to tell an author their only route is * provisioning. Update it in the same change that opens the write surface. */ +// ⛔ 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 today (#16678) — the data API's managed-update ` + + `sys_user.manager_id has no product write surface today — 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 populated by SCIM provisioning, a seed / bulk import ` + `or directory sync, NOT by editing the user in the Console.`; @@ -475,7 +479,7 @@ export function validateApprovalApprovers(stack: AnyRec): ApprovalApproverFindin `${MANAGER_ONLY_REMEDY} Populate it for everyone who submits this request, or add a ` + `fallback approver that 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 (#3424).`, + `recoverable only by a platform/tenant admin override.`, }); } From 05e4ac1410d7ccecad8dabe83e19304f561cfcd4 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 03:28:25 +0000 Subject: [PATCH 3/4] fix(lint): grade the approvers widening at minor, not patch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A rule that begins covering a case it was silent on is a purely additive widening of a published package's public surface, and the standing maintainer ruling puts a floor of `minor` on that act regardless of the commit type. The clause-② declaration this PR carries is correct and stays; it was the level under it that was wrong. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012GKcPZbMoGq7WPzKLfRBTU --- .../approval-approvers-manager-rung-may-resolve-empty.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.changeset/approval-approvers-manager-rung-may-resolve-empty.md b/.changeset/approval-approvers-manager-rung-may-resolve-empty.md index 3dad63bfb5..15fb865693 100644 --- a/.changeset/approval-approvers-manager-rung-may-resolve-empty.md +++ b/.changeset/approval-approvers-manager-rung-may-resolve-empty.md @@ -1,5 +1,5 @@ --- -"@objectstack/lint": patch +"@objectstack/lint": minor --- `approval-approvers-may-resolve-empty` now covers the `manager` rung, not just the group-routed ones. @@ -13,4 +13,6 @@ The rule exists for the empty-slate dead-end (#3424): an approver slate that res 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. From 0c7c3d9b2e5bb4dc3ee191784c181b32e98b8471 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 04:18:03 +0000 Subject: [PATCH 4/4] fix(lint): grade the manager-rung remedy routes, and say where the seed suppressor applies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The hint listed SCIM, bulk import and directory sync as equally available routes for populating `sys_user.manager_id`. Measured against this tree, two of the three have no writer here: `admin-import-users.ts` matches `manager_id` 0 times against a control of `phone_number` 8 and its update set is {name, image, locale} + phone_number + role, and the SCIM Enterprise `manager` attribute is declared without any non-test file projecting it onto the column. A seed — or any other system-context write — does work, because both write guards gate on `isUserContextWrite` (`userId && !isSystem`). An exact diagnosis whose prescription cannot be carried out is worse than no prescription, so the routes are now graded rather than listed. SCIM and directory sync stay named: a deployment running a real one may populate the column through it, and the defect was presenting them as something this platform provides. Also records the surface asymmetry of the seed suppressor. The runtime publish gate's context carries objects, permissions, books, datasets and pages, and no `data`, so the suppression is CLI-side only and a Studio publish draws the advisory however the tenant's users are wired. Prose only — the arm, the tier and the position arm are untouched. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012GKcPZbMoGq7WPzKLfRBTU --- ...pprovers-manager-rung-may-resolve-empty.md | 4 +- .../src/validate-approval-approvers.test.ts | 25 +++++- .../lint/src/validate-approval-approvers.ts | 77 +++++++++++++++---- 3 files changed, 90 insertions(+), 16 deletions(-) diff --git a/.changeset/approval-approvers-manager-rung-may-resolve-empty.md b/.changeset/approval-approvers-manager-rung-may-resolve-empty.md index 15fb865693..e4f6b467f8 100644 --- a/.changeset/approval-approvers-manager-rung-may-resolve-empty.md +++ b/.changeset/approval-approvers-manager-rung-may-resolve-empty.md @@ -8,8 +8,8 @@ The rule exists for the empty-slate dead-end (#3424): an approver slate that res - **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.** Populate `sys_user.manager_id` by SCIM provisioning, a seed / bulk import or directory sync — explicitly **not** by editing the user in the Console, which cannot write it — or add a fallback approver that cannot resolve empty, such as `{ type: 'org_membership_level', value: 'owner' }`. That second escape works today regardless of whether the column ever gains a write surface. -- **When it stays quiet.** 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. +- **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. diff --git a/packages/lint/src/validate-approval-approvers.test.ts b/packages/lint/src/validate-approval-approvers.test.ts index aa185e135e..9fe0c774e4 100644 --- a/packages/lint/src/validate-approval-approvers.test.ts +++ b/packages/lint/src/validate-approval-approvers.test.ts @@ -269,12 +269,35 @@ describe('unset-manager dead-end (#16748)', () => { 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('NOT by editing the user in the Console'); + 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'); diff --git a/packages/lint/src/validate-approval-approvers.ts b/packages/lint/src/validate-approval-approvers.ts index 4402d20290..220014cf7a 100644 --- a/packages/lint/src/validate-approval-approvers.ts +++ b/packages/lint/src/validate-approval-approvers.ts @@ -103,23 +103,62 @@ const GROUP_ROUTED_TYPES = new Set(['position', 'team', 'department']); * ⚠️ 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. So - * the only honest prescription is the provisioning path. + * 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, this string is the line that - * goes stale: it would then be wrong to tell an author their only route is - * provisioning. Update it in the same change that opens the write surface. + * 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 today — 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 populated by SCIM provisioning, a seed / bulk import ` + - `or directory sync, NOT by editing the user in the Console.`; + `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'; @@ -181,6 +220,18 @@ const TYPE_FIX: Record = { * 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[]) : []; @@ -476,10 +527,10 @@ export function validateApprovalApprovers(stack: AnyRec): ApprovalApproverFindin `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} Populate it for everyone who submits this request, or add a ` + - `fallback approver that 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.`, + `${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.`, }); }