Add a consistency test asserting FIX - #215
Merged
Jagadeeshftw merged 1 commit intoJul 28, 2026
Merged
Conversation
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.
Guard cross-route consistency of non-positive "amount" rejection
Closes the amount-validation consistency issue.
Problem
Three write paths accept an "amount" field in their request body and all three
validate it through the same helper, requirePositiveNumber in
src/utils/validation.ts:
POST /api/v1/quote -> QuoteService.quote
POST /api/v1/liquidity -> LiquidityService.addLiquidity
POST /api/v1/settlements -> SettlementService.open
The helper throws ApiError.badRequest('"amount" must be a positive number'),
which errorHandler renders as HTTP 400 with the envelope
{ error: { code: "BAD_REQUEST", message: ... } }. Because all three share the
helper, their 400 responses are identical today in status, code, and message.
Nothing proved that. Each route had its own suite, and each suite only checked
its own route. If one call site later replaced the shared helper with bespoke
handling - a custom error code, a different status, or a reworded message that
no longer names the offending field - every existing test would still pass
while API clients silently started seeing two different error contracts for
what is the same class of input fault. That is exactly the kind of divergence
that only a cross-cutting test can catch.
Investigation result
No inconsistency exists in production code. All three call sites already funnel
through requirePositiveNumber unmodified, and the HTTP responses were verified
to be byte-identical. Per the acceptance criteria, production code was
therefore left untouched: this PR is test-only.
What changed
One new file, src/routes/amountValidationConsistency.test.ts, exercising all
three routes over supertest against a real createApp() instance.
Table-driven per-route assertions
A route table (path plus the other, valid body fields, so "amount" is the sole
fault) is crossed with the non-positive values 0 and -1, producing six cases.
Each asserts:
status 400
error.code === "BAD_REQUEST"
error.message references "amount"
Envelope-equality assertions
Two further cases, one per invalid value, fire all three routes against a
single app instance and assert that the complete response bodies are deeply
equal to one another and to the expected shape
{ error: { code: "BAD_REQUEST", message: /"amount"/ } }.
This is the stronger half of the guard. The per-route cases pin the contract;
the equality cases pin the routes to each other, so a divergence in any single
call site fails even if that call site's own new behaviour would otherwise look
reasonable in isolation.
Validation
Mutation check. QuoteService.quote was temporarily rewritten to bypass the
shared helper and throw a custom INVALID_AMOUNT code with a message not naming
the field. Four of the eight new cases failed immediately; the change was then
reverted. The test demonstrably detects the drift it exists to prevent, rather
than passing vacuously.
Full suite: 41 suites, 477 tests, all passing
Build: npm run build (tsc) clean; tsc --noEmit clean
Lint: npm run lint clean
Coverage: 97.16% statements, 92.12% branches, 95.73% functions,
97.59% lines - above the 95% bar
Files
added: src/routes/amountValidationConsistency.test.ts
modified: PULL_REQUEST.md
production code: unchanged
Risk
None to runtime behaviour. Test-only, no dependency changes, no new exports.
The file adds roughly 0.1s to suite wall time.
Closes #150