feat: back ↔ front surface parity (phase H.1) + the defects the audit surfaced - #66
Merged
Conversation
Records, for every live HTTP route, either the front file that calls it or a documented UiLessReason. surface-parity.test.ts fails the build when a route is undeclared, stale, wrongly marked UI-less, or a declared consumer stops calling its route -- plus an assertion that closes the silent hole left by listBackRoutes dropping every ALL-method entry (a future real ALL route would otherwise vanish with no failing test).
PUT /admin/users/:id/role was live, guarded and audited but the front only rendered the role as a static badge. Adds setRoleMutationOptions, a SetRoleForm dialog on the admin user detail page, matching catalog keys in en/fr, and flips route-map.ts to point at the new consumer.
Replacing an avatar left the previous storage object orphaned forever.
IStorageService.keyFromPublicUrl derives the key from a public URL
server-side (front stays ignorant of the key format); DELETE /uploads
now accepts {key} or {url} and runs the derived key through the same
owner-prefix guard, returning {deleted:false} for a URL this storage
did not produce (e.g. a social-login avatar). upload-avatar.tsx
deletes the previous image after the new one is live, reporting a
failed cleanup to telemetry without breaking the avatar change.
Task 7b (H.1 surface parity scope extension): convert the remaining
21 toast.error(err.message) sites (webhooks, api-tokens, sso, admin-orgs,
security hooks, account hooks) to toastError(err, fallback) so refusals
render localized catalog copy instead of raw server English. features/auth/hooks
stays untouched — its err.message is already resolved via resolveAuthError.
10 new fallback keys added to packages/i18n/src/catalogs/{en,fr}/errors.ts for
hooks that call authClient directly and have no throwApiError fallback to borrow.
toastError(err, fallback) resolves via formatApiError, which needs
err.code or a numeric err.status to hit byCode/byStatus. The security/
and account/ hooks converted in task 7b threw a plain new Error(message)
with neither, so the catalog lookup always fell through to the generic
fallback and the specific rejection (e.g. INVALID_PASSWORD) was silently
discarded.
Add toAuthClientError(error, fallbackMessage) to shared/api/errors/api-error.ts,
carrying the auth client's {code, status} onto the thrown Error the same way
throwApiError already does for HTTP responses. Applied to the 10 mutationFns
that call authClient directly. use-add-passkey.ts's Cancelled flow-control
throw and its err.message guard are untouched.
Mount requireCurrentPolicies after requireAuth on the mutating routes
of profile, webhooks, api-token, organization, notifications and
billing, plus the public API's PATCH /api/v1/me. Retype the middleware
against a minimal { user?, session? } contract so it also mounts on
/api/v1's ApiTokenVariables context, and exempt impersonated sessions
so an operator inspecting an account is never 409-walled out.
Left ungated: /me/policies/* (would deadlock the only unblocking
route), the RGPD module (a data-subject right can't be conditioned on
accepting terms), sign-out/session reads, and every read-only route
(GET /api/v1/me and /api/v1/organizations included).
The three documented breakdowns were written from the pre-implementation audit and never refreshed: tasks 4 and 6 gave a consumer to PUT /admin/users/:id/role and DELETE /uploads, moving both out of the UI-less set. Verified against route-map.ts and listFrontConsumers(): 52 consumers, 19 UI-less, 1 dormant-by-design (POST /uploads/download). Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
[email protected]'s twoFactor plugin declares failedVerificationCount and lockedUntil on its twoFactor model and writes both during verification (rate-limit counter and lockout deadline). The Drizzle table declared neither, so the adapter rejected every verify with "The field failedVerificationCount does not exist in the twoFactor Drizzle schema" and 2FA could not be enabled at all — which in turn made /admin/* unreachable, since platform admin requires 2FA. Found while running the H.1 manual QA, which needed the admin panel. Audited the other nine mounted plugins against their declared schemas: no further drift. Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
166d4e1 added failedVerificationCount and lockedUntil to the twoFactor schema, but only `pnpm db:push` was run — the production path is `db:generate && db:migrate`, so no migration declared the columns and a fresh production database would recreate the bug that commit closed. The generated migration adds exactly the two columns (failed_verification_count integer default 0, locked_until timestamp) with no incidental drift. Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
The event picker renders every subscribable event type — 35 rows across groups — inside a DialogContent that has no height bound and no scroll container, so the dialog grew past the viewport in both directions with no way to reach the submit button. Each row was also a non-shrinking flex of a font-mono type plus its description, which pushed the width instead of wrapping. The picker now carries its own bounded scroll, so the URL field and the submit button stay put while the list scrolls, and the rows wrap. Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
requireCurrentPolicies answered a bare HTTPException(409), which the
error middleware stamps HTTP_409 and the front resolves through the
generic status copy ("This action conflicts with the current state").
A user still sitting on /settings/webhooks when a new terms version is
published therefore hit a wall with no path to acceptance: the redirect
to /legal/accept is decided in _shell's beforeLoad, which does not run
again while they stay on the page they are already on.
Same treatment this phase already gave the impersonation refusal:
- ddd-kit gains the _ACCEPTANCE_REQUIRED suffix at 409. The resolver
sorts suffixes longest-first, so it wins over _REQUIRED (401) rather
than being shadowed by it — the shadowing trap this phase already hit
with _PAYMENT_REQUIRED.
- The middleware raises POLICY_ACCEPTANCE_REQUIRED via AppErrorException.
- Both catalogs (en/fr) carry its copy, so the parity test stays equal.
- watchPolicyRefusals, wired once where the router and the query client
meet, refetches the policy status on that code and invalidates the
router — the existing shouldRedirectToLegalAccept decision then issues
the redirect. No second redirect mechanism; same shape as onAuthChange.
Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
The third assertion filtered on `!consumers.some((c) => c.route === route)` and never compared `c.file` to the declared `entry.consumer`. It therefore only ever asserted "something, somewhere, calls this route" — a boolean — while the file path is the one thing the map adds over that boolean. Moving a call between files left the map pointing at a file with no call in it, green. The assertion now matches route *and* file, and reports the declared path next to the files that actually hold the call so a failure says where to move the entry. All 71 declared consumers pass unchanged; verified the check bites by pointing one entry at a non-existent file. Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
…lse owns it Twenty-four controls hand-assembled `title` + `aria-describedby` from the impersonation guard, and four of them reworded the "is this freeze actually ours?" test in four different shapes. Four others skipped it entirely: on billing, admin orgs, the deletion card and the two deletion forms, the tooltip announced an impersonation block while `isPending` was what actually disabled the button — the message lied to the user. Promote the test onto the guard as `describeProps(otherwiseDisabled?)`, which returns the description only when impersonation is the real cause, and replace all twenty-four call sites with it. The six components that each re-declared `disabledReason?: string; describedBy?: string;` now take the guard itself, so a leaf control can answer the same question the same way (a revoking token row, an unread-less notification, a frequency select whose channel is off no longer blame impersonation either). No new i18n key: the reason still comes from `useImpersonationGuard`. Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
…d security The front mirrored only the `denyImpersonated` middleware family. The second server layer — the BetterAuth blocklist in `apps/api/src/shared/middleware/impersonation-blocklist.ts` — had no mirror, so name, e-mail, avatar, password, 2FA, recovery codes, passkeys and session revocation all stayed clickable during an impersonation and failed on the request. The same refusal was proactive on API tokens and reactive here. Apply `useImpersonationGuard` to exactly the controls whose endpoint is on that list (`/update-user`, `/change-email`, `/change-password`, `/two-factor`, `/passkey`, `/revoke-session`, `/revoke-other-sessions`), using the promoted `describeProps`. Nothing the server still accepts gets disabled — the sessions and passkeys lists, the avatar preview and every read stay live. One guard per card so its rows share a single description node. Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
`role` is nullable with no SQL default, so a legacy or imported account renders "—" and, until now, no "Change role" button — the exact gap this phase set out to close, one notch narrower. The button was gated on `user.role && isPlatformRole(user.role)`, which is precisely the case an admin needs to repair. Render the dialog unconditionally and let `SetRoleForm` accept `currentRole: PlatformRole | null`. The select falls back to the platform's own default role while the "nothing changed yet" test keeps comparing against the real current value, so both roles stay assignable to such an account. The badge is untouched. Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
Two false documentary claims, one of which hid a real hole. The counts: docs/FEATURES.md and docs/HISTORY.md said "50 call sites". listFrontConsumers() returns 52. Corrected in both. The exclusion list: the docs described the policy gate as a global mount with "four exclusion classes" (acceptance, RGPD, sign-out, impersonation). In reality the whole uploads module, all of consents and the entire /admin/* surface were also ungated and undeclared. The gate is an allowlist, not a global mount minus exclusions, so the docs now enumerate both sides. Of the three undeclared surfaces: - uploads was a genuine omission, not a choice — uploading is product usage, not a data-subject right, and nothing on /legal/accept uploads, so gating it cannot deadlock the route that clears the gate. Now mounted on all four routes, with a test covering both directions. - consents cannot be gated: the routes carry no requireAuth at all (the cookie banner records for anonymous visitors), and withdrawing consent is the same family of right as RGPD. - /admin/* stays ungated deliberately: an operator whose own acceptance went stale would be 409-walled out of the console, including POST /admin/impersonation/stop, the one route they would need. Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
None of DialogContent, AlertDialogContent or SheetContent declared a height bound or a scroll container, so any content taller than the viewport spilled past both edges with no way to reach it — the dialog is centred with translate(-50%,-50%), so growth is symmetric and the submit button ends up off-screen. The webhook endpoint dialog was the first to hit it; the audit-log metadata sheet renders arbitrary JSON and was next. Bounding it at the call site would have meant repeating the same two utilities on sixteen modals and forgetting them on the seventeenth. The now-redundant overflow-y-auto on the delivery sheet goes away. Verified in the browser: a short dialog is unchanged (419px, no scroll), and 3000px of injected content stays fully inside a 1223px viewport and scrolls internally. Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
The Category cell's note text inherited whitespace-nowrap from the shadcn TableCell primitive, stretching the table to 772px inside a 663px card and truncating the "Enforce for members" header/switch. Wrap it with whitespace-normal (same pattern already used in cookies.route.tsx / sub-processors.route.tsx). Also caps the notification bell popover (w-96) at 100vw-2rem so it cannot overflow a viewport under ~400px, per audit finding #6/LU.
The snippet extracted t= only to rebuild the signed string, and never
checked it. Since the signature covers the timestamp, a receiver
following this example accepts a captured request forever — a replay
stays valid until the secret rotates. docs/EVENTS.md already states the
contract ("reject if timestamp drift > 5 min"); the example customers
actually copy did not implement it.
Number(ts) on a non-numeric t= yields NaN, and NaN > 300 is false, so the
guard checks Number.isFinite first rather than letting garbage through.
The delivery worker signs with Date.now() on every attempt, so a
five-minute window does not reject legitimate retries.
Reported by Axel.
Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
…nections A dead `pg` connection emits `error` *then* `end`, and both handlers called `scheduleReconnect()` — two reconnections per outage. The dying client was never unwired either: its `notification` handler kept relaying every NOTIFY, and its own `error`/`end` handlers kept scheduling further reconnections. One killed backend produced 3 live LISTEN connections, then 4, then 9, and a single inserted notification fanned out to 4 `GET /unread-count` calls from a single tab. Nothing ever brought the count back down short of an API restart. Both halves are fixed: at most one reconnection is ever in flight (a single pending timer, cleared in `stop()`), and a dying client is torn down exactly once — listeners removed, socket closed, `listenClient` cleared when it still pointed at it. The uniqueness is held by the timer, not by a `connecting` flag: an `error` raised while `connect()` is still pending would wedge such a flag forever. `reconnectBackoff` still resets only on a connection that actually reached `LISTEN`. `pg`'s `Client` is now taken as a structural shape with an injectable factory, so the contract is provable without a database: the regression test counts live clients after N simulated outages, not just scheduled reconnections. Against the previous code it reproduced the audit exactly — 1 → 3 → 9 clients and a single NOTIFY dispatched 7 times. Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
…erywhere The `pg_notify` rail only fired `AFTER INSERT`, so marking a notification read on one device never reached another open tab: polling is disabled while the stream is connected and `refetchOnWindowFocus` is off, leaving the badge stuck on a stale count until a full reload. Measured: database at zero unread, badge still showing "9+" more than two minutes later. A second trigger now emits the same signal on the `read_at` transition. It is a separate trigger rather than `AFTER INSERT OR UPDATE`, because a combined trigger cannot carry a `WHEN` clause referencing `OLD` — and without that clause every write touching `read_at` would signal, including no-ops. The channel is renamed `notification_changed`: it no longer carries only births, and a name that lies is how the next reader gets it wrong. No front change is required — the stream carries a signal, not data, and the client already invalidates the whole `["notifications"]` key on any frame, which covers `unread-count`. Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
Marking a notification read writes `read_at` — a persistent state change with
no event, so root rule 6's test ("trace the action to an `addEvent`/`emitEvent`
in the same TX") failed on it. Rule 6's exceptions cover retention sweeps, not
an explicit user action.
`notification.read` joins the catalog (typed declaration, Zod payload,
retention, visibility, description) and both routes now emit it inside the
transaction that performs the write, through `ITransactionService.run` — the
same shape `PUT /profile/locale` already uses.
Rule 7: subject and actor are the same person here — both routes carry
`denyImpersonated` and the update is scoped to rows the caller owns — so
`userId` alone is the actor. The payload says so in a comment rather than
leaving the reader to infer it from the absence of `actorUserId`.
The store's write methods now take the caller's transaction and return the ids
they actually wrote, so the payload reports what changed rather than what was
asked for; nothing changed means nothing is emitted. Retention is
`operational`, not `compliance`: a read receipt is derived UI state, not a
forensic record. `read-all` reports a count and no ids — the unread set has no
upper bound, and copying it into the outbox and the audit row would buy a
detail nobody reads at the price of rows of arbitrary size.
Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
The hub does real I/O — a dedicated `pg` connection holding a `LISTEN` — yet took no `IInstrumentation` and answered all three of its catches with a local `logger.warn`. Root rule 8 is not optional here: this is the component whose connection leak drifted for an entire audit while producing nothing but WARN lines nobody was watching. Instrumentation now arrives through the constructor (never a module singleton), `start()` and `connectListener()` declare outer spans with a `db.query` inner span around the `LISTEN`, and every catch calls `capture` before recovering. A listener close with no error is a breadcrumb, not a capture — it is expected during a Postgres restart, and only becomes a signal next to the reconnect that follows it. Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
`/api/v1` is authenticated by bearer token only and deliberately absent from `AppType`, but a third party holding a token had nothing to read: the only mentions of it lived in the internal architecture docs, which explain why the sub-app is separate and never how to call it. D.2 (generated OpenAPI + Scalar) is cut, so a hand-written reference is the only path. `docs/PUBLIC-API.md` documents the three routes with their real request and response shapes, the exact header and token format, the scope required per route, the token lifecycle and every way it dies, the two rate-limit windows with the headers they advertise, the common error envelope with the codes a client can actually see, and the boundary of what is deliberately unreachable by token (token management lives outside `/api/v1`, so a token cannot mint a token; there is no `admin` scope). `POLICY_ACCEPTANCE_REQUIRED` gets its own section: the H.1 gate on `PATCH /api/v1/me` means a token client can now receive a 409 it never received before, and it is not a transient failure — the owner must accept the new terms in the application, and no backoff will clear it. Linked from `docs/FEATURES.md` and `docs/OVERVIEW.md` where `/api/v1` is already discussed, and `ROADMAP.md` now says what was cut is the *generation*, not the documentation. Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
…ed address `check-fanout-preferences.ts` defaulted to a maintainer's personal address while `db:seed` created `[email protected]`, so `pnpm --filter api check:fanout` threw on every freshly seeded environment: "no user for [email protected]". A verification gate that does not run is a gate that does not exist — and this one is the only executable proof of the preference cascade, whose SQL no unit test can reach. The two literals had already drifted once, so the default now lives in one place both read (`scripts/seed-account.ts`); `SEED_EMAIL` overrides them together or neither. A personal address also has no business in a generic boilerplate, least of all as the default target of a `DELETE FROM notification`. The script also joins the `requireLocalDatabase` family it belonged to all along: it wipes rows for the resolved user, so a misconfigured `DATABASE_URL` must fail fast rather than delete someone's data. Verified green against a freshly seeded local database with no `SEED_EMAIL` set: 8/8. Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
…ation
`POST /notifications/read` scopes its UPDATE to the caller, so a foreign id
changed nothing — and the route still answered `200 {ok:true}`. The isolation
was never in doubt; the contract was. The repo's rule is explicit: wrong owner
is `Option.none()` on a read and `NOT_FOUND` on a write, never a 403 that
leaks the row's existence. A 200 that claims an action it did not perform is
no better than the 403: the client cannot tell "marked" from "ignored", and
its optimistic update decremented the badge by every id it sent, foreign ones
included.
The store now reports the ids it actually wrote, and the route compares them
against what was asked. A single unmatched id fails the whole batch with
`404 NOTIFICATION_NOT_FOUND` and rolls the transaction back — all of it
applied or none of it, never a silent partial. Repeats in the request body are
deduplicated first, so an id sent twice is not mistaken for a missing row.
Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
GET /notifications already returned nextCursor but the bell only ever fetched the first page, so anything past the last 20 notifications was unreachable. Switches the list to a TanStack infinite query, adds a "load more" control that disappears once nextCursor is null, and reworks the read-state patch (applyRead) plus the SSE invalidation to operate over every loaded page instead of a single one - so mark/mark-all stay accurate across pages and a live push no longer refetches every page the user has scrolled through.
`GET /notifications` returned the stored event payload verbatim, and the fan-out copies the event's payload verbatim into the row. Today the exposure is nil — `org.member.invited` carries its `invitationId` to an audience that already holds the invite capability — but there was no allowlist anywhere on the path, so any field added to a notifiable event's payload tomorrow would reach the recipient's browser by default, silently and forever. `NotificationConfig` now requires `payloadFields`, and the route projects through `publicNotificationPayload()`. Required, with empty as a valid answer: declaring nothing is a decision, inheriting everything is not — and the compiler now asks the question of anyone adding a notifiable event. The stored row keeps the full payload; only the read path narrows, so debugging and retention lose nothing. Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
…ey were written Three test files replaced `@packages/events` with a hand-kept copy of the catalog, and all three had frozen at the same commit: seventeen event types missing, `notification.read` among them. `mock.module` leaks across the whole process, so the copy did not fail its own file — it silently turned `EventTypes.X` into `undefined` in whichever *other* file happened to run after it. The new `notification.read` assertion failed in the full suite and passed in isolation, which is the shape this defect always takes. Each mock now spreads the real module and re-declares only what it means to stub, so a type added tomorrow is present everywhere and the deliberate stubs still win. The repo already required exposing the superset for the `drizzle` mocks; the same reasoning applies here. Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
Switching organization left the previous org's data on screen — and coming back served the entry the second org had overwritten, empty until a manual reload. The server scopes these responses on `session.activeOrganizationId` (`requireOrg`, or the handler reading it directly), but nothing in the URL or the call site said so, so a single cache entry served every organization. Clearing the cache on switch would have worked once; a key that carries the organization works for every query added later, and keeps each org's entry so going back is both instant and correct. Keyed: webhook endpoints/deliveries/delivery detail, API tokens, subscription, current membership — the list established from the back routes, not from names. SSO stays untouched: `GET /sso/providers` filters on the user, not the active org, and the provider detail is already keyed by `providerId`. Absence is `null`, never `undefined`, which a serialized key drops — collapsing "no org" and "org X" onto one entry, the very bug being fixed. Surfaces the server refuses without an org carry the guard in the factory (a call site that spreads the options would otherwise overwrite it); surfaces where `null` is a real scope keep answering it. `ensureOrgPermission` resolves the same org id in `beforeLoad`, so preloading and rendering share one entry. Every `invalidateQueries` follows the new segment — a key that gains one while an invalidation keeps the old prefix is a silent no-op.
Same defect a4cc582 fixed elsewhere, in the one query that commit could not reach: the route is requireOrg, so the response belongs to the active organization, but the key named only "org-preferences". One cache entry served every organization, and with staleTime 30s the switch back re-served the other one's matrix. Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
Same defect as a879d54 in the notification stream hub, but in the event pipeline: a dead `pg` connection emits `error` then `end`, both scheduled a reconnection, and the dying client was never unwired — listeners stayed attached and kept relaying NOTIFY, kept scheduling further reconnections. Every Postgres blip in production permanently added a live LISTEN connection and multiplied event processing. Applies the same fix shape as the hub: uniqueness held by a single pending reconnect timer (not a `connecting` flag, which would wedge if `error` fires while `connect()` is in flight), a dying client torn down exactly once via a `disposerFor` closure, `listenClient` cleared only when it still points at the dying client, and `reconnectBackoff` reset only on a connection that actually reached `LISTEN`. `pg.Client` is narrowed to an injectable structural shape (`OutboxListenClient`) so the contract is provable without a database, mirroring the hub's `NotificationListenClient`. Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
check-sweep-lock.ts acquires/releases real sweep leases and deletes rows
through purgeBatchWithTimeout against the real DATABASE_URL, exactly like
check-enqueue/check-marksent/check-wipe-rollback/check-fanout-preferences —
but it was missing their shared requireLocalDatabase("...") guard, so a
misconfigured DATABASE_URL could run its mutations against a shared or
production database instead of failing fast.
Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
The server sends event: ping every 25s (apps/api .../notifications/routes.ts, stream.sleep(25_000)) but the client had no time guard on it. A silent-but- open connection — a proxy that keeps the socket up without relaying anything — left connected=true forever: no reconnection, no polling fallback, and a frozen unread count with no signal to the user that anything broke. consume() now arms a stall timeout (2x the ping interval plus a margin) that cancels the reader when nothing arrives in time. Cancelling resolves the pending read as done, so consume() returns normally and the existing reconnect/backoff loop in useNotificationStream takes over. The timer is rearmed on every frame received, ping included, and always cleared in a finally so it can't outlive the read loop. Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
The `frequency` column and its selector shipped and persisted, but nothing read them: the fan-out wrote `email_pending_at = occurredAt` unconditionally and the flush selected `email_pending_at <= now()`, so `hourly` and `daily` behaved exactly like `immediate`. A preference set to daily produced a digest seconds later. Read it at the fan-out, not at the flush. The fan-out already joins the recipient's preference row to decide `enabled`; resolving `frequency` through the same org-lock -> user -> org-default -> fallback cascade costs one more COALESCE on a join that exists anyway, where reading it at the flush would mean re-resolving every recipient's preference on every tick. So the flush's selection did not change at all — the whole feature is one column's meaning: `email_pending_at` is now "when this may be mailed", never "when this happened". Windows anchor on wall-clock boundaries (`digestDueAt`): the next full hour, or the next NOTIFICATION_DIGEST_HOUR_UTC (default 08:00). Deliberately not "24h after the last send", which drifts a little further every cycle and is reproducible in no test. The boundary is strict — an event landing exactly on the anchor goes in the next window, since the digest for the one it sits on has already been cut. The three candidate timestamps are computed in TypeScript and bound as parameters; only the choice between them is SQL, so the window rule stays somewhere a unit test can reach. `forced` events never defer. They join no preference row, so the fan-out emits no frequency branch for them at all — which is the point: every forced event in the catalogue (password changed, MFA toggled, passkey added, deletion requested, payment failed, subscription cancelled, token created) loses its value if it arrives tomorrow. The flush now takes the `sweep_lock` lease, same primitive as the retention sweeps. `FOR UPDATE SKIP LOCKED` already made a double send impossible, but two concurrent runs could each claim half of one user's due rows and each mail a digest — no duplicate, two emails for a window that promised one. An empty window still sends nothing, and a replay sends nothing. Verification: `digestDueAt` is pure and covered by 9 unit tests. The parts that live in SQL are covered by `pnpm --filter api check:digest` (`scripts/check-digest-window.ts`, 19 assertions against a real Postgres through the real signed route) — a mocked tx evaluates no CASE, and asserting on generated SQL text is banned here for good reason (it depends on the real `sql` tag, which another file's `mock.module` replaces process-wide). Proven in execution: the three cadences write the three expected due dates, a forced event stays immediate under a daily preference, an empty window enqueues nothing, a due window groups four notifications into one email, and of two concurrent runs exactly one works while the other answers `skipped`. Time travel is done by dating rows into the past, not by waiting. No new event type — the catalogue stays at 82. This changes when an existing write becomes eligible, not what is written. Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
The create form offers two scopes, "Personal" (organizationId: null, the default) and the active organization, but the list only ever asked for one: `organizationId: session.activeOrganizationId ?? null`. Every user owns a personal organization, so the `?? null` branch is unreachable and a token created with the Personal scope never appeared for its owner — a live, valid credential nobody could see, and therefore nobody could revoke. `TokenOwner` becomes a closed union — `personal` | `orgAndPersonal` — so the selected set is named by the variant instead of encoded in a nullable field, and a third scope has to be declared rather than smuggled in as a flag. The repository widens its organization leg to "no organization OR this one" (`or(isNull, eq)`), always AND-joined on `userId`; `ownerReaches` states the same rule in the application layer and `ApiTokenService` re-applies it to what the repository returns, so a row out of reach is absent, never 403. Revocation inherits the widened scope, which is what makes a personal token revocable again. The list now mixes both scopes, so each row carries a scope badge — the organization's name, or "Personal" — behind new keys in both catalogs. Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
bun's mock.module replaces a module for the whole process, not for the
file that calls it, so the suite's verdict was a function of file order.
That is not theoretical: `bun test src/modules/admin src/modules/api-token`
ran 63 of 73 tests and reported 3 failures and an error, while the same
files under --isolate run all 73 and pass. Three separate defects in this
session traced back to it, including hand-kept @packages/events copies
that blanked 17 event types in *other* files, whose assertions then passed
against undefined.
--isolate makes the property structural rather than disciplinary, and
--parallel makes the suite faster than it was sequentially (2.5s -> 1.1s).
The hand-copied export surfaces go away with the leak that motivated them:
what remains spreads the real module and derives its maps from the real
catalog, because a frozen copy is correct exactly once.
module-isolation.{1,2}-*.test.ts is a named canary — one file replaces a
module nothing else imports, the other asserts it never saw the
replacement — so losing isolation fails there rather than as an
unexplained cascade elsewhere. Verified: it fails without --isolate.
pino-pretty ships to a worker thread and a per-file registry would spawn
and tear one down per file, racing into "the worker thread exited"; the
logger stays in-process and silent under test, where nothing asserts on it.
865 pass on three randomized seeds.
Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
check-digest-window.ts is a 19-case executable check written precisely because the frequency cascade lives inside an INSERT ... SELECT and the due filter inside a SELECT ... FOR UPDATE SKIP LOCKED — neither of which a mocked tx evaluates. Its siblings check:fanout and check:sweep-lock are CI steps; this one was declared in package.json and called by nothing, which is the exact failure the rule it cites warns about: a rule everyone believes is enforced and nothing enforces. The counts said 82/35/47 in one place and 67/28/39 in another, in the same file. Only current-state claims are corrected; the per-phase lines that record what a catalog looked like at its own phase stay as they are. Also adds PATCH /api/v1/me to the token-reachable list, which docs/PUBLIC-API.md documents at length as the public API's only write. Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
The flush now re-checks the current email preference (same org/user precedence as the fan-out) instead of trusting the decision made at insertion, drops rows whose channel was turned off since (clearing emailPendingAt instead of leaving them eligible forever), and pages through the whole due backlog inside one lease so a run bigger than batchSize no longer splits into two digests. Also closes an untested reconnect race (error landing while connect() is in flight) in both the notification stream hub and the outbox dispatcher, aligns OutboxDispatcher.start() with NotificationStreamHub's re-entrance guard, and replaces a stale "confinement test exists" docstring on the api-token repository with an executable check against a real database (check:api-token-visibility, wired into CI). Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
notInArray(n.id, claimedIds) grew one bound parameter per row across the whole backlog — pathological NOT IN and a hard wall at Postgres's 65535-param limit on a large digest window. The real fix was to stop deferring the "kept for sending" rows: mark their emailSentAt in the same transaction, in the same loop iteration that selected them, instead of waiting for the merged digest to be built after every page had been read. Once both outcomes (dropped -> emailPendingAt = null, sent -> emailSentAt = now) happen per page, the WHERE clause alone keeps a row from resurfacing on the next page and the accumulator is gone — memory and query size stop growing with backlog size. check-digest-window.ts [8] now also proves no row is processed twice across pages and a replay of the same paged window sends nothing. Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
`check:wipe-rollback`, `check:enqueue` and `check:marksent` sit on the same rail as the four already wired: each proves a behaviour that lives inside SQL (a transaction rollback, `onConflictDoNothing`, a `CASE` plus an attempts increment), which a mocked transaction can only assert was called, never that Postgres agreed. They were written, scripted and documented, and nothing ran them — so they proved nothing. All three create and remove their own fixtures, so they are placed after the seed and leave the audited account untouched for the a11y gate that follows. Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
The branch shipped 50 commits and the documents describing them stopped being true along the way. Corrected against commands, never against prose: - The event catalog moved for the first time since E.1a. `notification.read` makes it 82 / 35 public / 47 internal, and eight documents still claimed 81, 80 or 67. Where a document was recording a phase's *own* contribution rather than the running total, the stale total is dropped instead of refreshed — a value sheet is not a history. - `docs/HISTORY.md` gains what H.1 delivered beyond its plan: the dead 2FA path, the unbounded modal primitives, the replayable verification example, the public API reference, the org-scoped cache keys, the notification rail audit, the invisible personal tokens, and the test isolation that revealed ten tests were never running. - `apps/api/CLAUDE.md` still described `index.ts` as the place routes are mounted and the error handler installed; both moved to `app.ts` when app construction was split from server boot. - Three counts were wrong before this branch and are corrected too: README's a11y page count, `HEALTH-PROBES.md`'s claim that shutdown stops two runners when it stops four, and the policy gate's route count (19 business routes plus `PATCH /api/v1/me`, not 20 plus one). - `EVENTS.md`'s receiver snippet verified the first `v1=` only, contradicting the rotation paragraph directly above it, and checked the signature before the timestamp. Aligned with the snippet the product actually hands out. Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
Deleting `if (this.reconnectTimer !== null) return;` from either service left every reconnection test passing, so the line that *is* the connection-leak fix was asserted by nothing. The reason the existing tests miss it is worth stating, because it is not an oversight in them: `dispose()` removes the client's listeners synchronously, so the `end` that `pg` emits right after `error` no longer reaches a handler — the double schedule cannot come from that pair any more. It comes from the one path left: `error` fires while `connect()` is still in flight and schedules, then `connect()` rejects and `connectListener`'s catch schedules again. Both tests now drive exactly that, and both fail with the guard removed. Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
…ed it `check:digest` case 5 required the losing run to report `skipped: true`, which only happens when it reaches the lease before the winner releases it. On a slower runner the two runs serialize instead, the loser reports `skipped: false, flushed: 0`, and the check failed with nothing wrong — the window still produced exactly one digest, which the next two assertions already prove. The invariant is one window, one digest. The lease's contention semantics are covered against the same table by `check:sweep-lock`; what belongs here is that the second run sent nothing, whichever way it lost. Also stop the `if: failure()` API-log step from failing on its own when the run died before the API started — a missing-file error printed over the real one. Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
|
🎉 This PR is included in version 1.24.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
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
1 participant
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.
What this is
Phase H.1 — Back ↔ front surface parity, plus everything the audit turned up while running. 52 commits.
The plan's own deliverable: a checked-in surface map (
apps/api/src/shared/surface/) that reads the route table off the live Hono app rather than being hand-maintained beside it, extracts the front's call sites from source, and fails the build when a route gains or loses a consumer without the map saying so. 71 routes, 52 with a front consumer, 19 declared UI-less with a typed reason each. Readingapp.routesfrom a test required splitting app construction from server boot —app.tsnow builds,index.tsonly boots.Both confirmed gaps are closed: a platform admin can change an account role from the UI (the route was live, guarded and audited, with nothing calling it), and replacing an avatar no longer leaks the previous storage object. A third suspected gap — front controls calling nothing — was audited and infirmed, recorded so nobody re-runs it.
Beyond the plan
Auditing a finished surface finds what a plan written before the audit could not list. Every item below was live on
dev:two_factorcolumns[email protected]writes were undeclared, so every verification 500'd — and sincerequirePlatformAdmindemands a fresh MFA challenge, the whole/admin/*console was unreachable. Schema and migration (db:pushalone doesn't reach a fresh production database).@packages/ui, not per call site.t=<ts>, signed with it, and never checked it. A captured request stayed valid until secret rotation.requireOrgqueries had a key that didn't name the organization, so withstaleTime: 30sthe previous org's response was served and kept.LISTENconnection leaks (hub and dispatcher: both handlers reconnecting, dying client never disposed); an email-frequency selector that persisted and was never read; history with no way to reach page two; a mark-read path that emitted no event, against root rule 6; and an SSE frame forwardinginvitationId— the acceptance token — to every connected client.?? nullon a value that is never null, so org-less tokens never appeared: live, unrevocable credentials nobody could see.bun testshares one module registry, somock.moduleleaks process-wide. Measured:bun test src/modules/admin src/modules/api-tokenran 63 of 73 with 3 failures; with--isolate, 73/73 pass./api/v1had no referencedocs/PUBLIC-API.md— auth, scopes, routes, error table, signature scheme. Every example executed against a running instance.Gates
pnpm --filter api test867/867 across 105 files, green on three--isolate --randomizeseeds — the assertion that matters after an isolation fix, since one ordering proves nothing. app 328/328, i18n 21/21. Biome 912 files, 0 fixes.type-check13/13.knipexit 0.jscpd35 clones / 517 lines / 1.15% against E.1b's 1.13% — two hundredths across ~9,200 new lines.Seven executable checks now run against a real Postgres in CI. Four were wired here; the other three (
check:wipe-rollback,check:enqueue,check:marksent) already existed, were already documented, and nothing had ever run them.Docs
Every document was re-derived from commands rather than from the prose beside it. The event catalog moved for the first time since E.1a (82 / 35 public / 47 internal) and eight documents still claimed 81, 80 or 67. Three counts that were already wrong before this branch are corrected too.
https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju