Skip to content

Commit d438b3a

Browse files
claude[bot]claude
andauthored
feat(plugin-auth): the bulk import admits manager_id, resolved in a second pass through the admin write surface's own refusals (#18046)
Fixes #18028 Clause-②: yes Ruling `5651634638` row 7, split out of #16678 by its Phase 3 landing report (`5653051072`): `manager_id` is admitted to the bulk-import tier, resolved in a **second pass** keyed on the importer's identity key, with every row-2 refusal applied per row and an unresolved key reported as a per-row error. Maintainer verbatim, from the ruling this implements: > 同意 经理 = 管理员在用户上显式设置的 manager_id;部门负责人 = 单元上的 manager_user_id,两者独立。 ## The card's three open questions, answered by measurement **1. Does the importer have a place for a second pass, or does it commit row-by-row?** It has one, and no restructure is owed. `runAdminImportUsers` calls the shared row engine ONCE over the whole batch (`runImport`, one call) and already carries a post-write section that walks `results` after it returns — the delivery pass (invitation email / SMS / temporary password) and the run-level audit row both live there, both gated on `!prepared.dryRun`. `createData` does commit per row, but nothing observes those commits until `runImport` has returned, so "every row in the batch exists" is a state the endpoint already reaches on its own. The manager pass is a third phase in that existing section. ⇒ **small addition, not a restructure.** **2. What does the per-row error channel look like?** Two channels, and the card named the right one. `rows[]` is `ImportRowResult` (`row`, `ok`, `action`, `id`, `code`, `error`) plus this endpoint's own extensions, of which `rows[].delivery` is the precedent: a small string union stamped by a post-write phase. The sibling post-write FAILURE (`INVITE_EMAIL_FAILED` / `INVITE_SMS_FAILED`) rides `code` + `error` on a row that stays `created`. This change follows both: `rows[].manager` carries the machine-readable outcome in `delivery`'s shape, `rows[].error` carries the sentence. It does **not** stamp a `rows[].code`, and that is a fence rather than an oversight — see *What is fenced out* below. **3. Are N row-wise `runSetUserManager` calls acceptable, or is a batch seam owed?** Row-wise is acceptable at this endpoint's cap; a batch seam is not owed. Per link the derivation issues 1 `sys_user` read for the user, 1 for the proposed manager, at most 2 `sys_member` reads, and 1 read per ancestor while walking the chain (1-2 in a flat org, `MAX_MANAGER_CHAIN_DEPTH` = 20 worst case), then 1 update. Against `IMPORT_USERS_MAX_ROWS` = 500 that is ~3k reads typically and ~12k worst case — on an endpoint whose module header already records ~100ms/row of scrypt hashing, i.e. ~50s of CPU the batch is already paying. The seam that IS owed is a different one, and it is small: `runSetUserManager(deps, actor, request)` takes an HTTP `Request`, so it could not be driven row-wise as it stood. Rather than fork the predicates, the refusal core is extracted: ``` export async function applyUserManagerLink( deps: SetUserManagerDeps, userId: string, managerId: string | null, ): Promise<SetUserManagerRefusal | null> ``` `runSetUserManager` is now exactly that function plus body parsing and an HTTP envelope; the importer is exactly that function called once per row. Zero behaviour change on the endpoint — its own 27-test suite is unchanged and green. ## The refusal-reuse fence, and how it is held ⛔ There is no second copy of the five predicates. The importer imports `applyUserManagerLink` and reports the `SetUserManagerRefusal` it gets back — status, code and the `reason` discriminator — without re-wording it. Four of the five are driven end-to-end through the importer in the new suite and asserted to surface with the endpoint's own reason: `self_assignment`, `cycle`, `idp_provisioned`, `cross_organization` (that last one proves the delegate's `sys_member` screen really runs, since nothing in the importer reads that table). The fence is also pinned as an ABSENCE, with a positive control so the absence is a reading rather than a vacuous pass after a rename: the importer's source is asserted to contain `applyUserManagerLink` and the delegate's module specifier, and to contain zero occurrences of each of the five reason literals, of `MAX_MANAGER_CHAIN_DEPTH`, and of `sys_member`. ## Proving the pass is really SECOND A pin that only checks "a manager got resolved" passes against a single-pass resolve that happens to work because the fixture was written in dependency order. The discriminating fixture is deliberately out of order — row 1 names a manager that row 2 creates — and three assertions stand on it: | assertion | what it catches | |---|---| | row 1 links to the id row 2 minted | the feature | | CONTROL: the harness records `sys_user` at the instant each row was written, and the manager is NOT in the snapshot taken for row 1 | proves the forward reference is real, so the pass above is not explained by ordered input | | CONTROL: the link's `update` has a later `invocationCallOrder` than the LAST `createUser` | a single-pass resolve writes row 1's link before row 2 is created, so this is false for it | Both directions of the per-row error are pinned too, because a test asserting only the first passes against an implementation that aborts everything: the offending row reports (`manager: 'unresolved'`, an `error` naming the column), AND `summary.created` is 2, `summary.errors` is 0, the other row carries no code, and the unlinked user exists with `manager_id` null — ⛔ not a silent skip. ## Reverse verification — three ablations, each with an on-disk proof and a restore proof Every leg: mutate by exact string replacement, prove the mutation reached disk by occurrence count (never an editor's exit code), run, restore with `git checkout HEAD -- path`, and prove the restore by blob hash against `HEAD` plus a clean `git diff HEAD`. All three legs restored cleanly; the tree ended at `git diff HEAD` clean. | ablation | expected | MEASURED | |---|---|---| | **C** — withhold the second pass itself (its guard to `false`) | red | **14 of 20 red.** The 6 that stay green are the ones that should: the empty-cell case, the Tier-1 pin, the source-fence pin, the upsert-patch pin, the dryRun pin, and the fixture-premise control | | **B** — neutralize the DELEGATE's `self_assignment` predicate | red in BOTH suites | **2 red, one in each suite** — `admin-import-users-manager-pass.test.ts` and `admin-set-user-manager.test.ts`. One mutation, two callers: that is the single derivation, demonstrated rather than asserted | | **A** — withhold the batch-wide in-batch index (`idByKey`) | red | **GREEN, 20 passed — reported as measured, not as expected.** Correct and informative: a created row is in `sys_user` by the time the pass runs, so the directory lookup resolves it anyway. The index is an OPTIMIZATION (it saves a read per row); the MECHANISM is the pass's position, which is what C and the two ordering controls measure | Control run on the unmutated tree: both suites, 47 passed. ## What is fenced out, and routed rather than ridden in ⛔ `packages/spec` is untouched, and one thing wanted to reach into it. The obvious symmetry with `INVITE_EMAIL_FAILED` would be `rows[].code = 'MANAGER_UNRESOLVED' | 'MANAGER_REFUSED'`. It was written that way first, and `pnpm check:dispatcher-error-vocabulary` refused it: ``` [unclassified-site] packages/plugins/plugin-auth/src/admin-import-users.ts stamps unregistered code 'MANAGER_REFUSED' (codehelper) and packages/runtime/src/dispatcher-error-vocabulary.ts does not classify it. That file is inside the published face, so the way out is the LEDGER: register 'MANAGER_REFUSED' in packages/spec/src/api/error-code-ledger.zod.ts under its stamping package ``` That registration is a `packages/spec` edit this lane is fenced out of, and #17995 already carries the closed-vocabulary question for this endpoint's refusals. So the code half was **removed, not registered and not faked**: the machine-readable half lives on `rows[].manager` (which discriminates `unresolved` from each refusal `reason` — finer than two codes would have been) and the sentence on `rows[].error`. ⛔ Reaching for an already-registered code whose meaning is something else would be the lenient alias Prime Directive #12 refuses. The absence is pinned by a test, so the symmetry cannot be restored without the registration that makes it legal. The gate is green. Also untouched, deliberately: `SYS_USER_PROFILE_EDIT_FIELDS` and `SYS_USER_IMPORT_UPDATE_FIELDS` (asserted unchanged by a test, exactly as #16678's delivery pinned them — the import reaches the column by system context, the way it already reaches `phone_number` and `role`); `content/docs/releases/**`; and §4 org-unit derivation, which the ruling CUT on axis ①. ## Clause-② re-derived from the BUILT entry, with both controls `packages/plugins/plugin-auth/src/index.ts:26` carries `export * from './admin-import-users.js'`, so the importer's exports reach the package entry. Measured by occurrence count in `dist/index.d.ts` after a real build, before and after this change: | symbol | before | after | reading | |---|---|---|---| | `IMPORT_USERS_MAX_ROWS` | 2 | 3 | POSITIVE control — a symbol known to be published | | `runAdminImportUsers` | 2 | 2 | POSITIVE control | | `IdentityImportRowResult` | 0 | 2 | NEW, and in the entry's `export { … }` clause | | `ImportManagerOutcome` | 0 | 3 | NEW, and in the entry's `export { … }` clause | | `SetUserManagerRefusalReason` | 0 | 3 | newly reachable STRUCTURALLY (inlined as `ImportManagerOutcome`'s union), not by name | | `applyUserManagerLink` | 0 | 1 | a docblock mention only — ⛔ not an export; `admin-set-user-manager.js` is NOT in `index.ts` | | `SetUserManagerRefusal`, `runSetUserManager`, `MAX_MANAGER_CHAIN_DEPTH` | 0 | 0 | still unreachable from the entry | | `resolveManagerKey` / `noteManagerFailure` / `identityKey` / `ManagerKeyRef` | 0 | 0 | NEGATIVE controls — present in the source at 2 / 5 / 9 / 3 occurrences, absent from the built entry | ⇒ the card's `Clause-②: yes` is CONFIRMED, not merely inherited: two named type exports land on the published entry and a third union arrives structurally. The changeset is graded `minor` accordingly (AGENTS.md: a PR declaring `Clause-②: yes` takes at least `minor`), which `check-changeset-no-major --base origin/main` reads as discharged. ## Gates Head SHA: `2be67d2d4a7bdfc43fe674ab343ea462ba0ac885`. - `pnpm --filter @objectstack/plugin-auth test` — **111 files, 2360 tests, all passed.** - `pnpm --filter @objectstack/plugin-auth typecheck` — **exit 0** (includes `check:test-typecheck` against `tsconfig.test.json`). - `pnpm lint` (the whole repo, `eslint . --no-inline-config`) — **exit 0**, 2m59s. Not a narrowed run: the full scan. - The derived gate families for this diff, via `node scripts/pm/dispatch-gates.mjs --commands` and reconciled with `--ran`: **69 derived, 68 run green, 1 NOT MEASURED, 0 unrun**, plus 2 run beyond the union. The one NOT MEASURED family, declared rather than rounded to green: > `pnpm check:type-check-debt` — exit 3, PREREQUISITE NOT MET. Its first exit 3 named four unbuilt workspace dependencies; the closure was then built (`turbo run build --filter='./packages/*' --filter='./packages/*/*'`, exit 0) and the prerequisite it named is satisfied. It now dies differently: its pinned `tsc --max-old-space-size=6144` re-measure is killed by the V8 heap limit on this shared container (`FATAL ERROR: Ineffective mark-compacts near heap limit`). ⛔ Not a pass and not a finding — nothing was measured. Its sibling `pnpm check:type-check-coverage` (the non-re-measure half) is **exit 0**. ⚠️ One result in this run was a false red and is recorded so it is not re-diagnosed: `pnpm --filter @objectstack/plugin-auth typecheck` reported 25 errors in `admin-import-users.ts` while the workspace closure build was mid-flight rewriting `dist/`. Re-run against the finished closure: exit 0. The full `Lint & Repo Gates` step sequence is CI's — it is ~200 sequential steps and the derived-family set above is what this seat can measure locally without running the whole farm twice. ## Acceptance notes Noted, not filed — neither is a reproducible defect, a contract violation, or a metadata-authoring trap, so neither meets the filing bar: - **A row-level `error.code` for the manager outcome is owed to the ledger, not to this lane.** `MANAGER_UNRESOLVED` / `MANAGER_REFUSED` would need registering in `packages/spec/src/api/error-code-ledger.zod.ts` under `@objectstack/plugin-auth` (where the endpoint's sibling row codes `INVITE_EMAIL_FAILED`, `NO_IDENTITY`, `CREATE_FAILED` already sit). Handover: #17995 item 2, which is already queued to decide dedicated codes for these same refusals in the same file — one visit to one package. - **`.changeset/16678-admin-set-user-manager.md`** (the sibling from the parent card, still unreleased at HEAD) grades `@objectstack/plugin-auth: patch` while #16678 declared `Clause-②: yes`. Consequence is already neutralized: this PR's `minor` on the same package governs the next release of the fixed group. Handover: the release seat reading grades at version time. ⛔ Draft, not flipped, no auto-merge armed — landing is the PM seat's. --- _Generated by [Claude Code](https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj)_ --- _Generated by [Claude Code](https://claude.ai/code)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent b3d6918 commit d438b3a

