Skip to content

Commit 9f15939

Browse files
fix(lint): the approval-approvers-may-resolve-empty remedy names the admin bulk import's manager_id column (#22703)
Fixes #22683 Clause-②: no ## What was wrong On a node whose approvers are all `{ type: 'manager' }`, `os lint` reports `approval-approvers-may-resolve-empty` (info). Its hint ended the SCIM clause with "and its admin bulk import does not write it either". That has been false since the import admitted a `manager_id` column (PR #18046), so the remedy sent an author with a whole file of reports away from a supported route. The guide (`automation/approvals.mdx`, corrected by PR #22681) already names that column and says `os lint` "carries the same remedy text", so the lint string was one clause behind it. ## What changed (3 files, +69 / -15) - `packages/lint/src/validate-approval-approvers.ts` - `MANAGER_ONLY_ROUTES`: the false clause is gone. The bulk import (`POST /api/v1/auth/admin/import-users`) is named as a route that works, right after the endpoint and its refusals and ahead of the clause that hands SCIM and directory sync to the deployment. It reads a `manager_id` column holding the manager's email (or phone where phone sign-in is enabled), links each row once every row in the file exists, through the same refusals, and reports a link it cannot make on that row's `manager` result without failing the user. The `idp_provisioned` clause keeps its wording and now says the import's column is refused too. - `MANAGER_ONLY_REMEDY`: names Set Manager (Setup → Users, row menu or record header) beside the endpoint it posts to. The card's done-when says "beside Set Manager and the endpoint"; the hint did not name Set Manager at all before, so this is the one addition beyond dropping the clause. The word "Console" is still used only inside the existing denial. - The file's own docblock graded the import "NOT here ... ruled but a separate change"; that grade is replaced with the measured one, so no comment in the file contradicts the string. - Unchanged: what the rule fires on, its level (`info`), `stackWiresManagerChain` as the one silencer, the fallback clause, the closing sentences. - `packages/lint/src/validate-approval-approvers.test.ts`: the pin on "admin bulk import does not write it either" is replaced. The 'GRADES the routes' test now asserts the import path, `reads a manager_id column`, `once every row in the file exists`, the per-row report, that the old clause is gone, and that the import is named before the clause that hands the remaining routes to the deployment. The 'names the REAL remedy' test asserts Set Manager. - `.changeset/22683-approver-remedy-bulk-import.md`: `@objectstack/lint` patch, `Clause-②: no`. ## Premise check (measured on origin/main cb3bb93 before writing a sentence) The import does what the new sentence says, read in `packages/plugins/plugin-auth/src/admin-import-users.ts`: `MANAGER_COLUMN = 'manager_id'`; `resolveManagerKey` reads the cell as an email, or a phone number only when the phone plugin is wired; the cell is deleted from the row before the identity write; the second pass runs after `runImport` returns, resolves the key against this batch's rows and then against `sys_user`, and calls `applyUserManagerLink` (the endpoint's derivation); a failure goes to `noteManagerFailure` on the row, never a whole-import failure; `dryRun` skips the pass. Behaviour: `pnpm --filter @objectstack/plugin-auth exec vitest run --maxWorkers=2 src/admin-import-users-manager-pass.test.ts` after building `@objectstack/plugin-auth^...`: `Test Files 1 passed (1)`, `Tests 20 passed (20)`, including a link to a manager created by a later row and its control where the manager does not exist yet. ## Grep for the family (the card's done-when), on this branch at head d63e3f6 ``` $ git grep -n "bulk import does not write" -- packages/lint/src content/docs skills (no output; exit 1) $ git grep -n "bulk import does not write" cb3bb93 -- packages/lint/src content/docs skills # positive control, the base commit cb3bb93:packages/lint/src/validate-approval-approvers.test.ts:313: expect(finding.hint).toContain('admin bulk import does not write it either'); cb3bb93:packages/lint/src/validate-approval-approvers.ts:199: `admin bulk import does not write it either — and where an identity carries ` + ``` A wider pass over `packages`, `content`, `skills` (CHANGELOGs excluded) for "import does not write" and "does not write it either" has one hit: a comment in the test file that quotes the retired clause. It also finds no other claim that the import or anything else fails to write `manager_id`; the only near-misses are two past-tense history notes ("had no product write surface") in `auth-route-ledger.ts` and `admin-set-user-manager.test.ts`, true of the column before the endpoint landed. With this card the enumeration covers `packages/lint/src`, `content/docs` and `skills/`. ## Verification - Pin test, before the change: `Tests 47 passed (47)`; after: `Tests 47 passed (47)` (`pnpm --filter @objectstack/lint exec vitest run --maxWorkers=2 src/validate-approval-approvers.test.ts`). - Reverse verification, run from the committed fix: the source file restored to its parent version (working tree only), the same test: `Tests 2 failed | 45 passed (47)`, `expected 'sys_user.manager_id is not a profile …' to contain 'Set Manager'` and `... to contain '/api/v1/auth/admin/import-users'`. Direction: red, as expected. Restored with `git checkout HEAD -- path`; the file's blob hash read `143561c356c5871ccbc2c42d082c1b854797171d` before and after, and `git diff HEAD` was empty. - Whole package: `pnpm --filter @objectstack/lint exec vitest run --maxWorkers=2`: `Test Files 135 passed (135)`, `Tests 6231 passed (6231)` (at b9731fc; the later commit changes one regex in the test file, re-run on its own: 47 passed). - `pnpm --filter @objectstack/lint run typecheck` (tsc --noEmit plus `check:test-typecheck`): OK. `pnpm --filter @objectstack/lint run build`: success, `check-dts-emitted: 6/6`. In the built `dist/index.js` and `dist/index.cjs`: new clause 1, old clause 0, control string 1. `validateApprovalApprovers` called from the built `dist/index.cjs` on a manager-only flow returns one `info` finding whose hint carries the new text. - Gates: `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` derived 62 commands from this diff at head d63e3f6 (identical to the dispatch-time list); all 62 run on that head, every one exit 0, reconciled with `--ran`: `62 derived, 62 run, 0 NOT-MEASURED, 0 UNRUN`. Two ratchet gates (`check:type-check-debt`, `check:query-options-erasure`) hit this box's own 240 s runner cap under load 17 on the first pass and passed on re-run (`re-measured in 522.7s, 26 raw tsc error(s) total, none above its recorded number`; `ratchet holds`). - Narrowed `eslint --no-inline-config --format json` on the two changed files: 2 files, 0 errors, 0 warnings. The repo-wide `pnpm lint` is CI's. Invariance: `eslint.config.mjs` never enables type-aware linting (no `parserOptions.project`, line 328), so this diff cannot move the verdict of a file it did not touch. - Control-character scan over the three files: none. ## Acceptance notes - Not done: AGENTS.md section 10's "pull main and run the full suite once". `origin/main` moved to d8830c2 during the run; its only difference from the base on `packages/lint` and `.changeset` is an unrelated changeset, so this branch was not merged forward. The merge queue validates the merged generation. - Not run locally, CI's: the workspace type-check lanes, the sharded `Test Core`, `Build Core`, dogfood and temporal jobs, repo-wide `pnpm lint`. - The `idp_provisioned` clause now says the import's column is refused too. That is the import's `applyUserManagerLink` delegation (covered by `admin-import-users-manager-pass.test.ts`, "directory-owned identity"), not a new claim about the endpoint. - No `rule-explanations.ts`, `content/docs` or `skills/` edits. --- _Generated by [Claude Code](https://claude.ai/code/session_01S3aAf11JjbW1mSGL1EhfFj)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 2fbe1fd commit 9f15939

