Skip to content

fix(plugin-auth): phone_number is better-auth's, and D7 derives the auth manager's whole plugin set - #7980

Merged
huangyiirene merged 4 commits into
mainfrom
claude/issue-7820-d7-plugin-surface
Aug 12, 2026
Merged

fix(plugin-auth): phone_number is better-auth's, and D7 derives the auth manager's whole plugin set#7980
huangyiirene merged 4 commits into
mainfrom
claude/issue-7820-d7-plugin-surface

Conversation

@huangyiirene

Copy link
Copy Markdown
Collaborator

Fixes#7820

Executes the maintainer's ruling of 2026-08-12 (读法 1). Both halves, quoted verbatim and untranslated:

裁定:读法 1 —— phone_number 归 better-auth;同时把 D7 的推导面拉宽。

  • sys_user.phone_numberMANAGED_EXTENSION_FIELDS 删除:auth-schema-config.ts 已发货的显式映射(phoneNumber → phone_number)是所有权证据;该字段不在 MANAGED_EXTENSION_EDITABLE_FIELDS,无通用写面损失。
  • D7 的插件推导拉宽到与 auth manager 实际装配一致,采用 better-auth-schema-parity 已写下的同一条理由(「feature-flag 关闭的插件也要算 —— 列必须先存在,flag 才能打开」),并把这条理由写在 D7 处;拉宽后 D7 对本次删除应转绿,对未来任何新 overlap 应真实转红。

What changed

1. sys_user.phone_number leaves MANAGED_EXTENSION_FIELDS.AUTH_PHONE_NUMBER_USER_FIELDS in auth-schema-config.ts has shipped the explicit phoneNumber to phone_number mapping since #2766, so better-auth writes that exact column whenever the phoneNumber plugin is on. No write surface moves: the field was never in MANAGED_EXTENSION_EDITABLE_FIELDS (verified — the only consumer of these maps anywhere in packages/ is auth-plugin.ts, and it reads the editable map only), and the admin bulk-import path that does upsert it runs under a system context off SYS_USER_IMPORT_UPDATE_FIELDS, an unrelated list.

2. D7's derivation loads the auth manager's whole plugin set. It loaded organization alone while buildPluginList() assembles fourteen factories, so for a fully mapped object like sys_user the guard compared against a fraction of better-auth's real surface and answered green about the rest. Measured, at the pinned 1.7.0-rc.2, the widening adds to sys_user: two_factor_enabled, role, banned, ban_reason, ban_expires, phone_number, phone_number_verified; and to sys_session: impersonated_by.

auth-manager.ts is not touched — the widening reads its plugin assembly, it does not change it.

The half that "green on the removal" cannot prove

The ruling asks for two directions, so the red one is pinned in-repo rather than asserted in prose:

  • findCollisions() is now a pure function, exercised against a synthetic registry declaring two_factor_enabled — a column that reaches the surface only through a plugin the old derivation did not load. Under the old plugin set that registry produced no collision at all; it now reports one. Camel-case spelling and a re-declared phone_number are pinned the same way, plus a non-vacuity case (manager_id stays silent).
  • A drift tripwire scans auth-manager.ts for the plugin factories it imports and reconciles them against this guard's accounting map in both directions, so a plugin added there cannot quietly stay outside the derivation.
  • The plugin set is derived from that accounting map, not written beside it. An entry carrying a construct thunk is in the derived surface, because the thunk is what builds it — there is no way to declare a plugin covered and not load it.
  • A literal list pins the widened columns. Written as literals, never read back out of the plugin list, so narrowing the list cannot shrink the expectation along with the surface it polices.

One finding worth flagging: do NOT copy the parity gate's call

The obvious widening — reuse better-auth-schema-parity.test.ts's getAuthTables() options — is wrong here, and measurably so. Those options carry our schema: overrides, and AUTH_INVITATION_SCHEMA declares ADR-0105 D8's additionalFields (businessUnitId, positions). Derived that way, the "better-auth surface" contains ObjectStack's own extension fields and D7 reports us colliding with ourselves — measured: two false collisions, sys_invitation.business_unit_id and sys_invitation.positions, demanding a rename plus migration of our own columns.

So D7 passes the plugin set without the schema overrides, and that omission is now load-bearing rather than accidental: a dedicated assertion fails if our additionalFields ever appear in the derived surface, with the reason in its message. The snake_case half the mappings would have provided was already covered — toSnakeCase records both spellings.

