Skip to content

fix(plugin-security): choose the platform-admin promotion target instead of sampling it — order the candidate read server-side and prefer the declared owner - #16863

Draft
os-trump wants to merge 4 commits into
mainfrom
claude/issue-16682-first-user-promotion-selection
Draft

fix(plugin-security): choose the platform-admin promotion target instead of sampling it — order the candidate read server-side and prefer the declared owner#16863
os-trump wants to merge 4 commits into
mainfrom
claude/issue-16682-first-user-promotion-selection

Conversation

@os-trump

@os-trump os-trump commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Fixes #16682

The single-posture first-boot promotion read sys_user with no orderBy and a cap of 50, then sorted that array client-side. So "the oldest authenticable user" meant the oldest authenticable user among whatever 50 rows the driver produced first — a sample sorted and reported as a global answer. And PLATFORM_OWNER_EMAIL_ENV, imported into that same file, was read only on the walled branch, so a deployment that had declared its owner could still have someone else promoted.

Both halves land together, per triage's ruling: ordering alone still promotes someone the operator never chose, and honouring the declaration alone leaves the no-declaration path sorting a truncated unordered sample.

Measured, on this branch, both drivers

113 seeded sys_user rows, 7 holding credentials, the intended owner inserted first with the oldest created_at and an id that collates last, OS_PLATFORM_OWNER_EMAIL=admin@objectos.ai:

driver the 50-row window promoted BEFORE promoted AFTER
memory (InMemoryDriver) window[0] = usr_zzz_owner (insertion order) admin@objectos.ai admin@objectos.ai
sqlite (SqlDriver, the default) window[0] = usr_ats_c001 — the owner is not in the window at all (id order) candidate001@mail.example admin@objectos.ai

Re-measured with OS_PLATFORM_OWNER_EMAIL unset as well (the pure ordering leg): before, the same split; after, admin@objectos.ai on both drivers, basis: oldest-authenticable. The raw unordered 50-row window is still driver-shaped after the change — that is a fact about the drivers, and the file pins it as an anti-vacuity case. What is no longer driver-shaped is the answer.

What changed

  • The read is ordered where the driver can see itcreated_at ascending with id as tie-breaker (seeded populations routinely share one timestamp, and among ties an unordered read is exactly the sample-dependent answer this fixes). There is deliberately no client-side re-sort left behind: one would re-rank the returned page and keep the guard passing if the ordering were ever lost again.
  • The declared owner is asked first — a declared, authenticable, human holder of an OS_PLATFORM_OWNER_EMAIL address is the target. isHumanUser still applies, so a declared address on a role: 'system' row is not a route to the grant.
  • A declared owner nobody can sign in as REFUSESreason: 'declared_owner_not_authenticable', a warning naming the variable and the addresses, and no grant row at all. No silent fall-back to whoever happens to be oldest; that is the outcome the card is about. The replay predicate promotes the declared owner as soon as their login exists.
  • Verified matches rank ahead of unverified ones among rows holding one declared address. matchesConfiguredPlatformAdmin states the threat for the walled derivation — "an attacker who registers the operator's address before the operator does gains no standing by it" — and this answers it by ORDER rather than by refusal: somebody who cannot read the operator's mailbox cannot verify. It is a preference, not a requirement, because single has no verification vocabulary (bootstrap-platform-admin-walled-owner.test.ts pins that an unverified first user is still promoted here), and making single verification-keyed is a policy change, not a repair.

The cap's disposition

Replaced, not merely raised, and never silent again. The bare 50 becomes PLATFORM_ADMIN_CANDIDATE_PAGE_SIZE = 200 walked oldest-first with a hard PLATFORM_ADMIN_CANDIDATE_SCAN_CEILING = 5000.

  • A ceiling is kept rather than dropped because this pass re-runs on every sys_user / sys_account insert until an admin exists (shouldReplayBootstrapFor), so an unbounded scan would be a per-sign-up full-table read on exactly the deployments that have not been promoted yet.
  • The difference from the old constant is the orderBy: an ORDERED page holds the OLDEST rows, which is exactly the set the age rule ranks, so truncation can only bite when every one of the oldest 5000 humans is non-authenticable. The unordered 50 could drop the answer on a 51-row install.
  • Reaching the ceiling WARNS, naming the number examined. Both constants are exported so the guard reads the same numbers the selection does rather than restating them.

The upgraded log line — exact text