3 files changed

Lines changed: 69 additions & 15 deletions

File tree

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
---
2+
'@objectstack/lint': patch
3+
---
4+
5+
`os lint`'s `approval-approvers-may-resolve-empty` hint no longer tells an author that the admin bulk import does not write `sys_user.manager_id`; it names the import's `manager_id` column as a working route, beside Set Manager and the admin endpoint
6+
7+
Clause-②: no
8+
9+
On a node whose approvers are all `{ type: 'manager' }`, the hint listed the routes that populate `sys_user.manager_id` and said of one of them: "its admin bulk import does not write it either". That has been false since the import admitted the column. `POST /api/v1/auth/admin/import-users` reads a `manager_id` column whose cell holds the manager's email address, or phone number where phone sign-in is enabled. The manager can be someone in the same file or an existing user. The import links each row once every row in the file exists, through the same derivation as `POST /api/v1/auth/admin/set-user-manager`, and reports each outcome on the row's `manager` result. A link it cannot make never fails the user it imported. The old clause sent an author with a whole file of reports away from that route.
10+
11+
The hint now names the bulk import's `manager_id` column among the routes that work on this platform, ahead of the clause that hands SCIM and directory sync to the deployment, and names Set Manager (Setup → Users) beside the endpoint it posts to. Nothing else about the rule moves: it fires on the same shape, at `info`, and a seeded manager chain remains the one silencer.

