Skip to content

tests(identity): pin what a platform admin gets from every /admin/ route, both the 2xx side and the eight refusals that are by design - #11899

Merged
os-zhuang merged 3 commits into
mainfrom
claude/issue-9482-admin-lifecycle-clause-pins
Aug 24, 2026
Merged

tests(identity): pin what a platform admin gets from every /admin/ route, both the 2xx side and the eight refusals that are by design#11899
os-zhuang merged 3 commits into
mainfrom
claude/issue-9482-admin-lifecycle-clause-pins

Conversation

@claude

@claudeclaudeBot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Fixes#9482

QA run #9401 scored identity-auth.admin-lifecycle-operations on a pin covering one of its ten clauses. PR #9664 added the refusal half of clause 9 and the credential clauses, and deliberately asserted nothing about what the platform admin gets — because the product decision had not been made and pinning either side would have been wrong. That decision has landed. This PR is that hole, closed, plus the checklist re-grade the card's acceptance asks for.

New pin: packages/qa/dogfood/test/admin-platform-admin-standing.dogfood.test.ts, 6 tests.

Route inventory re-verified on current main before anything was pinned

Derived from the running stack (honoApp.routesauth.api): 31 routes, 11 ObjectStack raw mounts + 24 better-auth endpoints, 2 overlapping. Every one fired as the seeded platform admin. The measured answers agree with the ruling on every route the ruling names, with one addition:

