fix(plugin-auth,lint): sys_user.manager_id gains an admin write surface, and the manager rung stops resolving to nobody - #17993
Conversation
WIP: endpoint, mount, ledger row and tests. Verification pending. Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj Co-authored-by: Claude <noreply@anthropic.com>
… remedy names the route Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj Co-authored-by: Claude <noreply@anthropic.com>
…l, tenant census - auth-route-ledger note carried a tracker id (check:doc-authoring Rule 3) - the test engine double read a WHERE combinator as a field name - the write-call-site census moved 222 -> 223 with the new engine write Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj Co-authored-by: Claude <noreply@anthropic.com>
…nager-admin-write-surface
📓 Docs Drift CheckThis PR changes 2 package(s): 51 hand-written doc(s) name something this change touched — list omitted above 15 rows. Re-derive on the tree named below: ⛔ 9 release-owned page(s) also affected — read-only, see AGENTS.md Documentation Guardrails. What this run could not see
Coarse fallback — 18 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 1890772821f44c3fd0fbff224c635c7562b2ce03 && git checkout 1890772821f44c3fd0fbff224c635c7562b2ce03
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 65767d2340dabce16953908699082dd97032da3b 02a036ad344290dad8565776ec5016637e722635 && git checkout -B drift-repro 65767d2340dabce16953908699082dd97032da3b && git merge --no-ff 02a036ad344290dad8565776ec5016637e722635
node scripts/docs-audit/affected-docs.mjs --json 65767d2340dabce16953908699082dd97032da3b
|
Contract reviewHead reviewed: Implemented-by: ① The declared conflict with the ruling — adjudicated, ⛔ not papered overThe delivery declined to resolve this silently and named it instead. Good. Ruling Adjudication: the measurement stands —
Limb (b): the endpoint's response is a new payload, ⛔ not a new key on an already-published one. A new route is wire, and wire compatibility is ⛔ not the floor. ⇒ reporting ✅ ②
|
🔴
|
…sweeps
Both /admin/ sweeps derive their population from the running stack, so the new
mount is in scope the moment it exists and must be classified with the answer a
non-admin actually gets. Measured against the live stack rather than assumed:
this mount runs gateAdmin() as its first statement, ahead of every body read, so
anon gets 401 UNAUTHENTICATED and a member 403 PERMISSION_DENIED on ANY body --
including an empty one, which is the opposite of unlock-user's ordering. The
platform admin gets 200 from the idempotent clear.
- admin-route-nonadmin-refusal: objectstack-gate, body { userId, managerId: null }
- admin-platform-admin-standing: ADMITTED + the same sweep payload
Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj
Co-authored-by: Claude <noreply@anthropic.com>
Contract review — RE-ISSUED for the post-rework headHead reviewed: Implemented-by:
① What the rework changed — and what it did not
⇒ Clause-② is unchanged: ② The rework answered the question it was sent to answer — by MEASURING, and the measurement cut against the file's own edgeThe dispatch warned that ⭐ Measured, the edge cuts the other way for this endpoint, and the entry records why: this mount runs
⇒ the answer is a ⭐ And it caught a vacuity trap the dispatch did not name. The payload entry is not optional even though the gate runs first: with no entry the handler answers ⭐ The allowed-side contrast was added too ( ③ The gate was satisfied, ⛔ not weakened⛔ No baseline, ⛔ no skip, ⛔ no entry with a guessed answer, ⛔ no re-run spent — a re-run cannot classify a route. The suite's bidirectionality (unclassified fails and stale fails) is intact, and this PR's route is now swept from here on. Verdict: PASS at
|
Fixes #16678
Clause-②: no
Phase 3 of the manager relationship, executed against the director-seat ruling (comment
5651634638, decision batch #127 item 1, option B) on the Phase 2 design (comment5648180780). Maintainer, verbatim — kept in the original, because rewriting the quotation rewrites the ruling:The hole, reproduced before it was closed
sys_user.manager_iddrives the approvals{ type: 'manager' }rung and theown_and_reportsread scope, and no product surface could write it. Measured on the branch point:admin-import-users.tsmatchesmanager_idphone_number8, same file, same scanplugin-auth/srcwritingmanager_idmanaged-extension-fields.tslisting it as not editablesys_user{name, image, locale}So on any install without a directory sync the rung expanded to nobody.
What this delivers
1. The endpoint.
POST /api/v1/auth/admin/set-user-manager, body{ userId, managerId },managerId: nullclears. An ObjectStack mount on the raw app ahead of the better-auth catch-all — theunlock-user/import-usersfamily — platform-admin gated (ADR-0068), ledgered inauth-route-ledger.tsand added to the conformance pin that asserts thesource: 'objectstack'set exactly.2. It reaches the column by CONTEXT, never by whitelist. The handler runs under a system context, exactly as
admin-import-usersalready reachesphone_numberandrole; both write guards gate onisUserContextWrite, spelledBoolean(userId) && isSystem !== true. SoSYS_USER_PROFILE_EDIT_FIELDS,MANAGED_EXTENSION_EDITABLE_FIELDS.sys_userandmanager_id'sreadonly: trueare all unmoved, and ADR-0092 D4 holds by construction. A test asserts the whitelist is still exactly{name, image, locale}, so a future change that "fixes" this by widening Tier 1 goes red here rather than quietly.3. Five refusals, every one enforced at the write, each with its reason in the code:
error.codeerror.details.reasonINVALID_REQUESTinvalid_bodyRESOURCE_NOT_FOUNDuser_not_foundINVALID_REFERENCEmanager_not_foundINVALID_FIELDself_assignmentRESOURCE_CONFLICTcycleVALUE_OUT_OF_RANGEmax_depth_exceededINVALID_REFERENCEcross_organizationPERMISSION_DENIEDidp_provisionedThe cycle check has to be here: the only manager-chain walkers in the open tree are single-hop (
ApprovalService.lookupManager,TeamGraphService.managerOf, one row each) and the multi-hop resolver ships outside this repo, so nothing downstream catches a loop this endpoint lets in. The walk carries its ownseenset, so a loop that already exists upstream is reported rather than hung on.The cross-organization refusal is the write-time half of
managerIsProvablyOutsideOrg, and deliberately shares its "provably" shape:sys_usercarries noorganization_id, sosys_memberrows are the only tenancy fact, and an absent fact is not a negative one. Both halves are wanted — data drifts after a write.The
idp_provisionedrefusal is the ruling's row 5 (P1, directory wins per identity), keyed on thesys_user.sourcecolumn the platform already stamps. It applies to the clear as well as the set, because both are writes the next sync would revert — the shape ADR-0049 exists to refuse.Why these codes and not dedicated ones:
error.codeis a closed vocabulary (StandardErrorCodeunionERROR_CODE_LEDGER) and both live inpackages/spec, which this lane is fenced out of. So every code is one this package may already emit, anderror.details.reasoncarries the machine-readable discrimination, pinned per refusal by the tests. A dedicated code per refusal is the better shape and is reported as a follow-up rather than reached over the fence for.4. The lint rule keeps its finding; only its remedy changed.
approval-approvers-may-resolve-emptyandstackWiresManagerChainare untouched as a rule — the dead end it reports survives the write surface, because a static check still cannot read the column; only its cause became recoverable.MANAGER_ONLY_REMEDY/MANAGER_ONLY_ROUTESnow name the endpoint, its body, how to clear the link and what it refuses; the docblock's graded routes are re-taken, and the two "NOT here" grades (SCIM, bulk import) re-measured unchanged.5. The docs callout in
content/docs/automation/approvals.mdxcarries the same rewrite in prose, including the directory-precedence rule.STOPPED AT THE
packages/specFENCE — exactly one partApproverType.describe()inpackages/spec/src/automation/approval.zod.tsstill asserts "that column has no product write surface", and that sentence stops being true with this PR. Nothing is pushed for it here.Verified on
origin/mainrather than assumed, as the dispatch required: commit2eb472418(#17640) is an ancestor of HEAD —git merge-base --is-ancestorexit 0, which is self-certifying and needs no control leg. But what that commit landed is a.describe()that points at the remedy while still carrying the now-false assertion. So the third dependent is landed and stale, not landed and fixed. The generatedcontent/docs/references/automation/approval.mdxrenders the string verbatim and is auto-generated, so it is downstream of the same fenced edit.⇒ For the
domain:specseat. It is the only part stopped at the fence.Acceptance notes
manager_idto the import tier is ruled but is not in this lane's scope fence.admin-import-users.tsis untouched and its 0-match reading is re-stated in the lint docblock as still current.sys_business_unit.manager_user_idandsys_user.manager_idstay independent;BusinessUnitGraphService.headOfis not consumed.delegated-admin-gate.tsis a governed permission-model change and ADR-0049 forbids declaring what is not enforced. Recorded as a follow-up, with the Entra precedent attached in the design.Organizationgroup plus a row action posting this endpoint, never a field in the standard edit form.manager:undefinedbecausevalueis omitted formanager; and the approvals: adepartmentapprover never resolves when the business unit hasorganization_id = null(every seeded BU) #3807 "expanded to nobody" pin table inapproval-service.test.tscoversteam/department/position/org_membership_leveland omitsmanager, so that behaviour is real by set membership and not by test. Neither file is touched here.content/docs/permissions/tenant-audit-census.mdxand its counts ledger moved 222 to 223 write call sites. That is this PR's own newengine.update, regenerated withscripts/tenant-audit-census.mjs --write, with the seven hand-written prose figures brought along.Clause-② — re-derived from the delivered diff, not predicted
New exported symbols in the diff, and whether each is reachable from a published entry.
plugin-auth'sexportsmap has exactly two entries (.todist/index.*,./rate-limit-storagetodist/rate-limit-storage.*);admin-set-user-manager.tsis deliberately not re-exported fromsrc/index.ts(0 matches). Measured on the rebuiltdist, with two positive controls that ARE re-exported, so the scan discriminates:dist/index.d.tsdist/rate-limit-storage.d.tsrunSetUserManagerMAX_MANAGER_CHAIN_DEPTHSetUserManagerDepsSetUserManagerEngineSetUserManagerResultSetUserManagerRefusalReasonrunAdminBanUser(control)SYS_USER_PROFILE_EDIT_FIELDS(control)No already-published payload gains a key — the endpoint's response is a new payload, not a new field on an old one. A new route is wire, and wire compatibility is not the Clause-② floor. ⇒ no, changeset
patch.scripts/pm/check-widening-tells.mjs --declaration noexits 0 but reports 8 of 8 files NOT MEASURED ("no declared surface covers it"), so it is not a reading in either direction here and is not offered as one.Clause-②: yesand goes through contract review". The PM claim declaresClause-②: no, explicitly as a prediction to be re-derived from the delivered diff, and names the exact flip condition — a re-export fromplugin-auth/src/index.ts, or a new key on a published payload. Neither holds, so both carriers now readnoand agree. If the seat that owns the vocabulary wants the tier anyway on the strength of the ruling's sentence, that is its call to make, and this note is here so it is made rather than inherited.Checks
Run on
614981107, which is this branch merged withorigin/mainviascripts/pm/os-regen-merge.sh, deps refreshed and every package rebuilt (72/72 turbo tasks) afterwards.pnpm --filter @objectstack/plugin-auth testpnpm --filter @objectstack/lint testpnpm --filter @objectstack/plugin-auth typecheckpnpm --filter @objectstack/lint typecheckscripts/pm/dispatch-gates.mjs --commands)--rancarrying an exit code per family, so the zero is derived rather than claimedpnpm --filter @objectstack/spec check:generatedpnpm exec eslint --no-inline-config(narrowed)Three gate families were red on the first sweep and every one was a real finding of this diff, fixed rather than baselined:
check:doc-authoring(the ledgernotecarried a tracker id — a runtime string no author can resolve; moved to an adjacent comment),check:where-matcher(the new test engine double read a WHERE combinator as a field name; it now refuses loudly), andcheck:tenant-audit-census(the new engine write moved the ratchet). Three more answered exit 3, PREREQUISITE NOT MET —check:skill-examples,check:dual-build-cjs-loads,check:type-check-debt— which is a statement about the tree and never a pass; the closure they name was built and all three then exited 0.The eslint narrowing is a measurement, not a skipped run, so its three pieces of evidence are stated rather than implied: (i) the population comes from
eslint.config.mjs's ownfilesglobs (**/*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}), not from a guess; (ii) the file count is read from--format jsonoutput, 7 entries; (iii) this repo runs oneeslint.config.mjswhich never enables type-aware linting for any file — noparserOptions.project, no typed rules, stated in that file's own header with a recorded positive control — so nothing in this diff can move the verdict on a file it does not touch. The repo-widepnpm lintis CI's run.Generated by Claude Code