Prefix unchanged (existing readers match on it); the basis and the pool are appended, and repeated in meta as basis / candidatePoolSize / userId for structured sinks. The returned report carries basis too.

[security] first user promoted to platform admin: admin@objectos.ai — basis: declared-owner; candidate pool: 1 address(es) declared in OS_PLATFORM_OWNER_EMAIL, 1 matching human user row(s)
[security] first user promoted to platform admin: admin@objectos.ai — basis: oldest-authenticable; candidate pool: 113 human user row(s) examined oldest-first by created_at

Truncation warning:

[security] the platform-admin candidate scan stopped at its ceiling of 5000 oldest sys_user row(s) and none of them can authenticate — rows beyond that point were NOT examined, so this deployment may hold a promotable human the boot did not see. Promote the intended administrator explicitly by setting OS_PLATFORM_OWNER_EMAIL.

验收备注

Triage's rubric, copied, each condition with its evidence. All in packages/plugins/plugin-security/src/bootstrap-platform-admin-promotion-selection.test.ts (23 cases) unless noted.

  1. 卡面给的回归测试形状原样采用 — seed 50-plus users with the intended owner sorting last by id and first by created_at, then assert the same user is promoted across driver row orders. ✅ The 113-row fixture is the card's, verbatim. ⚠️ The literal "memory AND sqlite" arm is a declared deviation — see the section below. What is pinned instead covers strictly more orders: three natural orders (AS_RETURNED, INSERTION, REVERSED), all on the real engine over the real better-sqlite3 driver, plus a case asserting all three agree. An anti-vacuity case proves the unordered 50-row window really does hide the owner on the real driver, so the file cannot pass for a reason unrelated to the repair.
  2. 声明 owner 的腿单独一条OS_PLATFORM_OWNER_EMAIL set to a user that is neither the oldest (its created_at is the newest of 113) nor in the first 50 rows (its id collates last); asserted promoted under all three orders, with basis: declared-owner, and explicitly not the oldest authenticable human. ✅
  3. 阴性对照必测:
    • 未设 OS_PLATFORM_OWNER_EMAIL 时回落到最老的可认证用户,且两驱动一致 — a fixture deliberately different from condition 1, where the oldest authenticable row is not the id-last row, so "oldest wins" and "the owner happens to sort last" are separated. Same answer under all three orders. ✅
    • plugin-security promotes the OLDEST human sys_user row, so an app that seeds a people directory grants platform admin to a row nobody can log in as #14348 修的那条仍然成立 — two cases. The two oldest rows are credential-less directory rows and the only login is newer than both: the login is promoted, under all three orders — so an implementation that simply took min(created_at) fails here while condition 1 stays green. And a 60-row population where nobody can authenticate promotes nobody (no_authenticable_user, zero grant rows, and no warning, because that population is far under the ceiling). ✅
    • 设了 OS_PLATFORM_OWNER_EMAIL 但该邮箱没有可用账号 — two sub-cases, no sys_user row at all and a row with no sys_account. Both refuse: declared_owner_not_authenticable, a warning naming the variable, the address and the phrase "NOT falling back to the oldest", and zero grant rows written. ⛔ No silent fall-back. ✅
    • Extra, not asked for but adjacent: the reserved fork (plugin-security promotes the OLDEST human sys_user row, so an app that seeds a people directory grants platform admin to a row nobody can log in as #14348 case D) re-asserted against the new selector — an existing unscoped grant still short-circuits to already_have_admin before any selection runs, so a declared owner cannot re-point an existing platform admin. ✅
  4. cap 的处置写进 PR — see "The cap's disposition" above. Any limit that remains warns on exceed, pinned in both directions (a population over the ceiling warns naming it; one inside it produces no such warning). ✅
  5. 日志升级 — exact text above; three cases pin the basis, the pool, the meta fields, and that the published prefix still matches. ✅
  6. ⛔ 不要改 :426 那处精确读 — untouched. git diff over the whole change contains no edit to that line. ✅

Declared deviation: the memory-driver arm is measured, not pinned

Condition 1 asks for the assertion on the memory driver and sqlite. @objectstack/driver-memory cannot be imported into this suite, for two reasons that are each outside a repair's authority:

  1. Declaring it needs packages/plugins/plugin-security/package.jsonclaimed by open PR fix(plugin-security)!: evaluate the insert-side RLS check on the row that will be stored, after beforeInsert #16805 under the single-writer rule, which the dispatch names as a stop-and-report.
  2. Independently, every declaration of that package in this repo must be disposed of in scripts/driver-memory-census.ledger.json, and a test consumer's only fitting axis is ruled-permanent, which the ledger states is "a maintainer ruling and lives in ruledConsumers; nothing else may claim it." The gate's own header says a third arrival "is now refused at the gate".

So the real memory-driver readings are in the table at the top of this PR — taken out-of-tree on this branch, before and after, and reported here rather than pinned. What the suite pins in their place is the property those two drivers were standing in for, over more orders than they produce between them, with the un-permuted arm being an ordinary real-driver run. If the maintainer wants the literal two-driver arm, it needs the ledger entry and the manifest edit, and both are theirs to make.

The facade is honest by construction: it permutes a result only when the query carried no orderBy — which is precisely the freedom a driver has there — and forwards an ordered query verbatim, returning the real SQL engine's rows untouched. It never sorts. So a fix that sent orderBy to a driver that ignored it would still be caught.

A pin was re-authored, and it deserves a maintainer's eye

bootstrap-platform-admin-walled-owner.test.ts carried a case asserting the exact behaviour triage ruled defective:

it('never consults the owner-email variable: a declared owner does NOT redirect the single-org promotion')

It is #11974's over-denial guard, and what it guards is unchanged: retiring the WALLED write must not retire the single one — adminPromoted === true with a grant row actually minted, still asserted. What changed is the incumbent it happened to snapshot alongside that invariant, and the file header's "byte-for-byte" wording. The case is re-authored with the ruling that replaced it, quoted verbatim in the test:

只做 1(服务端排序 + 去掉 cap)让结果与驱动无关、可复现 —— 但它仍然可能提升一个运营者没选的人(最老的可认证用户未必是 owner)。⇒ 修掉了不确定性,⛔ 没修掉安全性。

Flagging it explicitly because it is a collision between two rulings — #11974 / Choice 4A (2026-08-25) and #16682's triage (2026-09-08) — resolved in favour of the newer one because the dispatch carries it as binding.

Security boundary

This narrows who receives admin_full_access, which is the ruled direction. Three things worth naming rather than leaving to be found:

  • A grant is now written in one case where none was before: a promotable human outside the old 50-row window is now found. That is a false negative being repaired, not a new grantee class — the policy ("the oldest human that can authenticate, and only when no admin exists") is unchanged, and plugin-security promotes the OLDEST human sys_user row, so an app that seeds a people directory grants platform admin to a row nobody can log in as #14348's rule is preserved exactly.
  • The declared-owner leg does not require email_verified, for the reason above; it prefers it. Relative to the incumbent this is not a widening — today's rule hands the grant to whoever registers first, with no config knowledge required at all — but it is a deliberate choice and the maintainer's to overrule.
  • packages/cli's os meta resync calls this function and therefore inherits the declared-owner preference. Intended, and consistent.

Verification

Final head 141876814, on a tree merged with origin/main (no STALE TREE warning from the deriver).

  • pnpm --filter @objectstack/plugin-security test103 files, 1925 tests, all passing.
  • pnpm --filter @objectstack/plugin-security typecheck — clean, including check:test-typecheck (0 files / 0 errors / 0 pinned signatures in debt).
  • pnpm lint — the full repo-wide eslint . --no-inline-config, exit 0. Not narrowed, so no narrowing evidence is owed.
  • Derived gate union: 66 families, 66 run, 0 UNRUN (dispatch-gates --ran, reconciled). 63 exit 0. The three non-zero are all exit 3, PREREQUISITE NOT METcheck:dual-build-cjs-loads, check:i18n, check:type-check-debt each require a full workspace build that CI does before them, and each says in its own words that this is "NOT a pass" and nothing was measured. Zero findings (exit 1).
  • Three gates did red as real findings against the new doubles and were fixed rather than baselined: check:engine-double-contract (update() now routes through assertEngineUpdateDispatch; the pinned ledger grew via --write, the shrink-only baseline untouched), check:objectql-double-limit (the caller's bound applied after the filter, by presence), check:where-matcher (the matchers refuse a $-combinator by name instead of reading it as a field).
  • check:route-envelopedoes not apply, measured not assumed: the whole change adds 0 lines matching c.json( or res.json(.
  • Governed surfaces (docs/adr/**, .claude/**, skills/**, AGENTS.md, CLAUDE.md): none touched, so no maintainer-speed-read section is owed here.
  • Path constraint honoured: the change touches none of packages/plugins/plugin-security/package.json, tsconfig.json or vitest.config.ts — the three files PR fix(plugin-security)!: evaluate the insert-side RLS check on the row that will be stored, after beforeInsert #16805 claims. The suite reaches the real driver through devDependencies and vitest aliases that already existed.

Clause ② derivation: no

  • node scripts/pm/check-widening-tells.mjs --declaration no --diff PR.DIFFexit 0: "4 changed file(s) read, no widening tell on any declared surface."
  • node scripts/pm/dispatch-gates.mjs --tier → "no path-derived mandate: the surface hits none of the 3 declared glob(s)".
  • Content judgment, since the tier line is a floor and not a clearance: the direction is narrowing (fewer promotable people; a declared owner who cannot authenticate now refuses where somebody was previously promoted), no accept-set gains a spelling or a value, and the only public-surface movement is an optional basis field on a returned object — additive, and no consumer can be rejected by it.
  • needs:contract-review is therefore not hung. Say the word and it goes on both carriers.

Because the change is additive on a cross-package type, the "inject a key the new type rejects" recipe has nothing to reject. The equivalent two-legged proof was run against packages/cli, the one package importing this function as a value, after rebuilding plugin-security:

  • positive leg — a probe reading report.basis into 'declared-owner' | 'oldest-authenticable' | undefined produced 0 errors on the probe file, so the consumer read the rebuilt .d.ts and not a cache;
  • negative leg — assigning report.basis to number produced error TS2322: Type 'string | undefined' is not assignable to type 'number', so the type is live and rejecting. (The package's 111 pre-existing errors are unbuilt sibling packages, identical on both legs; the probe was removed and git status for packages/cli is clean.)

Ablation — four enforcement points, each proven able to fail

Each leg mutated one point, proved the mutation reached disk (anchor grep -c 1 to 0, injected-text count 0 to 1, and git hash-object differing from the HEAD blob), ran the guard, then restored with git checkout HEAD -- ABSPATH under a trap ... EXIT INT TERM and proved the restore (blob equal to the HEAD blob and an empty git diff HEAD).

ablation mutation result
A1 ordering drop OLDEST_FIRST from the candidate read 3 RED — including AS_RETURNED, the un-permuted real-driver arm
A2 declared owner disable the declared-owner leg 10 RED
A3 truncation warning make the scanTruncated branch unreachable 1 RED
A4 log line strip the basis and pool from the promotion line 5 RED

A fifth attempt was refused by the harness, not by me: replacing the log fragment with an empty string makes the injected-text count unobservable (grep -c "" matches every line), so the run was aborted as a possible no-op and redone with a countable marker. The tree is at HEAD with git status clean and the suite green after all four legs.

Out of scope, filed

  • plugin-security: the already_have_admin short-circuit reads sys_user_permission_set with an UNORDERED cap of 50, so an existing unscoped platform admin can be missed and a SECOND one minted #16861 — the same defect class on the other read in this function: already_have_admin reads sys_user_permission_set with an unordered cap of 50 and applies the deciding !organization_id predicate client-side, so on an install with 50-plus organization-scoped grants of admin_full_access the existing unscoped holder can be missed and a second unscoped admin minted. Not folded in: triage scoped this card to the promotion read, and repairing that one changes when the short-circuit fires, which is permission-boundary behaviour needing its own tests.
  • Noted, not filed: sys_user.email carries a UNIQUE index, so on the SQL family two rows can never hold one address — measured as SQLITE_CONSTRAINT_UNIQUE while writing the verified-first tie-break's fixture, which is why those two cases run on a double.

Generated by Claude Code

…ead of sampling it

The `single`-posture first-boot promotion read `sys_user` with no `orderBy`
and a cap of 50, then sorted that array client-side. So "the oldest
authenticable user" meant the oldest authenticable user among whatever 50
rows the driver produced first, and a client-side sort cannot notice: it
sorts a sample and reports a global answer.

Measured on 113 seeded users with the intended owner inserted first, holding
the oldest created_at and an id that collates last: the in-memory driver
returned it in row 1 and promoted it; the default sqlite driver returned id
order, never saw it, and gave the unscoped admin_full_access grant — plus,
through claimSeedOwnership, ownership of every seeded business record — to a
seeded job-seeker persona. Same code, same config, same data.

PLATFORM_OWNER_EMAIL_ENV was imported into this same file and read only on
the walled branch, so a deployment that had declared its owner could still
have somebody else promoted. Both halves land together: ordering alone still
promotes someone the operator never chose, and honouring the declaration
alone leaves the no-declaration path sorting a truncated unordered sample.

- the candidate read carries orderBy created_at asc, id asc, and no
  client-side re-sort is left behind
- a declared, authenticable, human holder of an OS_PLATFORM_OWNER_EMAIL
  address is preferred; verified matches rank ahead of unverified ones
- a declared owner nobody can sign in as REFUSES loudly and promotes nobody,
  never falling back to whoever happens to be oldest
- the bare cap 50 becomes a 200-row page with a 5000-row ceiling walked
  oldest-first, and reaching the ceiling warns with the number examined
- the promotion log line and the returned report record the basis and the
  candidate-pool size

Unchanged: no declaration still means promotion by age; a user nobody can
authenticate as is still never promoted (#14348); an existing unscoped grant
still short-circuits before any selection runs.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37
…ared engine-double contracts

The three doubles the new selection guard introduces are brought up to the
contracts the repo's gates enforce, and the pinned ledger is grown so it
actually protects this file:

- update() routes through assertEngineUpdateDispatch, so a fixture drifting
  to a call shape ObjectQL.update would refuse fails loudly
  (check:engine-double-contract)
- find() applies the caller's limit AFTER the filter and by presence
  (check:objectql-double-limit)
- the WHERE matchers refuse a $-combinator by name instead of reading it as
  a field and answering false (check:where-matcher)

scripts/engine-double-contract.pinned.json gains the new seams via
`--write`; the shrink-only baseline is untouched.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37
@github-actions github-actions Bot added size/xl documentation Improvements or additions to documentation tests tooling labels Sep 8, 2026
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/plugin-security, touching 8 documentable anchor(s).

9 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/data-modeling/objects.mdx (via sys_user_permission_set (literal, a string literal in bootstrapPlatformAdmin))
  • content/docs/data-modeling/validation-rules.mdx (via sys_account (literal, a string literal in bootstrapPlatformAdmin))
  • content/docs/deployment/environment-variables.mdx (via sys_user_permission_set (literal, a string literal in bootstrapPlatformAdmin))
  • content/docs/deployment/self-hosting.mdx (via sys_account (literal, a string literal in bootstrapPlatformAdmin))
  • content/docs/permissions/authentication.mdx (via sys_account (literal, a string literal in bootstrapPlatformAdmin))
  • content/docs/permissions/authorization.mdx (via sys_user_permission_set (literal, a string literal in bootstrapPlatformAdmin))
  • content/docs/permissions/delegated-administration.mdx (via sys_user_permission_set (literal, a string literal in bootstrapPlatformAdmin))
  • content/docs/permissions/permission-sets.mdx (via sys_user_permission_set (literal, a string literal in bootstrapPlatformAdmin))
  • content/docs/protocol/objectui/actions.mdx (via sys_account (literal, a string literal in bootstrapPlatformAdmin))

5 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/implementation-status.mdx (via sys_user_permission_set (literal, a string literal in bootstrapPlatformAdmin))
  • content/docs/releases/v13.mdx (via sys_user_permission_set (literal, a string literal in bootstrapPlatformAdmin))
  • content/docs/releases/v14.mdx (via sys_user_permission_set (literal, a string literal in bootstrapPlatformAdmin))
  • content/docs/releases/v16.mdx (via sys_user_permission_set (literal, a string literal in bootstrapPlatformAdmin))
  • content/docs/releases/v17.mdx (via tryFind (symbol, a top-level function), sys_account (literal, a string literal in bootstrapPlatformAdmin), sys_user_permission_set (literal, a string literal in bootstrapPlatformAdmin))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 2 anchor(s) matched too much of the corpus to be a work list: created_at (literal, 33 pages), sys_user (literal, 31 pages)
  • the SDK route bridge reached 60 of 216 client-bound route-ledger rows — the other 156 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 156: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 100 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • 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 — 15 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 12babac137cc826fa5ed888ce63c266c4d219ce3packageMentionDocs.

Which tree this was computed on

This run read content/docs from 3a12d9d825fea3ab65bbc7164a1d6c3eac38efcc — the merge of head 141876814df6763fca3b71f0b1ed970ad196869c into base 12babac137cc826fa5ed888ce63c266c4d219ce3, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 3a12d9d825fea3ab65bbc7164a1d6c3eac38efcc && git checkout 3a12d9d825fea3ab65bbc7164a1d6c3eac38efcc
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 12babac137cc826fa5ed888ce63c266c4d219ce3 141876814df6763fca3b71f0b1ed970ad196869c && git checkout -B drift-repro 12babac137cc826fa5ed888ce63c266c4d219ce3 && git merge --no-ff 141876814df6763fca3b71f0b1ed970ad196869c

node scripts/docs-audit/affected-docs.mjs --json 12babac137cc826fa5ed888ce63c266c4d219ce3

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 12babac137cc826fa5ed888ce63c266c4d219ce3 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

Copy link
Copy Markdown
Contributor

Contract review (claude-fable-5-1, isolated seat) — PR #16863 @ 141876814

Verdict: CHANGES REQUIRED

Ruling implemented: yes — the PR implements exactly the triage ruling on #16682 (comment 5578557793, by os-zhuang, author_association MEMBER, self-described as 分诊席 — a triage seat, ⛔ not a maintainer ruling): 「裁定:修法 1 + 2 都做,同一个 PR,⛔ 不要只做 1」, the six 验收口径 conditions, and 「⛔ 不要改 :426 那处精确读」. Both halves land in one change; :426's exact read is byte-identical at head (:491, tryFind(ql, 'sys_user', { id: holder.user_id }, 1)). Condition 1's literal "memory AND sqlite" arm is a declared deviation (three row orders on the real sqlite driver instead); the reasons given (single-writer claim on package.json by #16805, driver-memory-census.ledger.json ruled-permanent is maintainer-only) are both true against the tree.

Governed paths touched: NO — the diff vs merge-base ce8bfc9d6 is exactly 5 files: .changeset/platform-admin-promotion-selection.md, packages/plugins/plugin-security/src/bootstrap-platform-admin.ts (+338/−46), …/bootstrap-platform-admin-promotion-selection.test.ts (new, 812), …/bootstrap-platform-admin-walled-owner.test.ts (±56), scripts/engine-double-contract.pinned.json (+5). None under docs/adr/**, .claude/**, skills/**, AGENTS.md, CLAUDE.md, content/docs/releases/**. It does NOT touch packages/plugins/plugin-security/package.json, tsconfig.json or vitest.config.ts.

Clause-②: yes. Derivation, on the diff and the repo's own rules, not the body:

  • bootstrapPlatformAdmin is re-exported by name from packages/plugins/plugin-security/src/index.ts:33; the diff adds basis?: 'declared-owner' | 'oldest-authenticable' to its return type ⇒ a new key on a published payload. references/contract-review.md is unconditional here: 「机械地板 claim 时可查树:新导出符号或已发布载荷上的新键恒 yes,直接锁契约复审档」. The PR's "optional and additive, so no consumer can be rejected" is a conformance argument, and the same file says 「conformance 类 ⛔ 不机械化」 — it does not move the floor. The PM seat's correction (5586852304) reached the same yes; this review confirms it independently.
  • The two new export consts are NOT reachable from the published entry point (named re-export, exports map publishes only .) — correctly out of scope.
  • check-widening-tells.mjs exit 0 is expected and carries no information here: its T1/T2 surface is SUSPECT_TIER_GLOBS (packages/spec/src/**), T3 is packages/spec/api-surface/*.json, T4 is three named registries. packages/plugins/** is on none of them — that green is "did not look".
  • The behaviour change itself (who receives admin_full_access) is NOT what makes this clause-② — SKILL.md:516 「运行时权限/安全行为变更不是条款②,归人工地板安全/权限边界类」 — it is what makes this maintainer-only (see F3/F4).
  • Changeset level implied: the changeset exists and grades @objectstack/plugin-security: patch. check-changeset-no-major.mjs's LEVEL axis (its header: a PR that declares clause ② "may not grade a package it grew patch"; maintainer ruling 2026-09-04 on finding(changeset): two independent contract reviews read the repo's own history to opposite bumps for "add an exported symbol to a published index" #15294: an additive widening of a published surface "takes AT LEAST minor") cannot see this package: PUBLISHED_SOURCE_PATH = /^packages\/([^/]+)\/src\// (:822) matches one segment, and this file is packages/plugins/plugin-security/src/…[finding] The changeset LEVEL axis is blind to every NESTED package: packages/*/src/** matches one segment, so 51 of 74 workspace packages (all drivers/services/adapters) can pair Clause-②: yes with patch and stay green #16713 exactly. Check Changeset green on this PR is therefore "did not look", not "approved". ⇒ the changeset must be minor (F1).

Verification

  1. Diff vs merge-base — 4 commits (2 substantive + 2 origin/main merges), file list as above; git diff --name-only filtered against the governed globs and the three fix(plugin-security)!: evaluate the insert-side RLS check on the row that will be stored, after beforeInsert #16805 config paths returns nothing.
  2. Source change (bootstrap-platform-admin.ts): tryFind gains optional orderBy/offset and only puts them on the query when passed, so every pre-existing call site sends the same query. Leg 1 reads resolvePlatformAdminEmails() (memoized parser from @objectstack/core), queries sys_user by both spellings, filters normalizePlatformAdminEmail(row.email) === email + isHumanUser, sorts verified-first then byCreatedAtAsc, takes firstAuthenticable; refuses with declared_owner_not_authenticable and a warn when none. Leg 2 pages OLDEST_FIRST = [created_at asc, id asc]. Both legs run AFTER the already_have_admin short-circuit (:477 vs :754), which is unchanged.
  3. The scan loopfor (offset = 0; offset < 5000 && !target; offset += 200): breaks on empty page, on target, on a short page; sets scanTruncated only when offset + page.length >= ceiling. Terminates in ≤ 25 iterations even against a driver that ignores offset. orderBy reaches the driver: query.orderBy is set when passed; ObjectQL engine.ts:415 whitelists orderBy/offset and :9376 parses each node with SortNodeSchema ({ field, order: 'asc'|'desc' } — the shape the diff sends); SqlDriver applies it at sql-driver.ts:9127/:14790 and its paging tie-breaker (id) is already the second key. The truncation warning fires only when no target was found AND the ceiling was hit — pinned in both directions by the synthetic cases (ceiling + page ⇒ warns; ceiling − page ⇒ silent). shouldReplayBootstrapFor is unchanged (sys_user/sys_account × create/insert × non-walled), so a declared owner whose login lands later is promoted on that insert's replay — leg 1 runs on every pass.
  4. The re-authored pin (walled-owner.test.ts:461): the over-denial invariant IS still asserted — expect(r.adminPromoted).toBe(true) and expect(ql.grants()).toHaveLength(1); what moved is grants()[0].user_id from u_first to u_second plus basis === 'declared-owner'. Stated plainly: a pin that asserted a behaviour recorded under a maintainer ruling (platform-admin re-anchor L4 (plugin-security): bootstrap stops granting under walled postures; explain reports config-derived standing; deprecation log for legacy grants #11974 / Choice 4A — [Design] Re-anchor platform-admin: admin_full_access becomes a kernel metadata declaration; WHO holds it comes from env-configured verified emails — retiring the org-less row anchor #11663 comment 5404675670, 2026-08-25, 「接受你的建议,继续」; and the feat(plugin-security): walled bootstrap stops minting the platform-admin grant row; platformAdmin audit service; legacy-grant deprecation pointer (L4) #13514 contract-review PASS by zhuangjianguo, director seat, which recorded as verified 「single 晋升读最老 human、从不读 email/email_verified」) was rewritten under a triage seat's ruling of 2026-09-08. The dev named the collision honestly; it is not the dev's or the PM's to resolve.
  5. Security, declared-owner leg without email_verified — measured against the diff, precisely:
    • FROM (base, single, variable set or unset): the oldest authenticable human among an unordered 50-row driver window is promoted. An attacker who registers FIRST on an empty install is promoted; no config knowledge needed.
    • TO (head, variable unset): same policy, now over the full ordered population — the attacker-who-registers-first is promoted exactly as before.
    • TO (head, variable set): the attacker-who-registers-first is promoted ONLY if they hold the declared address; otherwise the pass refuses (nobody promoted) — a strict narrowing for that attacker.
    • ⚠️ But there is one direction that is NOT a narrowing: a legitimate human registers first (address ≠ declared), then an attacker registers the declared address, unverified. FROM: the legitimate first user is promoted. TO: the attacker is promoted (basis: declared-owner), because verified-first is a tie-break that only helps when a verified holder EXISTS, and on the SQL family sys_user.email is UNIQUE so the operator cannot even register alongside the squat. The PR's "the set of winning attackers strictly shrinks" is false for this case; the price of the squat is knowing the declared address (env, but typically admin@<domain>). This is the walled derivation's named threat, answered by refusal there and by preference here. Maintainer's call (F4).
  6. Tests — 19 literal it( in the new file; two sit inside for (const order of NATURAL_ORDERS) (3 orders) ⇒ 23 runtime cases, matching the claim. No .skip / .only / .todo / xit. ⛔ Not executed by this seat: this checkout has no node_modules and the suite needs the real better-sqlite3 driver, so a run is not cheap; the ablation table was reasoned from source instead. A1 (drop OLDEST_FIRST) reds under AS_RETURNED because sqlite's id order puts usr_ats_c001 (has an account) before usr_zzz_owner and there is no client re-sort — confirmed. A2/A3/A4 red on the assertions named. One structural gap in F5.
  7. CI on the head — 37 check runs, 0 failures (all success or skipped); mergeable_state: clean; PR is draft: true. RULE 2: none of the 4 commits carries a Fixes/Refs/Part of trailer (the only card mention is prose (#14348) in a bullet); the "Part-of PR must not also close its card" job, which runs check-partof-closing-keyword.mjs over the commit list, is green. Advisory only.
  8. plugin-security: the already_have_admin short-circuit reads sys_user_permission_set with an UNORDERED cap of 50, so an existing unscoped platform admin can be missed and a SECOND one minted #16861 — exists, open, bug/security/domain:services, and describes the already_have_admin read (sys_user_permission_set, unordered, cap 50, !organization_id applied client-side). Correctly filed out of scope.
  9. Single-writer — measured on fix(plugin-security)!: evaluate the insert-side RLS check on the row that will be stored, after beforeInsert #16805's current file list (11 files): it modifies scripts/engine-double-contract.pinned.json, which this PR also modifies. Collision confirmed (F2).

Findings

F1 — blocking. Changeset graded patch while the PR is clause-② yes (new key basis on a published payload). The LEVEL axis is blind to this package (#16713), so no gate will catch it. Expectation: regrade .changeset/platform-admin-promotion-selection.md to "@objectstack/plugin-security": minor.

F2 — blocking for landing (not a defect in the diff). scripts/engine-double-contract.pinned.json is also modified by open PR #16805; under single-writer this PR cannot enter the queue until #16805 lands or the maintainer sequences them. Expectation: land after #16805 and re-merge origin/main, or the maintainer rules the order.

F3 — blocking; maintainer ruling required. A pin recorded under a maintainer ruling (Choice 4A, and the #13514 PASS's 「从不读 email/email_verified」) is reversed under a triage seat's ruling. The diff is consistent with the newer ruling; whether the newer ruling stands is not a seat decision. Expectation: maintainer confirms that under single a declared owner may decide the promotion (option A in the dev report), or reverses — in which case leg 1 is withdrawn and #16682 falls to fix 1 alone.

F4 — blocking; maintainer ruling required. The declared-owner leg does not require email_verified; item 5 above shows one non-narrowing direction (a squat of the declared address now outranks a legitimate older first user). Expectation: maintainer chooses A (as landed, preference only) or B (require verified on this leg; note UNIQUE email means B refuses until the address is reclaimed). Whichever is chosen, the changeset should state the threat and the answer in one sentence.

F5 — not blocking. The card-shape fixture is 113 rows, below PLATFORM_ADMIN_CANDIDATE_PAGE_SIZE = 200, so the whole population fits one page. The guard catches a lost orderBy today only because no client re-sort exists; a future "defensive .sort()" plus a lost orderBy would go green. The anti-vacuity case measures the OLD 50-row window, not the new page. Expectation: grow the card-shape population past the page size (e.g. 250) or add one case at PAGE_SIZE + 1 with the owner id-last.

F6 — not blocking. With the variable set and the owner not yet registered, every sys_user/sys_account insert replays the pass and re-emits the full refusal warn — one warning per sign-up until the owner appears. Expectation: consider a once-per-process latch like reportLegacyPlatformAdminGrant's, or accept and say so in the changeset.

F7 — not blocking. The PR body's "Clause ② derivation: noneeds:contract-review is therefore not hung" is stale: the label is on both carriers (PM correction 5586852304) and this review derives yes. Expectation: edit that section to yes with the basis reason so the body and the carriers agree.

F8 — advisory. The memory-driver arm deviation is reasonable given the ledger and single-writer constraints; whether the literal two-driver arm is wanted is the maintainer's (dev report open question 1).

Maintainer-only merge: yes — Clause-② yes on a security gate (who receives the unscoped admin_full_access grant), a pin recorded under a maintainer ruling rewritten under a triage-seat ruling (F3), and an accept-set direction on that gate the maintainer has not ruled on (F4). No governed path and no !/breaking marker; those are not the reason.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/xl tests tooling

Projects

None yet

3 participants