Repository navigation
refactor: review every file against the house rules, refresh deps and guides - #70
Merged
Merged
Conversation
The guides had drifted for a month: the front one was 210 commits behind. Each claim was checked against the code and every stale one rewritten. Root: the real pre-push and CI split, the sso compose profile, the a11y gate command, the bootstrap targets, mise.toml, the BREAKING footer rule and the repo skills. Api: missing middlewares, cron/, migrate.ts, the ./app export and the real-database check scripts. Modules: the DDD tree is now labelled as the target shape (no module has domain/ today), the store vs repository naming, the write-and-emit-in-one-transaction rule and the notification fanout subscriber. Front: the mutations file name, the ui primitives promoted lately, page width and h1 conventions, the impersonation freeze, the api/errors flow, the named getErrorsT exception for fallback copy, and the cookie section moved to English.
Moves all caret ranges to the latest release they already allow: react and react-dom 19.3, vite 8.3, zod 4.6, hono 4.13.10, tanstack router/query, sentry 10.75 (last 10.x), aws-sdk, stripe, resend, react-email, lucide, and the tooling (biome 2.5.14, knip, jscpd, turbo, lint-staged, commitlint). The biome.json schema URL follows the installed biome. @better-auth/utils is pinned to exact 0.4.2 instead of floating to 0.4.3: better-auth, passkey, scim and sso 1.6.30 all declare it as an exact peer, so the caret produced an unmet-peer warning and a second copy. The better-auth family itself stays on its exact 1.6.30 pin for the reason recorded in docs/HISTORY.md (scim 1.7 rewrote its config contract).
@sentry/bun and @sentry/react go from 10 to 11.1. v11 removes sendDefaultPii and replaces it with a per-category dataCollection option whose defaults are inverted: user info, cookies, headers, request and response bodies, query params, DB query data and stack-frame locals are all collected unless opted out. A straight bump would have failed type-check on the app and, once fixed naively, silently started shipping PII. Both init sites now declare dataCollection explicitly with every category that can carry PII turned off, and keep v10's IP/identity header denylist. The beforeSend scrubbers stay as the second net. docs/OBSERVABILITY.md describes the new switch instead of the removed one.
vitest and @vitest/coverage-v8 go from 4.1 to 5.0 in the seven workspaces that declare them. None of v5's breaking changes reach this repo: no standalone bench API, no worker-id arithmetic, vi.mock calls are already hoisted at module top level, and the suites already reset their own mocks so the new clearMocks default changes nothing. All suites pass on 5.0.2, and ddd-kit's coverage run still reports. better-auth 1.6.30 declares vitest as an optional peer capped at ^4, for test helpers the repo never imports. A peerDependencyRules entry accepts 5 for that edge only; better-auth 1.7 already allows it, so the rule goes with the 1.6.30 pin.
dotenv goes from 17 to 18 in packages/drizzle. v18 drops .env.vault and `node -r dotenv/config` preloading, and moves its injection log to stderr; the package uses neither removed feature, only `import "dotenv/config"` and `config()` in drizzle.config.ts, both unchanged. Checked by loading a file through both paths and running drizzle-kit check against the config.
@simplewebauthn/server 14 is out, but it cannot be adopted here. The api declares it only so TypeScript can name the WebAuthn types that leak into the inferred `auth` type; those types come from the copy @better-auth/passkey depends on, which is ^13 in both the pinned 1.6.30 and the latest 1.7.6. Bumping the direct dependency to 14 splits the two copies and type-check fails with TS2883 on `auth` (AuthenticationResponseJSON and the two options JSON types can no longer be named portably). The ^13.3.3 range stays. The side-effect import now says why it exists and which major it has to follow, so the next refresh doesn't retry the bump blind.
Turbo 2.11 writes a managed AGENTS.md at the root whenever it detects an AI agent. The repo keeps its agent rules in the CLAUDE.md tree, so a second file would only drift from it; agentGuidance: false opts out.
WatchedList stored the caller's array as both its current and its initial items, so every add() also grew the initial snapshot and mutated the caller's array. An item added, removed and added again was then treated as initially present and never reported as new, which hides a real change from a repository persisting the diff. Both lists now start from their own copy. find() also returned None for a falsy match (0, "", false) because it tested the found value instead of its index.
The package tsconfig excluded __TESTS__, so the suite never ran under noUncheckedIndexedAccess and a scoped repository stub that did not match its port compiled only because nothing compiled it. Tests are now part of type-check, with the indexed accesses guarded and the stub typed against Note["_id"]. The Result.ok overload test now asserts at the type level (ts-expect-error) instead of describing the guarantee in comments, and the two httpStatusFromCode suites are merged into one file.
EventCollector.add normalises its input once and returns early instead of nesting the out-of-context branch, and a test now pins that events added outside a context are reported to the logger rather than collected. Em dashes in the log message and the unit of work comment are replaced.
The main pool and the dedicated rate-limit pool each re-implemented the DATABASE_URL presence check with the same error; they now call one requireDatabaseUrl() helper, and both lazy getters return early once the client exists instead of nesting the whole construction in an if. The config reuses DbClient instead of repeating its ReturnType. Comments in the sso and sweep schemas drop stale plan references and em dashes; no column, index or migration changes.
The revocation reasons were listed twice, in the api_token column type and in the api_token.revoked payload enum, so adding a reason to one silently left the other behind. The catalog now owns API_TOKEN_REVOKED_REASONS and the drizzle schema derives its column type from it (drizzle already depends on the events package for the notification enums). Admin payloads compose ActorRef and UserRef with extend(shape) instead of the deprecated zod merge, isSubscribableSelector returns early instead of nesting, UserEmailChangeRequested reuses the shared Email schema, and em dashes in comments are replaced.
The notification map suite was written in unaccented French while the rest
of the codebase is in English, and two describes carried plan labels
("C.7", "SOTA", "new constants") that mean nothing once the plan is done.
Names only; assertions are unchanged.
The owner role restated every application capability that the statement already declares, so a capability added to the statement could be forgotten on the owner. Both now spread one appStatement object. The underscore aliases around ORG_ROLES and OrgRole are dropped (same exports), the personal org comment points at the rules it refers to, and the rolesWith tests are named in English after what they assert; the last one was named for the opposite of its assertion.
…ation
The UI copy and code comments of the i18n, ui and emails packages used em
dashes as separators. They now read with commas, colons, parentheses or a
middle dot, in both locales so the two catalogs keep the same tone. Command
palette entries follow the existing "Theme: Light" pattern
("Settings: Organization"), and French keeps its lowercase-after-colon
convention. Keys are unchanged, so the parity gate is unaffected.
CopyableValue fired onCopied right after starting the clipboard write, so a denied permission or an insecure context still showed the "copied" toast while nothing reached the clipboard. The callback now runs when the write resolves; a rejection stays unhandled on purpose so the global error handler reports it instead of it being swallowed.
…exports The notification digest subject (plural count plus category) was computed twice, once by the component for its heading and preview and once by the renderer for the subject line. Both now call notificationDigestSubject, so the two can no longer drift apart. The package entry re-exported render and templates with export *, which hid the public surface. It now names each export explicitly, matching the i18n package entry; the exported set is unchanged.
Six readers (the error classifier, the global query-error handler, the message registry, the policy-refusal watcher and the toast helper) each re-implemented the same "is it an object, then cast to ApiError" guard before looking at status or code. apiErrorFields now owns that guard, so a new reader cannot get the non-object case wrong. throwApiError's Retry-After parsing moves into a flat helper instead of three nested branches, and messageFromCode/formatApiError read as early returns. Behaviour is unchanged; the existing classifier, message and api-error tests pass as before.
…emetry Six shared calls rejected without the HTTP status: the locale save threw the raw JSON body, the deletion preflight and the upload presign, confirm and delete threw a bare "HTTP <n>" Error, and the passkeys, sessions and subscription upgrade calls rebuilt BetterAuth errors from their message only. The error classifier treats a missing status as a network failure, so every expected 4xx on these paths was reported to telemetry as unexpected, a rate-limited locale save skipped the rate-limit toast, and the catalog could never resolve their error codes into localised copy. They now go through throwApiError and toAuthClientError like every other call site, keeping their previous fallback text when the body carries no message. The storage PUT stays a plain Error: it targets the bucket, not our API, and has no error envelope to read.
The active-org and current-membership queries each carried the same branch turning BetterAuth's NO_ACTIVE_ORGANIZATION error into a null answer. nullWithoutActiveOrganization now owns that translation, with its reason next to it, so the next org-scoped read cannot rethrow the transient state by forgetting the branch. Behaviour is unchanged.
…blank .env.example ships VITE_SENTRY_ENVIRONMENT and VITE_GIT_SHA as blank keys, which Vite reads as an empty string. Only three of the five optional keys mapped "" to absent, so with a DSN set Sentry received an empty environment instead of falling back to the build mode, and an empty release. The blank-as-absent rule is now one helper applied to every optional key.
billing-plans.ts and subscription.ts re-exported PlanCatalogItem and EntitlementsView from billing-types.ts. No module imports them from there, and a second path to the shared wire contract is a barrel in miniature: billing-types.ts stays the only place these types come from.
Four shared tests each walked the errors catalog by hand to fake i18next's t, three of them as near copies with small drifts (only one interpolated placeholders). catalogT and catalogLookup now live next to the i18n tests, so every shared test resolves copy the same way and asserts against the real catalog.
The house style bans em and en dashes in code, comments and strings. The shared api, i18n and legal files used them as clause separators; they now read with colons, commas or parentheses. Two small layout fixes ride along, blank lines between the hook call, the effect and the return in app-providers and between the two awaited calls in transfer-and-leave. No behaviour change.
The settings tabs and the command palette each re-implemented the same "requires an org membership, then the declared permissions" filter. The check now lives on useAuthorization as canReach, next to can and hasMembership, and both navigation sources declare their gate through the shared NavigationRequirement shape. Visibility is unchanged.
The user menu hardcoded the "⇧⌘Q" hint while the command palette owned the binding that actually listens for it, so the two could drift. The binding now lives next to useSignOut and both surfaces read it. The sign-out onSuccess callback also drops a needless async since it awaits nothing.
The consent settings card re-declared the four cookie categories and the optional subset that @packages/cookie-consent already exports, and both consent components cast the typed consent response to a hand-written shape. They now use the package constants and the inferred response type, so a new category or a renamed field breaks the build instead of drifting. The two consent mutations also share one refetch, toast and close step.
The category to catalog key map lived in the inbox item component while the preference matrix imported it from there. It now sits in notification-labels with the other label helpers, and categoryKeyFor returns the literal key union so the badge no longer needs an `as never` cast to reach t(). The broadcast patch and the SSE handler also read the unread-count key from its query factory instead of each redeclaring it.
Shared components reached their siblings through `../../shared/...`, which climbs out of shared/ only to come back in; they now use plain relative paths. canAccessPlatformAdmin builds on isPlatformAdmin instead of repeating its session cast, and the palette's theme option builder no longer carries a `use` prefix since it calls no hook.
The inventory said policies.config.tsx selects the locale module of a legal body. That is policies/bodies.ts; the config only holds versions and changelogs. The custom primitives list also gains the primitives added to @packages/ui in this pass.
…nt ip The verify-email hook recorded policy acceptance with the session ip, which BetterAuth derives from the raw X-Forwarded-For header, and the security events emitted from the BetterAuth hooks read that header directly. Any client could therefore write an arbitrary address into the compliance record and the audit trail. The request context now carries a lazy client ip resolved by the trusted-proxy resolver, and the BetterAuth hooks read it from there since they have no Hono context of their own.
…d result TransactionService.run committed whatever the callback wrote before it returned a failed Result, so a multi-write flow that failed halfway left partial state behind unless the caller remembered to throw. A failed Result now rolls the transaction back, writes and collected events included, and run resolves to that same Result, so the IUnitOfWork signature is unchanged. The service also receives the instrumentation port: run and startTransaction open a span and capture what they rethrow. A new check:uow-rollback script proves the rollback against Postgres and runs in CI next to check:wipe-rollback.
…rowing Consents, policies, rgpd, api-token and the token scanning route threw a "rollback" sentinel from inside the unit of work and kept the real error in a side variable, only to force the transaction to roll back. The unit of work now rolls back on a failed Result, so each callback returns the failure directly and the catch blocks handle genuine crashes only.
PATCH /api/v1/me updated the user name and then emitted user.profile.updated in a separate statement, so a failed enqueue left a renamed user with no event. updateUserName now accepts the transaction and the route runs both writes in one unit of work.
Replaying a delivery inserted a new webhook_delivery row with no event, so the audit trail never showed that a member resent a payload to a customer endpoint. The catalog gains webhook.delivery.replayed (internal, operational retention), emitted in the replay transaction with the new delivery id and the member the route passes as actorUserId.
…n account Signing in rewrote consent_record.user_id for the browser's guest records without any event, so the consent trail could not show when a guest consent became the account's. Reconcile now runs in a unit of work, the store returns the ids it attached, and user.cookie_consent.linked is emitted with them (compliance retention) only when a record actually changed owner, since the hook runs at every login.
ApiTokenCreatedPayload validated expiresAt with z.date(), unlike every other date in the catalog, so a payload read back from the outbox JSON (where the date is a string) failed validation. It now coerces like the rest.
.env.example leaves INTERNAL_SIGNING_KEY empty on purpose, so after pnpm bootstrap 18 internal-route and rgpd tests failed until a key was exported by hand. A bun test preload now sets a fixed test-only key when none is present; a key exported by the shell or the CI job still wins, and no working key ships in .env.example.
ApiTokenRecord exposed organizationId, lastUsedAt, expiresAt, revokedAt and revokedReason as T | null, leaving every consumer (service, scanning route, api-token middleware) to remember its own null guard. The store now maps them to Option on read and back to null on write, and the routes serialise tokens through a presenter that unwraps them, so the JSON the app receives is unchanged.
nextAttemptAt returned { date: Date | null, delayMs } and all three
workers only read date, each wrapping it back into an Option. It now
returns Option<Date> directly, and the webhook worker drops its private
copy of expectedDelayFromAttempts for the shared one.
findPasswordViolation and validatePassword answered PasswordViolation | null and the BetterAuth hook guarded it with optional chaining plus a separate null check. Both now return Option<PasswordViolation>, and the hook returns early on none before reading the violation once.
ScanningDeps.findUserById answered the user or undefined. The dependency now returns an Option, app.ts adapts the auth-queries lookup with Option.fromNullable, and the scanning route branches on isSome.
requireApiToken in shared/middleware imported IApiTokenRepository from the api-token module's application/ports, the shared-to-module coupling that shared/ports exists to prevent. The port moves to shared/ports/api-token.port.ts unchanged; the Drizzle repository stays in the module.
billing.middleware, quota-reservation and auth.ts imported Tier, Feature, QuotaKey and the entitlement predicates from modules/billing/config, so the shared kernel depended on a module. The catalog is config-as-code with four consumers, so it moves to shared/entitlements.ts unchanged and its two test files merge into one next to it.
Eight stores each declared their own storeFailure, repositoryFailure or
failure function building the same { code, message, metadata.cause }
error, with small drifts (quotas and notifications dropped the cause or
the operation). shared/db-failure.ts now exports dbOperationFailure(code)
and every store binds it once with its own code.
The admin and organization routes both validate { enforced: boolean }
before calling AdminActionService.setSsoEnforcement, each with its own
schema since modules cannot import each other. The admin DTO moves next
to the shared service and both routes use it.
Completes the previous commit, which only carried the file move: the admin and organization routes now import setSsoEnforcementBodySchema from shared/services, and the organization route drops its local copy.
The CSRF, rate-limit and CSP report paths, plus the BetterAuth abuse checks, each wrapped emitEvent in the same try/catch that logs a warning so a failing outbox never turns a refusal into a 500. event-emitter.ts now exports emitEventBestEffort and the four sites call it with their own warning.
STORAGE_PRESIGN_TTL_MIN_SECONDS and _MAX_SECONDS were optional in the env schema and every reader repeated the fallback (?? 60, ?? 3600) in three files. The schema now carries the defaults and the readers use the values as they are.
Six route suites replaced the whole auth.middleware module with a stub that only carried requireAuth and an empty AuthVariables. Any other export added to the module later would have vanished from those suites and failed at import. Each stub now spreads the real module and overrides requireAuth only, as the testing rules require.
The sweep-lock and sweep-purge suites replaced @packages/drizzle with a hand-kept copy of its exports (drizzle-mock.ts): a schema or helper added to the package was missing from those suites until someone copied it over, and any module reading a column at load time broke them. Both suites now spread the real module and override db only, so the copy is gone. sweep-notifications already rendered through the real builder.
Six service suites each declared their own no-op outbox, and three of them their own recording outbox or pass-through unit of work, all typed against the same ports. They now import one copy from shared/__TESTS__/outbox-fakes.ts, so a method added to IOutboxRepository or IUnitOfWork is fixed once. passthroughUow takes the transaction handle, so the suites that assert the tx a store received keep checking that exact handle. The module testing rules name the helper.
AdminActionService.setSsoEnforcement built its organization update inline, the only BetterAuth-owned table written outside auth-queries.ts. The update now lives there as setOrgSsoEnforced(organizationId, enforced, tx?) and the service passes its unit of work transaction, so the flag and SSO_ENFORCEMENT_CHANGED still commit together. The service test asserts that exact transaction handle, and its module stand-ins now spread the real event-emitter and auth-queries.
The shared kernel inventory missed pg-listener, event-actor, env-bounds, outbox-subscriber and the SSO enforcement body schema. The module rules claimed Result.ok<string>() no longer compiles: it does, as Result<void, string>, and the real guarantee (pinned by a ts-expect-error in ddd-kit) is that a no-argument success cannot be assigned to Result<string, E>. FEATURES.md listed the billing classes under old names and left out BillingPortalService and its Stripe gateway.
The CLAUDE.md tree, the repo skills and the CI workflow comments are what agents load and imitate, and they were the last place the code and copy passes left em dashes. Each one becomes a colon, comma or semicolon with the sentence otherwise unchanged.
TransactionService captured any error thrown inside run(), including the 4xx HTTPException routes throw to abort a transaction, so each expected 404 became a Sentry event and each real 5xx was reported twice (here and by the caller or the central handler). The unit of work now rethrows without capturing and leaves the classification to the caller; its span stays. check:uow-rollback asserts the error reaches the caller unchanged, the write is rolled back and nothing is captured.
BetterAuth fills session.ipAddress from the raw X-Forwarded-For header, which any client can forge, and USER_SIGNED_IN copied it into the audit trail. The session create hook now overwrites it with the address the trusted-proxy resolver put in the request context, so the stored session and the sign-in event both carry the real client ip.
The code and the agent-facing guides no longer use em or en dashes; the
rest of the documentation still had about 1 550. Each one becomes a
colon, comma, semicolon or parentheses, ranges are worded out ("1 to 5")
and empty table cells read "n/a". The French sections of EVENTS.md keep
the space before the colon. CHANGELOG.md stays as semantic-release
writes it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A maintenance pass over the whole repo: the CLAUDE.md guides brought back to the as-built state, every dependency refreshed, and all 871
.ts/.tsxfiles reviewed against the house rules in 12 batches, followed by a second pass on what crossed batch boundaries. Each batch left a file-by-file report with the rule behind every change and the reason behind every change it declined.Guides. The seven CLAUDE.md files had drifted for a month (the front one was 210 commits behind). Every claim was checked against the code. The modules guide now says plainly that the DDD tree is the target shape and that no shipped module has a
domain/yet.Dependencies. 50 outdated entries down to 8, all pinned on purpose: better-auth 1.6.30 and its plugins (1.7 changes the SCIM config format, which is a task of its own),
@better-auth/utils0.4.2 (better-auth requires it exactly),@simplewebauthn/server13 (@better-auth/passkeystill needs^13, 14 breaks the type-check), andconventional-changelog-conventionalcommits9 (10 brings back the release bug fixed ine4d12b69). Sentry 11 collects user info, cookies and bodies by default, sodataCollectionnow opts out of every category.Bugs fixed, almost all with a test or a real-database check that fails before the fix:
X-Forwarded-Forany client can forge;TransactionService.runcommitted when the callback returned a failedResult; it now rolls back, and no longer reports every aborted transaction (4xx included) to Sentry;PATCH /api/v1/me, webhook replay, cookie consent linking,ORG_DELETEDafter a SCIM deprovision;WatchedListmutated the caller's array; several front calls dropped the HTTP status so expected 4xx reached Sentry; SSO redirects were reported as errors; clipboard toasts claimed success on failure; a wrong current password surfaced under the new-password field.Structure. Remaining
T | nullin ports moved toOption, shared no longer imports from modules, repeated helpers were promoted (store failures, test fakes,throwApiErrorfallbacks, catalog-backedtin tests), and new@packages/uiprimitives keepclassNameto layout. The code, copy, guides and docs no longer use em dashes.Checks. The pre-push chain is green (Biome, build, type-check, 906 api and 356 app tests, knip, jscpd), plus the api
check:*scripts on a local Postgres, including the newcheck:uow-rollback. Known gap: the session IP fix lives in a BetterAuth hook with no test seam, so it has no automated test.Merge commit, not squash: the conventional commits drive the next release.