‎packages/lint/src/validate-approval-approvers.test.ts‎

Lines changed: 29 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,10 @@ describe('unset-manager dead-end (#16748)', () => {
267267
expect(finding.hint).toContain('/api/v1/auth/admin/set-user-manager');
268268
expect(finding.hint).toContain('{ userId, managerId }');
269269
expect(finding.hint).toContain('managerId set to null');
270+
// The in-product door onto the same endpoint (the `set_user_manager` action
271+
// on `sys_user`, label "Set Manager") — an admin reading this in a terminal
272+
// is looking for the button, not the HTTP call.
273+
expect(finding.hint).toContain('Set Manager');
270274
expect(finding.hint).toContain('SCIM');
271275
expect(finding.hint).toContain('import');
272276
expect(finding.hint).toContain('directory sync');
@@ -306,19 +310,40 @@ describe('unset-manager dead-end (#16748)', () => {
306310
expect(finding.hint).toContain('written by a seed, or by any other system-context write');
307311
expect(finding.hint).toContain('bypasses the managed-update whitelist');
308312

309-
// ⛔ The two that are NOT this repo's to offer must be marked as the
313+
// The bulk route also WORKS HERE, so it is graded beside the endpoint and
314+
// ⛔ never among the routes the deployment must supply: the admin import
315+
// admits a `manager_id` column whose cell is the manager's identity key and
316+
// links it in a second pass, once every row of the file exists, through the
317+
// endpoint's own derivation. The clause this replaces told the author the
318+
// import does not write the column — away from the one supported way to
319+
// link a whole file. It must be gone from the string, not merely
320+
// contradicted further down it.
321+
expect(finding.hint).toContain('/api/v1/auth/admin/import-users');
322+
expect(finding.hint).toContain('reads a manager_id column');
323+
expect(finding.hint).toContain('once every row in the file exists');
324+
expect(finding.hint).toContain("reported on that row's manager result");
325+
expect(finding.hint).not.toMatch(/does not write it/);
326+
// Placement is the grade: ahead of the clause that hands the remaining
327+
// routes to the deployment.
328+
const importAt = finding.hint.indexOf('/api/v1/auth/admin/import-users');
329+
const deploymentAt = finding.hint.indexOf('a provisioning path your own deployment supplies');
330+
expect(importAt).toBeGreaterThan(-1);
331+
expect(deploymentAt).toBeGreaterThan(importAt);
332+
333+
// ⛔ The one that is NOT this repo's to offer must be marked as the
310334
// deployment's own, and the hint must say WHY rather than merely hedging.
311335
expect(finding.hint).toContain('a provisioning path your own deployment supplies');
312336
expect(finding.hint).toContain("declares the SCIM 'manager' attribute without projecting it");
313-
expect(finding.hint).toContain('admin bulk import does not write it either');
314337

315338
// ⛔ And they must not be deleted: a deployment running a real directory
316339
// sync may well populate the column, and the defect was presenting all
317340
// three as equally available, never naming them at all. They now also take
318-
// PRECEDENCE — the endpoint refuses an `idp_provisioned` identity — so the
319-
// hint must not read as "use the endpoint instead of your directory".
341+
// PRECEDENCE — the endpoint refuses an `idp_provisioned` identity, and so
342+
// does the import's `manager_id` column, which runs the same derivation —
343+
// so the hint must not read as "use the endpoint instead of your directory".
320344
expect(finding.hint).toContain('SCIM provisioning and directory sync can populate it');
321345
expect(finding.hint).toContain("source 'idp_provisioned' the admin operation refuses");
346+
expect(finding.hint).toContain("the import's manager_id column included");
322347
});
323348

324349
it('does not claim a runtime fact it did not read', () => {

‎packages/lint/src/validate-approval-approvers.ts‎

Lines changed: 29 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -136,11 +136,23 @@ const GROUP_ROUTED_TYPES = new Set(['position', 'team', 'department']);
136136
* `packages/plugins` or `packages/runtime` projects it onto the column:
137137
* measured 0, against a control (`SysScimGroup`) the same scan does find,
138138
* so the scan discriminates.
139-
* - Admin bulk import — NOT here, re-measured and unchanged.
140-
* `admin-import-users.ts` matches `manager_id` 0 times against a control of
141-
* `phone_number` 8, and `SYS_USER_IMPORT_UPDATE_FIELDS` is
142-
* `{name, image, locale}` plus `phone_number` and `role`. Admitting the
143-
* column to the import tier is ruled but is a separate change.
139+
* - Admin bulk import — AVAILABLE HERE, and the grade this block used to
140+
* give it ("NOT here") went stale when the import tier admitted the column.
141+
* `POST /api/v1/auth/admin/import-users` (`admin-import-users.ts`) reads a
142+
* `manager_id` column (`MANAGER_COLUMN`) whose cell holds the MANAGER'S
143+
* identity key — an email, or a phone number where the phone plugin is
144+
* wired — strips it from the row before the identity write, and links it in
145+
* a SECOND pass once every row of the batch exists, through
146+
* `applyUserManagerLink`, the derivation `set-user-manager` itself runs.
147+
* So it carries the endpoint's refusals (self-assignment, cycle, depth cap,
148+
* cross-organization, directory-owned identity); a link it cannot make is
149+
* reported on that row's `manager` result and never fails the user it
150+
* imported. `SYS_USER_IMPORT_UPDATE_FIELDS` is unchanged — the column rides
151+
* the second pass, ⛔ not the upsert patch. Measured by
152+
* `admin-import-users-manager-pass.test.ts`: a link to a manager created by
153+
* a LATER row lands, with a control where the manager does not exist yet.
154+
* It is the route to name for a whole file; a remedy that said otherwise
155+
* sent the author away from a supported path.
144156
*
145157
* ⇒ SCIM and directory sync stay NAMED, because a deployment running a real
146158
* one may well populate the column through it — but named as something the
@@ -182,7 +194,8 @@ const MANAGER_ONLY_REMEDY =
182194
`sys_user.manager_id is not a profile column — the data API's managed-update whitelist is ` +
183195
`{name, image, locale} and the column is readonly on the user form, so it is never populated by ` +
184196
`editing the user in the Console. It has a dedicated admin operation instead: a platform admin ` +
185-
`POSTs { userId, managerId } to /api/v1/auth/admin/set-user-manager, with managerId set to null ` +
197+
`uses Set Manager (Setup → Users, from the user's row menu or record header) or POSTs ` +
198+
`{ userId, managerId } to /api/v1/auth/admin/set-user-manager, with managerId set to null ` +
186199
`to clear the link.`;
187200

188201
/**
@@ -192,13 +205,18 @@ const MANAGER_ONLY_REMEDY =
192205
const MANAGER_ONLY_ROUTES =
193206
`That endpoint is the route this platform gives you, and it refuses a link that would make a ` +
194207
`user their own manager, close a cycle, run past the chain depth cap, or point across an ` +
195-
`organization boundary. The column is also written by a seed, or by any other system-context ` +
208+
`organization boundary. For many users at once, the admin bulk import ` +
209+
`(POST /api/v1/auth/admin/import-users) reads a manager_id column holding the manager's ` +
210+
`email address, or phone number where phone sign-in is enabled — someone in the same file or ` +
211+
`an existing user — and links each row once every row in the file exists, through those same ` +
212+
`refusals; a link it cannot make is reported on that row's manager result and never fails the ` +
213+
`user it imported. The column is also written by a seed, or by any other system-context ` +
196214
`write, which bypasses the managed-update whitelist. SCIM provisioning and directory sync can ` +
197215
`populate it too, but only through a provisioning path your own deployment supplies: this ` +
198-
`platform declares the SCIM 'manager' attribute without projecting it onto the column, and its ` +
199-
`admin bulk import does not write it either — and where an identity carries ` +
200-
`source 'idp_provisioned' the admin operation refuses, leaving that directory the one surface ` +
201-
`that authors its manager. Or declare onEmptyApprovers: 'fallback' on the node, with a ` +
216+
`platform declares the SCIM 'manager' attribute without projecting it onto the column — and ` +
217+
`where an identity carries source 'idp_provisioned' the admin operation refuses, the import's ` +
218+
`manager_id column included, leaving that directory the one surface that authors its ` +
219+
`manager. Or declare onEmptyApprovers: 'fallback' on the node, with a ` +
202220
`fallbackApprovers list: an empty manager rung then opens the request on those people instead ` +
203221
`of on a slot nobody can act on, which needs no write to the column at all.`;
204222

0 commit comments

Comments
 (0)