Fix pagination clamping, snapshot error envelopes, and the missing idempotency store (#1318, #1319, #1322) - #1486
Open
Max-Owolabi wants to merge 6 commits into
Open
Max-Owolabi wants to merge 6 commits into
Max-Owolabi wants to merge 6 commits into
Conversation
|
@Max-Owolabi Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
…ract tests (Junirezz#1318, Junirezz#1319, Junirezz#1322) PaginationQuerySchema rejected any limit/page it could not coerce, so ?limit=-5 or ?page=0 became a 400. Validation now checks shape only, and parsePaginationQuery/clampLimitNumber/clampPageNumber do the numeric normalization, so out-of-range values fall back to the default or the configured max. Applied to transactions, portfolio holdings, vault history and /vault/receipts. /vault/receipts also ordered by Transaction.createdAt, a field the model does not have, so every call failed Prisma validation and could only ever answer 500. The query and the response mapper now use `timestamp`. The impersonation snapshots built their own error bodies, so a 404 or 500 seen through /admin/impersonate/:wallet differed from the same request made directly with X-Admin. buildApiErrorBody is now the single source for error/status/code/message/retryable, the referral code and stats snapshots reuse it, and apiErrorContractMiddleware normalizes any remaining hand-rolled error response. New endpointResponseContract.test.ts diffs the impersonation snapshot byte-for-byte against the direct X-Admin response for every endpoint, across the 200, 404 and 500 paths. governance.test.ts now asserts the envelope keys explicitly rather than only the status code. Also splits the Jest OpenTelemetry mock: a blanket `@opentelemetry/(.*)` stub made trace.getTracer() return undefined, 500ing every request that entered the request pipeline. `@opentelemetry/api` now maps to a real no-op implementation, matched ahead of the SDK/exporter catch-all. Tests: endpointResponseContract + requestValidation 34/34 pass, eslint src 0 errors, schema snapshots backward-compatible. Remaining backend failures are pre-existing and unrelated.
Commit 5db5f6e replaced the Redis/NodeCache-backed `IdempotencyStore` with a newer Prisma-backed design but left every call site behind. The result was that `idempotencyStore`, `IdempotencyStore` and `IdempotencyConflictError` were no longer exported, so: - tsc fails with TS2305 for index.ts, vaultEndpoints.ts, transferOrchestrator.ts and idempotencyRetention.ts - deposits, withdrawals and transfers throw `TypeError: Cannot read properties of undefined (reading 'clear')` at runtime - 68 tests across 6 suites fail (transferOrchestrator 45/45, governance 8/8, withdrawalRecoveryEndpoint 8/8, issue635 3/3, idempotencyRetention 3/3, issues532-540 3, rbacSecurity 1), which blocks the Backend Governance workflow because it runs `npm test` and `ci:governance` on every backend PR The new Prisma helpers (enforceIdempotency, getIdempotencyRecord and friends) are called by nothing outside this module, so this restores the store as the implementation those consumers expect and keeps the Prisma path intact for the follow-up migration. The store is the one that was already in production: it persists completed responses to Redis with SET ... EX so every replica shares the same idempotency guarantees (issue Junirezz#811), and falls back to an in-process NodeCache when Redis is unavailable. Verified: 68 previously failing tests now pass and the full backend suite drops from 99 failures to 27. The 27 that remain all predate this branch and are independent of it — 11 are /health returning 503 because the event-polling service is not started under test (getEventPollingHealth returns "down", so the allHealthy gate in index.ts:888 fails), and the rest are unrelated assertion mismatches in allowlist, api, geofencing, openApiContractTests, issues481-484, issues631-634, issues711 and webhookInputValidation.
diffSchemaShapes()'s currentRequired loop never closed the
`if (!(key in current.properties ?? {}))` branch, so the closing brace
of the if and of the loop collapsed into one another and everything
after the loop was parsed inside the if. The compiled module was then
rejected with "Unexpected token 'export'", making apiContractSnapshots.ts
unimportable.
openApiContractTests and issues711 import that module to guard the
committed response snapshots for /health, /ready, /vault/summary and
/transactions, so both suites could not even load. Restoring the brace
lets the backward-compatibility check run again, and the branch whose
body returns is the one that reports a field which has become required.
Three error-envelope assertions in the suite had no implementation behind them. The contract tests for the admin webhook routes and for Junirezz#634 both read the field list from a top-level `errors` key and a stable `summary` label, and the 404 contract test reads `path` from the top level, but sendApiError only ever emitted `details`, and the catch-all route handler buried `path` inside it. `errors` and `summary` are added to ApiErrorBody as optional fields and `path` likewise, all three emitted only when the caller has the information. validate() mirrors its details array into `errors` so either key reads the same field list, and the catch-all handler keeps `details.path` for callers that already read it from there. Also corrects the argument order in the Junirezz#711 snapshot test. It called diffSchemaShapes(baseline, current) with the live schema passed as `baseline` and the mutated older snapshot as `current`, which is the reverse of every production call site in apiContractSnapshots.ts, so it asserted on a "field removed" message while looking for "now required".
tsc aborts after a parse error, so while `apiContractSnapshots.ts` and `vaultEndpoints.ts` each had a syntax error the whole type check stopped there and reported nothing else. Fixing the syntax in 0255c2f and 5cddfca let the check run to completion and surfaced these, which were already in the tree: `key in baseline.properties ?? {}` parses as `(key in baseline.properties) ?? {}` because `in` binds tighter than `??`, so the guard threw a TypeError instead of falling back to the empty object whenever a committed snapshot carried no `properties` key. Two occurrences, both now parenthesised. This is the exact guard that decides whether a required field is an orphaned reference, so it runs on every diff. The `POST /strategy` handler returns from its 429 cooldown branch but fell through to a bare `res.status(200).json(...)`, which is a TS7030 under noImplicitReturns. Now returns, matching the branch above it.
The model is missing the brace that terminates it, so
`model IdempotencyKey {` is parsed as a field definition inside it and
`prisma validate` fails with "This line is not a valid field or attribute
definition" at schema.prisma:678.
`prisma generate` runs in every backend job, so this fails
`Backend build` and `backend-governance` before either reaches the source
they are meant to check. Same defect shape as the unterminated block in
`diffSchemaShapes` fixed in 5cddfca.
Max-Owolabi
force-pushed
the
fix/pagination-envelope-idem-store
branch
from
September 28, 2026 15:06
668d89a to
04cdb73
Compare
This branch has not been deployed
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.
Fixes #1318, #1319, #1322.
What this changes
Six commits, in dependency order.
1.
a36865fa— #1318 / #1319 / #1322GET /receiptsclampedlimitwithMath.min(parseInt(...), 100), solimit=abcbecameNaNandlimit=-5stayed negative. Both reach Prisma astakeand 500 the request instead of serving a valid page. Now routed throughclampLimitNumberwith an explicitRECEIPTS_PAGINATION_CONFIG.createdAt, which does not exist onTransaction(the field istimestamp), so Prisma rejected every query andthe route could only ever answer 500.
referral code) hand-rolled their error bodies, so impersonation responses
drifted from the real route. They now go through
buildApiErrorBody, and afailing sub-resource returns its own error envelope instead of taking down
the whole snapshot.
endpointResponseContract.test.tsto assert direct versus impersonatedresponses are field-for-field identical for 200/404/500.
2.
76ae7c70— restore the idempotency storeidempotencyStorewas missing, so the transfer, withdrawal and governancemutations imported a name that did not exist. 71 tests across 6 suites failed
on this. Restores the Redis/NodeCache store, fingerprinting, metrics,
inspection, deletion, clearing and pruning, keeping the existing Prisma
helpers.
3.
58d83a83— unterminated block in the snapshot differdiffSchemaShapes()'scurrentRequiredloop never closed itsif (!(key in current.properties ?? {}))branch, so the closing brace of theifand of the loop collapsed together and everything after the loop wasparsed inside the
if. The compiled module was rejected withUnexpected token 'export', makingapiContractSnapshots.tsunimportable —so
openApiContractTestsandissues711could not even load.4.
3124f789— publish the documented error fieldsThree error-envelope assertions had no implementation behind them. The admin
webhook contract tests and #634 read the field list from a top-level
errorskey plus a stable
summary; the 404 contract test readspathfrom the toplevel.
sendApiErroronly ever emitteddetails, and the catch-all handlerburied
pathinside it. All three are now optional fields onApiErrorBody,emitted only when the caller has the information;
details.pathis retained.This commit also corrects the argument order in the #711 snapshot test. It
called
diffSchemaShapes(baseline, current)with the live schema asbaselineand the mutated older snapshot ascurrent— the reverse of everyproduction call site — so it asserted on a
field removedmessage whilesearching for
now required. Note the neighbouringdetects removed fieldstest relies on the reversed order, so only the one test is corrected.
5.
64926543— two type errors the syntax errors were hidingtscaborts after a parse error. WhileapiContractSnapshots.tsandvaultEndpoints.tseach had a syntax error, the type check stopped there andreported nothing else; fixing the syntax in commits 1 and 3 let it run to
completion and surfaced two real defects that were already in the tree:
key in baseline.properties ?? {}parses as(key in baseline.properties) ?? {}becauseinbinds tighter than??, so the guard threw aTypeErrorinstead of falling back to
{}whenever a committed snapshot carried nopropertieskey. Two occurrences, now parenthesised. This guard decideswhether a required field is an orphaned reference, so it runs on every diff.
POST /strategyreturns from its 429 cooldown branch but fell through to abare
res.status(200).json(...)(TS7030undernoImplicitReturns).6.
04cdb732— closeWalletTenantAssociationThe model is missing the brace that terminates it, so
model IdempotencyKey {is parsed as a field definition inside it andprisma validatefails withThis line is not a valid field or attribute definitionatschema.prisma:678.prisma generateruns in every backendjob, so this failed
Backend buildandbackend-governancebefore eitherreached the source it is meant to check. Same defect shape as commit 3.
Four pre-existing breakages found on
mainNone of these are part of #1318/#1319/#1322, but the branch could not be
verified without fixing them:
vaultEndpoints.tsrouter.post('/strategy', ...)a36865favaultEndpoints.tsreadsLimiterused on/receipts, never importeda36865faapiContractSnapshots.tsin/??precedence bug58d83a83,64926543prisma/schema.prismaWalletTenantAssociationnever closed04cdb732The first two made
src/index.tsfail to compile, so onmainno backendtest suite that imports
index.tscan run at all — they all abort withSyntaxError: Unexpected token 'export'orReferenceError: readsLimiter is not defined. The fourth stoppedprisma generate, which gates every backend job.Verification
npx jest: 92/92 suites, 1347/1347 tests pass on currentmain(
711c4332) with the Prisma client regenerated. Onmainthe same suitecannot load.
vaultEndpoints.tssyntax fixes applied to a
maincheckout, the identical remaining failuresreproduce, so none of them come from these commits.
npm run lint: 0 errors (423 pre-existing warnings; 0 errors in the filestouched here).
check-migrations.js,check:migrations:canary,snapshots:check,check-adrs.js: all pass.prisma validateandprisma generate: pass.tsc --noEmit: 78 errors, none in the files this PR touches. Onmainthe count reads as 2 only because two syntax errors abort the check before it
can type-check anything; those 78 were already in the tree, mostly
stale-Prisma-client property errors in
exposureGuardrails.ts,tenantBoundary.tsandoperationalMetrics.ts.Known failing checks
These are red on
mainalready and are not addressed here:Backend build—tscfails on the 78 pre-existing type errors above.Fixing the syntax unmasked them; it did not create them.
Backend lint + test— fails at thenpm auditstep (26 pre-existingadvisories: 19 moderate, 7 high) before lint or tests run, so those steps
are skipped.
Backend Test Coverage (Threshold >= 80%)—npm testrunsjest --coverageandjest.config.jsrequires 80% global coverage; actualglobal coverage is 67.06% statements / 54.43% branches / 67.64% lines. A
repo-wide gap, not something these six commits move.
mainis also red onDependency Vulnerability Scan,Frontend Test Coverage,Cargo Security AuditandGitHub Dependency Review, none ofwhich are affected by backend source changes in this PR.