5 files changed

Lines changed: 994 additions & 22 deletions

File tree

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
---
2+
'@objectstack/plugin-auth': minor
3+
---
4+
5+
The bulk identity import admits `manager_id`, resolved in a second pass keyed on the importer's identity key
6+
7+
`POST /api/v1/auth/admin/import-users` now reads a `manager_id` column. Until
8+
this change it matched `manager_id` **0** times — against a positive control of
9+
`email` at 73 — so a CSV naming everyone's manager built the org chart for
10+
nobody, silently: the column was dropped on create (the identity write path
11+
composes its own better-auth body) and filtered out on upsert (it is not in
12+
`SYS_USER_IMPORT_UPDATE_FIELDS`). With
13+
`POST /api/v1/auth/admin/set-user-manager` shipped, the import surface was the
14+
one remaining route that could populate the column at scale and did not.
15+
16+
**What the cell holds is an identity key, not a user id.** A CSV author has the
17+
manager's email or phone number, never their `usr_…` id, so the cell is read
18+
with the same key the importer already keys rows by. One spelling, `manager_id`
19+
— the phone column's three historical aliases are debt this key does not
20+
inherit.
21+
22+
**The pass is SECOND, and that is load-bearing.** A manager named in row 40 may
23+
be created by row 90, so the links are applied after the row engine has
24+
returned and every row in the batch exists. A resolve inside the per-row write
25+
would refuse exactly that input and would appear to work only on a file whose
26+
rows happened to arrive in dependency order. A manager who is *not* in the file
27+
is resolved against the directory instead, so an org chart can be grown one
28+
batch at a time.
29+
30+
**Every refusal is the write surface's, applied per row.** The importer calls
31+
`applyUserManagerLink` — the same derivation `POST /admin/set-user-manager`
32+
runs — so self-assignment, a link that closes a cycle, a chain past the depth
33+
cap, a manager provably outside every organization the user belongs to, and any
34+
identity whose `sys_user.source` is `idp_provisioned` are refused on import
35+
exactly as they are on the endpoint, with the endpoint's own `reason`
36+
discriminator carried through. There is no second copy of those predicates.
37+
38+
**A manager problem never costs the row its identity.** The user is created
39+
either way; the failure is reported on that row — `rows[].manager` carries the
40+
machine-readable outcome in the shape `rows[].delivery` already uses
41+
(`unresolved`, or the refusal's own `reason`), and `rows[].error` carries the
42+
sentence. It is ⛔ not a whole-import failure and ⛔ not a silent skip, and an
43+
engine fault while linking is reported the same way rather than turning a 200
44+
that created N users into a 500 that reports none of them. No `rows[].code` is
45+
stamped for a manager outcome: a row-level code would have to be registered in
46+
the `packages/spec` error-code ledger, which this change is fenced out of, so
47+
the machine-readable half lives on `rows[].manager` instead of on a code the
48+
vocabulary does not carry.
49+
50+
**New on the response.** `data.summary.manager` is
51+
`{ linked, unresolved, refused }`, beside `data.summary.delivery`, and the
52+
run-level `sys_audit_log` row records the same split. Row objects are typed as
53+
the newly exported `IdentityImportRowResult`, whose `manager` member is an
54+
`ImportManagerOutcome`.
55+
56+
**Unchanged, deliberately.** `SYS_USER_PROFILE_EDIT_FIELDS` and
57+
`SYS_USER_IMPORT_UPDATE_FIELDS` are untouched — the import reaches the column
58+
by system context, the same way it already reaches `phone_number` and `role`,
59+
and the same way the admin endpoint does. `manager_id` keeps `readonly: true`
60+
on the column. Nothing derives a manager from org-unit membership. A dry run
61+
does not run the pass at all and reports zeroes rather than half-answering
62+
about links it could not evaluate.

0 commit comments

Comments
 (0)