fix(plugin-auth): single written definition for the auth basePath default - #17898
os-project-manager merged 3 commits into
Conversation
…ault
DEFAULT_AUTH_BASE_PATH ('/api/v1/auth') is now the one place this literal
is written. Before this, it existed independently at four sites: the
AuthPlugin constructor default, two later re-derivations inside AuthPlugin
(registerAuthRoutes, the OIDC discovery well-known alias), and
AuthManager.configuredBasePath()'s own fallback. Every site evaluates
byte-identically to before -- this collapses where the value is WRITTEN,
not what any site evaluates to, and does not touch the
configuredBasePath -> rootedBasePath -> getBasePath normalisation chain
(#16399) or the published OAuth iss / RFC 8707 aud identifiers.
Adds a source-scan regression test (auth-manager-base-path.test.ts) that
fails if a future edit reintroduces a second hardcoded literal at any of
the four sites -- the divergence class this card is about is otherwise
unfalsifiable by construction on the live path (AuthPlugin always
supplies basePath, so AuthManager's own fallback never runs there).
Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj
Co-authored-by: Claude <noreply@anthropic.com>
…th-base-path-single-definition
…sePath default deriveBasePath() read AuthPlugin's basePath fallback as a re-typed string literal at a fixed regex position. #16384 replaced that literal with an import of the package's single DEFAULT_AUTH_BASE_PATH definition, so the regex could no longer find a value and the gate refused (NOT MEASURED) instead of running its census. The fallback is still read from source rather than hardcoded here -- the same justification the function already carried -- just one hop further: when the fallback is a bare identifier instead of a literal, resolve it from the module that actually declares it (auth-manager.ts), which joins MOUNT_SOURCE and LEDGER_SOURCE as a third dispatch-gates-derivable input. In-place per this repo's bounded-fix exemption: same file MOUNT_SOURCE already names (so squarely dispatch-gates-derived for this same diff), mechanical (a second alternative in one regex plus a same-shape lookup), held by no other claim, and no new verification surface -- the two new self-test assertions extend an EXISTING battery's existing coverage of the existing "the base path is derived" invariant rather than opening a new one. Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj Co-authored-by: Claude <noreply@anthropic.com>
📓 Docs Drift CheckThis PR changes 1 package(s): 11 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
⛔ 5 release-owned page(s) also name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 14 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 13887497fc09e961bcde3ad556fe14cd8230f0de && git checkout 13887497fc09e961bcde3ad556fe14cd8230f0de
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin ef47459474dcf53bf3899c6712c22e7a2ebc7a9d 04a25ccc92ac78b16ee79f4d99700db914a57116 && git checkout -B drift-repro ef47459474dcf53bf3899c6712c22e7a2ebc7a9d && git merge --no-ff 04a25ccc92ac78b16ee79f4d99700db914a57116
node scripts/docs-audit/affected-docs.mjs --json ef47459474dcf53bf3899c6712c22e7a2ebc7a9d
|
|
Docs Drift Check read — disposition: no documentation change on this PR. ⛔ Do not edit any of the 16 listed pages here. The advisory is correct that 11 hand-written pages and 5 release-owned pages name something this diff touched. It does not follow that any of them is now wrong, and on this diff none can be:
⇒ Nothing a page could document has changed, so there is nothing to re-verify and no edit to make. ⛔ The 5
Generated by Claude Code |
Contract reviewHead reviewed: In-seat Clause-② review, default judgement tier. Per ① Derived judgments — every accept-set and public-surface change, named from the diff
⭐ Worth recording on (5): the minimal patch would have been to widen the regex alone. That would have left the gate underivable from the file that now holds the value — ② Semver grading — consistent
③ Boundary flags and
|
|
Clause-② provenance — both carriers cleared.
⛔ Not yet enqueued. Landing pre-check ③ is not satisfied:
Generated by Claude Code |
|
Landing pre-checks all satisfied — ready-flipped and auto-merge armed. (Supersedes the ⛔ hold in
Ready-flip confirmed by reading back ℹ️ The ⛔ Enqueue is judged by the
Generated by Claude Code |
Fixes #16384
What changed
'/api/v1/auth', the shipped default forAuthPlugin'sbasePathoption, was written independently at four live sites inpackages/plugins/plugin-auth/src/(re-derived onorigin/main, not inherited from the card or from triage — both are stale, see below):auth-plugin.ts:326—basePath: '/api/v1/auth',AuthPluginconstructor defaultauth-plugin.ts:2026—this.options.basePath || '/api/v1/auth'registerAuthRoutes's fallbackauth-plugin.ts:3099—(this.options.basePath ?? '/api/v1/auth').replace(...)registerOidcDiscoveryRoutes's well-known-alias fallbackauth-manager.ts:5695(insideconfiguredBasePath()) —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/getMcpResourceUrlas 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 inauth-manager.ts(besidereadMcpServerEnabledEnv), imported intoauth-plugin.ts. This only moves where the literal is written — every site evaluates byte-identically to before. In particular this does not touchAuthManager'sconfiguredBasePath()→rootedBasePath()→getBasePath()normalisation chain, and does not move the published OAuthiss(getAuthIssuer()) or the RFC 8707aud(getMcpResourceUrl()) — that value is #16399's decision territory, not this card's, per the hard fence in the dispatch.Byte-identical proof:
git diffon 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 existingauth-manager-base-path.test.tssuite (36 cases covering every normalised spelling on a realbetterAuth()instance) still passes unchanged.Where the shared definition lives — Clause-② outcome
DEFAULT_AUTH_BASE_PATHis declared withexport constinauth-manager.ts.index.tsre-exportsauth-manager.tswholesale (export * from './auth-manager.js'), so the constant is reachable from the package entry — confirmed by building the package and grepping the emitteddist/index.d.ts, where it appears in the finalexport { ... }list alongsideAuthManagerandAuthPlugin.I considered keeping it unreachable (a
export defaultfromauth-manager.ts, whichexport *never re-exports), but rejected it: this codebase has essentially zero precedent for default exports inpackages/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 inauth-plugin.tsand havingauth-manager.tsimport it) was the other alternative; rejected because it introduces a new import cycle where none exists today (auth-plugin.tsalready imports fromauth-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-②: yesdeclaration anticipates.needs:contract-reviewis attached to this PR and to #16384 in this same push.New test — and why it can't be a value-comparison
AuthPluginalways suppliesbasePathtoAuthManager(viaauthConfig = { ...this.options, ... }), soAuthManager'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.tsnow 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 — theDEFAULT_AUTH_BASE_PATHdeclaration itself — and thatauth-plugin.tsreferences 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.tsgains a runtime test that was simply missing before: the existing suite tested a custombasePathreaching the mounted route, but never the default one.should mount the default base path when none is configuredcloses that gap end-to-end (constructor default →registerAuthRoutes→ the mockedrawApp.allcall), asserted against the literal'/api/v1/auth'(not the constant) so a typo inDEFAULT_AUTH_BASE_PATHitself would still fail it.Collateral fix, in-place per this repo's bounded-fix rule
scripts/check-auth-mount-ledger.mjs'sderiveBasePath()read the plugin'sbasePathfallback by regex, anchored on a trailing string literal at the exact site this PR changes (auth-plugin.ts'sregisterAuthRoutes). 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-ledgerexit 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 makedispatch-gatesderive this family for a diff touching it;AUTH_MANAGER_SOURCEnow 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'sbasePathdefault — and are outside this PR's file surface:packages/client/src/index.ts— one row in the client'srouteMap(16 rows, one per service).packages/metadata-protocol/src/protocol.ts— one row inSERVICE_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 (confirmedDEFAULT_AUTH_BASE_PATHin emitteddist/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 asorigin/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— 0dispatch-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-measureleg —PREREQUISITE NOT MET(exit 3, not a finding): both need a full-monorepopnpm 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, reportsOK).check:pm-dispatch-gates— genuinelyNOT 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