You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Commit ee6fbd7
Browse filesBrowse the repository at this point in the historyBrowse files
fix(plugin-auth): single written definition for the auth basePath default (#17898)
Fixes#16384
## What changed
`'/api/v1/auth'`, the shipped default for `AuthPlugin`'s `basePath`
option, was written independently at **four** live sites in
`packages/plugins/plugin-auth/src/` (re-derived on `origin/main`, not
inherited from the card or from triage — both are stale, see below):
| # | Site (this PR's before-state) | Role |
|---|---|---|
| 1 | `auth-plugin.ts:326` — `basePath: '/api/v1/auth',` | `AuthPlugin`
constructor default |
| 2 | `auth-plugin.ts:2026` — `this.options.basePath \|\|
'/api/v1/auth'` | `registerAuthRoutes`'s fallback |
| 3 | `auth-plugin.ts:3099` — `(this.options.basePath ??
'/api/v1/auth').replace(...)` | `registerOidcDiscoveryRoutes`'s
well-known-alias fallback |
| 4 | `auth-manager.ts:5695` (inside `configuredBasePath()`) —
`this.config.basePath \|\| '/api/v1/auth'` | `AuthManager`'s own
fallback |
(Plus two `@default '/api/v1/auth'` TSDoc annotations —
`auth-plugin.ts:168`, `auth-manager.ts:573` — left untouched; they're
documentation, not a second definition, and out of this card's stated
scope.)
The card said "written twice"; triage corrected that to six and named
`getAuthIssuer`/`getMcpResourceUrl` as two of them — both were already
collapsed by #16399 before this card was claimed, so the live count is
four, not six, not two.
All four now read `DEFAULT_AUTH_BASE_PATH`, declared once in
`auth-manager.ts` (beside `readMcpServerEnabledEnv`), imported into
`auth-plugin.ts`. **This only moves where the literal is *written* —
every site evaluates byte-identically to before.** In particular this
does **not** touch `AuthManager`'s `configuredBasePath()` →
`rootedBasePath()` → `getBasePath()` normalisation chain, and does not
move the published OAuth `iss` (`getAuthIssuer()`) or the RFC 8707 `aud`
(`getMcpResourceUrl()`) — that value is #16399's decision territory, not
this card's, per the hard fence in the dispatch.
Byte-identical proof: `git diff` on the four sites is a pure token
substitution (`'/api/v1/auth'` → `DEFAULT_AUTH_BASE_PATH`, same
operator, same position); `DEFAULT_AUTH_BASE_PATH === '/api/v1/auth'`
(asserted in the new test, as a hardcoded literal independent of the
constant); the full existing `auth-manager-base-path.test.ts` suite (36
cases covering every normalised spelling on a real `betterAuth()`
instance) still passes unchanged.
## Where the shared definition lives — Clause-② outcome
`DEFAULT_AUTH_BASE_PATH` is declared with `export const` in
`auth-manager.ts`. `index.ts` re-exports `auth-manager.ts` wholesale
(`export * from './auth-manager.js'`), so **the constant is reachable
from the package entry** — confirmed by building the package and
grepping the emitted `dist/index.d.ts`, where it appears in the final
`export { ... }` list alongside `AuthManager` and `AuthPlugin`.
I considered keeping it unreachable (a `export default` from
`auth-manager.ts`, which `export *` never re-exports), but rejected it:
this codebase has essentially zero precedent for default exports in
`packages/plugins/*` (one file, for an unrelated reason), and using one
here to dodge the surface diff would read as gaming the gate rather than
a real design choice. A reverse import (declaring the constant in
`auth-plugin.ts` and having `auth-manager.ts` import it) was the other
alternative; rejected because it introduces a new import cycle where
none exists today (`auth-plugin.ts` already imports from
`auth-manager.ts`; the reverse edge did not exist).
So: **this is a published-surface addition** (one new named export, a
plain string constant, no behavior attached), which is exactly the kind
of widening this seat's `Clause-②: yes` declaration anticipates.
`needs:contract-review` is attached to this PR and to #16384 in this
same push.
## New test — and why it can't be a value-comparison
`AuthPlugin` always supplies `basePath` to `AuthManager` (via
`authConfig = { ...this.options, ... }`), so `AuthManager`'s own
fallback was already dead on the live path before this change — a fact
this card's dispatch notes was proven by an INERT mutation ablation
during #16025's round. That means a **value-equality** test (assert site
A's default equals site B's default) would still pass after a regression
reintroduced a second hardcoded literal with the same current value — it
would only fail once someone edited the value in one place and not the
other, which is exactly the failure mode this card exists to make
impossible.
So the regression test is structural, not behavioural:
`auth-manager-base-path.test.ts` now reads both files' own source text
and asserts the literal `'/api/v1/auth'` appears in **code** position
(comments excluded) exactly **once** across the two files — the
`DEFAULT_AUTH_BASE_PATH` declaration itself — and that `auth-plugin.ts`
references the shared identifier at all four collapsed call sites. A
future edit that reintroduces a hardcoded default at any of the four
sites fails this test immediately, before it can drift.
Alongside that, `auth-plugin.test.ts` gains a runtime test that was
simply missing before: the existing suite tested a *custom* `basePath`
reaching the mounted route, but never the *default* one. `should mount
the default base path when none is configured` closes that gap
end-to-end (constructor default → `registerAuthRoutes` → the mocked
`rawApp.all` call), asserted against the literal `'/api/v1/auth'` (not
the constant) so a typo in `DEFAULT_AUTH_BASE_PATH` itself would still
fail it.
## Collateral fix, in-place per this repo's bounded-fix rule
`scripts/check-auth-mount-ledger.mjs`'s `deriveBasePath()` read the
plugin's `basePath` fallback by regex, anchored on a trailing string
literal at the exact site this PR changes (`auth-plugin.ts`'s
`registerAuthRoutes`). After the literal became an imported identifier,
the gate refused (`NOT MEASURED — no basePath default could be derived`)
instead of running its 19-mount census — confirmed before this fix
(`pnpm check:auth-mount-ledger` exit 2) and after (exit 0, `OK — 19
ObjectStack auth mount(s), all accounted for`).
This qualifies for the repo's bounded in-place-fix exemption: same
defect class (this PR's own consolidation broke it), mechanical (one
more regex alternative plus a same-shape lookup one file over), the file
is held by no other claim, and no new verification surface is added —
the two new self-test assertions extend an *existing* battery's
*existing* coverage of the *existing* "the base path is derived"
invariant, they don't open a new one. `MOUNT_SOURCE` (`auth-plugin.ts`)
was already one of the two files that make `dispatch-gates` derive this
family for a diff touching it; `AUTH_MANAGER_SOURCE` now joins it as a
third such input, so the family keeps deriving correctly for either file
going forward.
## Explicitly out of scope (per the dispatch's own clearance)
Two out-of-package hits share the same string but are a different kind
of thing — a per-service route-convention table row, not a declaration
of `AuthPlugin`'s `basePath` default — and are outside this PR's file
surface:
- `packages/client/src/index.ts` — one row in the client's `routeMap`
(16 rows, one per service).
- `packages/metadata-protocol/src/protocol.ts` — one row in
`SERVICE_CONFIG`'s route table.
Neither is touched.
## Local verification
- `pnpm --filter '@objectstack/plugin-auth^...' build` — 0 (dependency
closure)
- `pnpm --filter @objectstack/plugin-auth build` — 0 (confirmed
`DEFAULT_AUTH_BASE_PATH` in emitted `dist/index.d.ts`)
- `pnpm --filter @objectstack/plugin-auth typecheck` — 0, re-run on the
final commit (test-layer debt ledger unchanged: 94 errors / 23
signatures, same as `origin/main`)
- `pnpm --filter @objectstack/plugin-auth test` — 0 (108 files / 2287
tests)
- `pnpm check:nul-bytes`, `pnpm check:auth-mount-ledger` (post-fix),
`pnpm check:ratchet-remedy-authority`, `pnpm check:pnpm-filter-targets`
— 0
- Full `dispatch-gates`-derived set for this diff (82 commands across
both commits) — all 0 except:
- `check:dual-build-cjs-loads`, `check:type-check-debt`'s `--re-measure`
leg — `PREREQUISITE NOT MET` (exit 3, not a finding): both need a
full-monorepo `pnpm build`, which is CI's to run per this repo's own
local-scope convention (`check:type-check-coverage`'s own base
measurement, which doesn't need the full closure, reports `OK`).
- `check:pm-dispatch-gates` — genuinely `NOT MEASURED`: its self-test
ran 1887+ clean assertions with zero failures observed over ~11 minutes
before I stopped it as disproportionate to a p3, proportionate-scope
card testing the PM tooling's own self-consistency rather than this
diff's behaviour. CI runs it required and will give the authoritative
verdict.
---
_Generated by [Claude
Code](https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj)_
---------
Co-authored-by: Claude <noreply@anthropic.com>
0 commit comments