Composition with #7822

This is a removal plus a derivation widening, so it composes with #7822's refusal rather than fighting it: nothing was added to MANAGED_EXTENSION_FIELDS, which doubles as the ADR-0092 D2 write whitelist. That failure message is preserved, retargeted only where it named a function this PR renamed.

One deviation to flag.#7770's exemptions for sys_two_factor / sys_device_code / sys_jwks stay registered and their "only holds while no extension field is declared" note is untouched — but their reason text said the plugins were not loaded by this call, which the widening makes false. The reasons are restated to say the models are now derived while MODEL_TO_OBJECT still deliberately does not map them (mapping them is a coverage expansion the ruling kept unmoved). The sso/scim reasons had a related inaccuracy the widening exposed: getAuthTables() does emit their models, under better-auth's own names — what those plugins accept no schema option for is the mapping, so nothing can key them onto sys_sso_provider / sys_scim_provider. Corrected in place.

Verification

Predict-then-mutate, with every direction written down before the mutation. All matched; one produced more reds than predicted (noted below).

MutationPredictedObserved
Restore phone_number to the registryRED, exactly sys_user.phone_numbermatched
Delete the phoneNumber entryRED: widened-columns pin + tripwirematched, plus the synthetic pin (3 reds, not 2)
Mark phoneNumber as a documented skipRED: widened-columns pin; tripwire greenmatched (2 reds)
Feed the schema overrides inRED: additionalFields pinmatched, plus the real collision assertion

The extra red in row 2 is correct and not a surprise worth suppressing: narrowing the set removes phone_number from the derived surface, so the synthetic re-declaration pin stops firing too. Reported as observed rather than as predicted.

  • pnpm --filter @objectstack/plugin-auth test — 43 files, 1046 tests, all passing (D7 file: 19 tests, up from 12).
  • pnpm --filter @objectstack/plugin-auth typecheck — clean. That script excludes *.test.ts, so it does not read this diff's main file; the TEST_DEBT ratchet's own measurement was reproduced for this package instead: 107 raw tsc errors on origin/main, 107 on this branch — zero added. The single diagnostic in the changed test file is the pre-existing import.meta TS1470, which merely moved line.
  • pnpm exec eslint --no-inline-config on both changed files — clean (no inline disables were used; they are inert on the runner).
  • node scripts/check-nul-bytes.mjs — OK, 7279 files. Plus a direct control-byte scan of the changed files. pnpm check:adr-anchors — clean.
  • Consumer sweep, downstream direction (--filter '...@objectstack/plugin-auth' — the prefix form, i.e. the 16 packages that DEPEND ON it: cli, client, client-react, cloud-connection, dogfood, driver-turso, example-crm, example-showcase, example-todo, hono, http-conformance, plugin-auth, plugin-dev, runtime, service-sms, verify): exit 0, all green. The full workspace build was run first — the sweep's first attempt produced false reds from an unbuilt dist (Cannot find package '@objectstack/plugin-auth/rate-limit-storage'), a missing-build artefact and not a signal about this diff.

Generated by Claude Code

@vercel

vercelBot commented Aug 12, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
ProjectDeploymentActionsUpdated (UTC)
objectstackIgnoredIgnoredAug 12, 2026 9:20am

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/plugin-auth.

8 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/deployment/cli.mdx(via @objectstack/plugin-auth)
  • content/docs/deployment/production-readiness.mdx(via @objectstack/plugin-auth)
  • content/docs/kernel/contracts/cache-service.mdx(via @objectstack/plugin-auth)
  • content/docs/kernel/services-checklist.mdx(via @objectstack/plugin-auth)
  • content/docs/permissions/authentication.mdx(via @objectstack/plugin-auth)
  • content/docs/permissions/sso.mdx(via @objectstack/plugin-auth)
  • content/docs/plugins/index.mdx(via @objectstack/plugin-auth)
  • content/docs/plugins/packages.mdx(via @objectstack/plugin-auth)

2 release-owned page(s) also reference the affected code. These are read-only:

  • content/docs/releases/implementation-status.mdx(via @objectstack/plugin-auth)
  • content/docs/releases/v9.mdx(via @objectstack/plugin-auth)

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.

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

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

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

D7 collision guard derives a narrower plugin surface than the auth manager loads — sys_user.phone_number is a real overlap it cannot see

2 participants

@huangyiirene@claude