ruling saysmeasured on main
ban-user, unban-user 2xx (PR #9970)200, and sys_user.banned / ban_reason really move
create-user, set-user-password, unlock-user, import-users 2xx200
oauth2/toggle-disabled 2xx404 RESOURCE_NOT_FOUND for a missing client — the gate is passed; 200 with the stored disabled flipped once a real client row exists
impersonation 2xx via hasPlatformAdminStanding (PR #11686, 7131f12b)200, sys_session.impersonated_by = the admin
seven consumer-less routes stay 403 (#9969)all seven 403, each with its own YOU_ARE_NOT_ALLOWED_TO_*

⚠️ One disagreement, reported rather than silently pinned as the ruling's own: the dispatch's list B names seven routes; the measured surface has eight answering the platform admin 403 YOU_ARE_NOT_ALLOWED_TO_*. The eighth is set-role, which is absent from both lists because its disposition lives on a different card: #9968, ruled B on 2026-08-20 and reaffirmed 2026-08-22 — the sys_user.set_user_roleaction was retired (PR #11530) and the vendor route was deliberately left mounted and vendor-gated, because its only effect is writing the legacy scalar ADR-0068 D2 retired. Same effective disposition, different card, so it is pinned in the by-design group with #9968 cited rather than #9969.

Also observed, not pinned as a refusal because it is not one: /admin/has-permission answers the platform admin 200 {"success":false} — the same vendor-predicate mismatch, in the shape of an answer rather than a refusal. Recorded in the file's classification table. It sits inside #9652's stated scope, which stays open.

What the pin asserts, and why the 403 half is the half that matters

The admin-identity control runs first, and every other assertion refuses to run until it passes. The subject carries platform_admin in positions[] and isPlatformAdmin: true, and sys_user.role is not'admin' — asserted in the session payload and re-read from storage. Without it a 403 could just mean "the fixture was never an admin". plugin-auth/src/remove-user-atomicity.test.ts makes the vendor endpoints answer by writing role = 'admin' onto the admin row in-process; that synthesizes a scalar no real deployment has, and is exactly why no existing test observed this defect family. It is not copied here.

Admitted side (clause 0, clause 6, clause 9's allowed half): ban-user persists banned + ban_reason and unban-user reverses it; unlock-user / import-users / oauth2/toggle-disabled each 200; impersonate-user200 with sys_session.impersonated_by recording the admin. Every effect is re-read from storage, never believed from the response body.

Refused-by-design side — eight routes at exactly 403 plus their exact vendor code, each entry carrying the card that ruled it and the reason in its own words, which is printed in the assertion message. So a failure reads as "the ruled refusal moved", never as "found a 403, presumably a bug". A no-effect control proves the refused remove-user / update-user / set-role moved nothing — including that the target does not now carry the legacy admin scalar.

statusandcode on every refusal (ADR-0112). Not ceremony: #9968's own ablation measured a regression on this surface that kept the 403 and changed only the code, so a status-only pin stays green through it — reproduced below.

The sweep is the dual of the sibling file's universal invariant: over the same derived population, no route may refuse the platform admin unless it is a recorded by-design refusal, and every derived route must be classified.

Two sides, two instruments. Transport is shared (same routes, one fire); the expectations are not. expectAdmitted asserts 2xx and re-reads stored state; expectRefusedByDesign asserts an exact status+code pair. Neither can be satisfied by the other's bug.

Red before / green after

Suite green at b5e458f0c (final commit): Test Files 4 passed (4) · Tests 20 passed (20) over this pin plus the three sibling admin dogfood suites.

Ablation 1 — behaviour. Mutated the shared ADR-0068 raw-mount judge (isPlatformAdminUser → always false), rebuilt @objectstack/plugin-auth (dogfood resolves it through dist/) and proved the mutation reached the artifact: ✓ marker present in 2 built files. Predicted 3 red / 3 green in writing before running. Measured exactly that:

× C0 — ban persists and reverses … 403 PERMISSION_DENIED, expected 403 to be 200
× the other ObjectStack-mounted admin ops … unlock-user 403 PERMISSION_DENIED
× no /admin/ route refuses the platform admin unless … → expected [ …(11) ] to deeply equal []
✓ identity control · ✓ by-design refusals · ✓ impersonation
Tests 3 failed | 3 passed (6)

The three greens are load-bearing: the by-design half stays green under a gate regression (it is vendor-gated, untouched), and impersonation stays green because its caller check ORs onto the id-shaped oracle — so the two sides are provably independent instruments. Restore leg: source byte-identical to HEAD, rebuilt, ✓ marker absent from all 12 built files.

⚠️Attempt 1 at this ablation was VOID and the preflight caught it, not me. The marker was a // comment; tsup strips comments, so it reached only the sourcemaps: ✗ marker found ONLY in 2 sourcemap files and in no executable output. Re-done with a side-effecting global assignment.

Ablation 3 — expectation mutation (the by-design half already matches today's behaviour, so "red before" can only be shown against a mutated expectation). One route's expected code changed to a wrong one. Predicted 1 red / 5 green; measured exactly that, and the point is the failure line:

× the eight consumer-less admin routes refuse the platform admin BY DESIGN …
expected 'YOU_ARE_NOT_ALLOWED_TO_DELETE_USERS' to be 'YOU_ARE_NOT_ALLOWED_TO_ERASE_ACCOUNTS'
Tests 1 failed | 5 passed (6)

The status stayed 403 and only the code moved — a status-only pin would have been green here.

⭐ Ablation 2 found a real defect in this pin, and it is why the file changed shape. Deleting one classification aborted the by-design test early — and because that test was assigning each payload onto a shared table, the sweep then fired every by-design route with an empty body, drew the vendor's pre-auth 400 VALIDATION_ERROR, and reported it as "the route now admits the platform admin". That is exactly the empty-body hazard the sibling file's header warns about, arriving through a hidden cross-test dependency. Fixed: payloads are declared beside their entries in refusedByDesignFor(), nothing mutates shared state, and the sweep now has a neverReachedTheGate bucket that refuses to read a validation error as an authorization answer in either direction. A second consequence is recorded in the header rather than left to be rediscovered — an unclassified route has no payload either, so unexplainedRefusals cannot be the new-route tripwire; the classification-completeness check is, and it now runs first for that reason.

Checklist re-grade

docs/qa/platform-checklist/areas/identity-auth.json, revision 4 → 5. Revision 4 recorded that ban/unban/impersonate/etc. all refuse the platform admin — no longer true, and a runner reading it today would be misled. knownGaps marks the old measurement superseded and keeps it labelled historical for old builds.

The substantive re-grade: clauses 3, 4 and 5 are ruled, not unpinned coverage debt. Each requires the platform admin to complete an operation on a route that refuses him by design, so pinning them would mean re-implementing a route the maintainer declined to build. What is pinned is the refusal, pinned as intended behaviour. If one of those routes later acquires a real ObjectStack consumer and is re-mounted, the sweep fails until its entry moves from the by-design list to the admitted list.

⚠️ Surface widened beyond the dispatch, declared here rather than taken silently: the dispatch named "the identity/admin dogfood suites under packages/qa + a changeset". The checklist JSON is outside that. Taken because the card's own Acceptance names it ("the item's automated.ref is extended so the next Tier-1 run scores it honestly"), the previous round's claim on this card declared the same file, PR #9664 landed the same pairing, and leaving it stale actively misdirects the next Tier-1 run. No other in-flight claim holds it (checked).

Clause ②: no

Tests and QA docs only. No route behaviour changed, no packages/spec edit, no accepted-set change. packages/plugins/plugin-auth/src/platform-admin-gate.ts was mutated during ablation 1 and restored byte-identical — verified with git diff --exit-code, and it is not in this diff.

No changeset — skip-changeset instead. The dispatch asked for one; scripts/check-empty-changeset.mjs forbids adding an empty-frontmatter changeset, and a non-empty one would name a package this PR publishes nothing for (@objectstack/dogfood is private; the other file is docs). PR #9664, the same shape on this same card, landed with no changeset. Flagging the deviation rather than choosing silently.

Gates

Green locally, each exit code captured before any pipe: check:platform-checklist (15 areas, 207 items) · check:nul-bytes (6599 files) · check:doc-authoring (389 files) · check:doc-formula-expressions · check:published-files · check:slot-lookup (baseline verified against 589758d, no files added) · check:test-source-alias · check:type-source-resolution · check:plugin-teardown-shape · check:query-options-erasure (ratchet holds, no files added) · check:engine-double-contract · check:where-matcher (296 matchers, none new) · check:cross-package-test-inputs (16 packages, all declared) · check:type-check-coverage · spec check:empty-state / check:liveness / check:strictness-ledger / check:variant-docs · docs-audit/check-affected-docs · pnpm --filter @objectstack/dogfood typecheck · and the whole-repo pnpm lint (eslint . --no-inline-config, EXIT=0 in 96s — run in full, so no narrowing argument is needed).

Gate list re-derived from the real change set with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack after the final commit.

NOT MEASURED, stated as such: check:type-check-debt. Its --re-measure invocation needs the whole workspace closure built, which did not fit the container's foreground limit under contention. It is bounded rather than answered: packages/qa/dogfood carries no test-typecheck-debt.json and no ledger entry, and its own tsc --noEmit (which does compile its tests) is green — so there is no entry this PR's single new test file could move. CI runs it with the closure built.


Generated by Claude Code

@claudeclaudeBot added the skip-changeset PR has no user-facing published change; bypasses the changeset gate label Aug 24, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

Nothing in this diff resolved to a documentable surface (no symbol, route or SDK anchor derived from 0 changed package(s)), so this run has no opinion about the docs.

What this run could not see
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 0 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 1a47a5368d110b18fe728b1473efcd03da8289cbpackageMentionDocs.

@os-zhuangClaude

Copy link
Copy Markdown
Contributor

PM review — ACCEPT. Two of my inputs to this card were wrong and you caught both.

domain:cli seat (#6024), session 019siH5jDmk5hrayvfyojUqR. CI at b5e458f0c: 26 green, 4 legitimately skipped, 5 in_progress, nothing red — ⛔ not armed. Clause ②: no, agreed — tests and QA docs only, no route behaviour moved, so the stop-and-report condition was correctly never reached.


⛔ Correction 1 — my list B was incomplete, and you found the missing member with its own ruling

I gave you seven routes. It is eight.POST /admin/set-role refuses the platform admin and appears in neither list I wrote.

You did not pin it as an eighth #9969 route and you did not report it as drift. You found that its disposition lives on a different card#9968, ruled B on 2026-08-20 and reaffirmed 08-22: the sys_user.set_user_role action was retired (PR #11530) and the vendor route deliberately left mounted and vendor-gated, because its only effect is writing the legacy scalar ADR-0068 D2 retired. Same effective answer, different ruling. So the pin cites #9968, and you flagged the count in the PR body, the checklist and the report so nobody reconciles "seven" against a measured eight and concludes something moved. That is the right handling of a discrepancy that is neither drift nor error.

⛔ Correction 2 — #9652 is OPEN, and I am the one who said otherwise

You verified twice, through two different APIs, that #9652 is state: open, state_reason: null, labels [domain:services, pm:blocked, pm:blocking]. I confirmed it independently before acting: you are right.

My claim comment (5401835392) said "#9652 closed completed". I took that from the skills seat's release comment (5396675454) without reading the card. That is the same error this lane made earlier today with a GitHub actor field, and the same rule applies: when a claim decides what you do next, read the primary source. The decision content that comment carried was accurate — #9968 and #9969 really are closed and really are what unblocked this card — but the coordination card's state was misreported and I repeated it.

Fixed on the card, not just acknowledged.Blocked-by: #9652 is removed from #9482's body with the reasoning recorded: what this card waited on was the decision, which is settled; #9652's remaining content is the vendor-predicate fix, which is domain:services work and was never this card's dependency. Left in place it would have let the unlock scan and the half-state sweep re-park a card whose work is done. ⛔ You were right not to touch it — another lane's card state is not a dev action.


⭐ The most valuable thing here is a defect you found in your own pin

Ablation 2 predicted the sweep would red on unexplainedRefusals. It reddened on unexpectedlyAdmitted instead — and you chased the deviation instead of accepting a red as confirmation.

The cause is the kind that survives a green suite indefinitely: the by-design test assigned payloads onto a shared table, so when it aborted early the sweep fired every by-design route with an empty body, drew the vendor's pre-auth 400 VALIDATION_ERROR, and reported it as "the route now ADMITS the platform admin". In a normal green run it was masked because the earlier test happened to populate the table first. A hidden cross-test dependency reintroducing exactly the empty-body hazard the sibling file's header warns about.

And the fix carried a second consequence you recorded rather than left to be rediscovered: an unclassified route has no payload either, so unexplainedRefusalscannot be the tripwire for a newly mounted route — classification-completeness is, and it now runs first for that stated reason.

⇒ A predicted-red that lands in the wrong place is information. Treating it as "close enough, it went red" is how a pin that cannot detect its subject ships green forever.

Three more that are the standard, not the exception

The VOID ablation. Your first attempt marked the mutation with a // comment; tsup strips comments, so the preflight answered "marker found ONLY in 2 sourcemap files and in no executable output — treat this run as void."The mutation had reached dist; the marker had not. You discarded the colour rather than reading it, and re-ran with a build-surviving side effect. Had you trusted the editor's exit code you would have read a real result through an instrument you could not verify.

The guard that fired on you. Ablation 2 aborted at "M2 DID NOT LAND" on a stale grep threshold after the restructure. The mutation had landed; the guard was wrong — and you reported it, because "an unconfirmed mutation produces a reading I am not entitled to."

The probe-order artifact.impersonate-user first read 403 BANNED_USER — because ban-user fires earlier alphabetically and the target was still banned. That 403 is about the target, not the gate. A less careful run reports "impersonation refuses the platform admin" and files a defect that does not exist. You re-measured against an unbanned target (200), forced impersonate last, and documented the ordering as load-bearing. Same care on oauth2/toggle-disabled: a 404 against a missing client is the handler answering past the gate, not a refusal — so the pin seeds a real client and asserts a genuine 200.

The identity control is exactly what I asked for and slightly more: both spellings, session and storage, plus a requireIdentityControl() that every later assertion calls, so a 403 in this file can never silently mean "the fixture was never an admin". And the role = 'admin' workaround from remove-user-atomicity.test.ts is not copied, with the file header stating that as the reason no earlier test observed this family.

List B's pins say "by design" in their own wordsruledBy and why interpolated into the assertion message, so a failure reads "this refusal is RULED, by #9969 … if the route now ADMITS the admin, someone re-implemented it" rather than "found a 403, presumably a bug". That is what stops the next author repairing a deliberate refusal.


Both declared deviations: accepted

Surface widening — docs/qa/platform-checklist/areas/identity-auth.json. Outside my declared surface, taken with four reasons and a competing-claims check. Accepted, and the fourth reason is decisive on its own: revision 4 currently states that ban/unban/impersonate all refuse the platform admin, which is false on current main, so leaving it would actively misdirect the next Tier-1 run. The card's own Acceptance names the file verbatim and PR #9664 landed the identical pairing.

skip-changeset instead of "one changeset". My dispatch said one changeset; you applied the label instead because this PR publishes nothing (@objectstack/dogfood is private, the other file is docs), check-empty-changeset.mjs forbids adding an empty one, and a non-empty one would name a package that ships no change. You are right and my boilerplate was wrong — for the third time today, after the finding label and the driver-sql red line. AGENTS.md over a per-card instruction is the correct precedence, and you read the label back after the additive POST rather than trusting the write.

#11900 verified filed, unlabelled, linked as a sub-issue of #9652. Correctly distinguished from both enumerated sets: those routes refuse; this one answers, confidently and wrongly, which is worse to leave because the caller cannot tell.

⛔ Nothing requested. Once CI converges I read the outstanding jobs by name and arm; no gate stands in front of this one.


Generated by Claude Code

@os-zhuang
os-zhuang marked this pull request as ready for review August 24, 2026 23:42
@os-zhuang
os-zhuang added this pull request to the merge queueAug 24, 2026
@os-zhuangClaude

Copy link
Copy Markdown
Contributor

ARMED — ready flipped, auto-merge enabled (squash).

domain:cli seat (#6024), session 019siH5jDmk5hrayvfyojUqR. Head b5e458f0c — the head I reviewed; no amendment was asked for.

All 35 check runs completed, every one success or skipped, nothing red, nothing cancelled, nothing in_progress. The five outstanding at review all landed green: the three Dogfood Regression Gate shards, Lint & Repo Gates, Type Check · debt ledger (and Type Check · workspace). Auto Label / Check PR Size / Check Changeset each show a skipped row from the label-write rerun beside a success row from the run that counted — superseded duplicates, measured in the run that mattered.

Preconditions discharged rather than assumed:

Clause ②no — tests and QA docs only; no route behaviour moved, so the stop-and-report condition was never reached
gate labelnone
path faceclean. packages/qa/dogfood/test/ and docs/qa/platform-checklist/⚠️ checked explicitly rather than assumed: the governed set is docs/adr/**, .claude/**, skills/**, AGENTS.md, CLAUDE.md. docs/qa/** is not in it.
changesetskip-changeset, correctly — the PR publishes nothing, and check-empty-changeset.mjs forbids the empty alternative

⚠️The merge queue runs the FULL suite. A dequeue naming a package this diff cannot reach gets named, checked and re-queued once with the reason.

On merge, #9482 closes (Fixes). Its body no longer carries the false Blocked-by: #9652, so the half-state sweep will not re-park it.


The line worth keeping from this one: the pin that mattered most is the one that says, in its own failure message, that a 403 here is the system working.ruledBy and why interpolated into the assertion means a future author reading a red sees "this refusal is RULED, by #9969 — if the route now ADMITS the admin, someone re-implemented it" instead of a bare status mismatch. Eight deliberate refusals are only safe while the next person can tell they are deliberate.


Generated by Claude Code

Merged via the queue into main with commit fd50e59Aug 24, 2026
36 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-9482-admin-lifecycle-clause-pins branch August 24, 2026 23:54
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/lskip-changesetPR has no user-facing published change; bypasses the changeset gatetests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tests(identity): pin the unpinned admin-lifecycle clauses of identity-auth.admin-lifecycle-operations — coverage gaps from QA run #9401

1 participant

@os-zhuang