Closes item 5 of #307. Item 4 is not closed and cannot be yet — see the end.
The circular dependency that kept this open
GET /api/items/:id was the last route reading its id with a bare Number(), so an unreadable id reached Postgres and came back as a 500 for an item that cannot exist. It answers 404 now, like every other id-taking route since #207.
It could not be fixed on its own, which is exactly why it stayed. errorHandling.integration.test.ts used this route's looseness as its way of making a handler reject — so tightening the parse would have left that test green while removing the thing it tests.
The test now fails a database call directly, spying on pool.query against a route that makes one. That is the failure the error middleware actually exists for, and it depends on no route declining to validate. The previous comment on the issue reached this conclusion itself: moving the trigger to cart.ts "only moves the wart".
Two things switching to readId would not have fixed
readId was not as strict as its name suggests.Number reads these as positive integers, so every check it made passed:
Input
Number()
Old readId
Now
5.0
5
5
null
1e2
100
100
null
0x10
16
16
null
+5
5
5
null
So /items/5.0 answered with item 5. This never raised an error and so never announced itself — the issue noticed it only because #308 converted the comparison to a real integer. Had I switched the route to readId and stopped there, the issue's own stated concern would still be live. An id is a string of digits, so it is now matched against digits before being parsed.
readId is also bounded at the top of a 32-bit serial. Above that Postgres raises 22003 rather than returning nothing — the same wrong answer to the caller as the 22P02 the function exists to prevent.
The leak assertion was passing for the wrong reason
It checks the response contains neither syntax nor items. The error it checked against happened to contain both only by accident of which route was used. The injected failure now contains both words deliberately, and the whole message is asserted against too, so a future error format cannot slip through by wording itself differently.
Verification
tsc clean · lint 0 errors, no new warnings · 485 unit tests across 33 suites, seven of them new and covering the inputs above.
The integration suite cannot run on this machine, so whether the rewritten error test passes is for CI to say.
Item 4 (coverage) is blocked, not skipped
The SonarQube Scan step has been skipped on every recent run — it is gated on the earlier steps succeeding, and those steps were failing. So the dashboard is stale and the duplication question item 4 asks about cannot be answered from it yet.
What the test steps themselves reported on run 875:
Statements
Branches
Functions
Lines
Backend
75.94%
59.37%
74.42%
77.47%
Frontend
77.59%
69.70%
72.68%
78.55%
Item 4 needs one run to go green end to end before it can be closed. Reported on the issue rather than guessed at.
Closes **item 5** of #307. Item 4 is not closed and cannot be yet — see the end.
## The circular dependency that kept this open
`GET /api/items/:id` was the last route reading its id with a bare `Number()`, so an unreadable id reached Postgres and came back as a **500 for an item that cannot exist**. It answers 404 now, like every other id-taking route since #207.
It could not be fixed on its own, which is exactly why it stayed. `errorHandling.integration.test.ts` used this route's looseness as its way of making a handler reject — so tightening the parse would have left that test **green while removing the thing it tests**.
The test now fails a database call directly, spying on `pool.query` against a route that makes one. That is the failure the error middleware actually exists for, and it depends on no route declining to validate. The previous comment on the issue reached this conclusion itself: moving the trigger to `cart.ts` "only moves the wart".
## Two things switching to `readId` would *not* have fixed
**`readId` was not as strict as its name suggests.** `Number` reads these as positive integers, so every check it made passed:
| Input | `Number()` | Old `readId` | Now |
| --- | --- | --- | --- |
| `5.0` | 5 | **5** | `null` |
| `1e2` | 100 | **100** | `null` |
| `0x10` | 16 | **16** | `null` |
| `+5` | 5 | **5** | `null` |
So `/items/5.0` answered with item 5. **This never raised an error and so never announced itself** — the issue noticed it only because #308 converted the comparison to a real integer. Had I switched the route to `readId` and stopped there, the issue's own stated concern would still be live. An id is a string of digits, so it is now matched against digits before being parsed.
**`readId` is also bounded at the top of a 32-bit serial.** Above that Postgres raises 22003 rather than returning nothing — the same wrong answer to the caller as the 22P02 the function exists to prevent.
## The leak assertion was passing for the wrong reason
It checks the response contains neither `syntax` nor `items`. The error it checked against happened to contain both only by accident of which route was used. The injected failure now contains both words deliberately, and the whole message is asserted against too, so a future error format cannot slip through by wording itself differently.
## Verification
`tsc` clean · lint 0 errors, no new warnings · **485 unit tests across 33 suites**, seven of them new and covering the inputs above.
**The integration suite cannot run on this machine**, so whether the rewritten error test passes is for CI to say.
## Item 4 (coverage) is blocked, not skipped
The **SonarQube Scan step has been skipped on every recent run** — it is gated on the earlier steps succeeding, and those steps were failing. So the dashboard is stale and the duplication question item 4 asks about cannot be answered from it yet.
What the test steps themselves reported on [run 875](https://gitea.bermudalamb.synology.me/bermudalamb/redefined-designs/actions/runs/875):
| | Statements | Branches | Functions | Lines |
| --- | --- | --- | --- | --- |
| Backend | 75.94% | 59.37% | 74.42% | 77.47% |
| Frontend | 77.59% | 69.70% | 72.68% | 78.55% |
Item 4 needs one run to go green end to end before it can be closed. Reported on the issue rather than guessed at.
Refs #307
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Item 5. GET /api/items/:id was the last route reading its id with a bare Number(), so an unreadable id reached Postgres and came back to the caller as a 500 for an item that cannot exist. It answers 404 now, like every other id-taking route since #207.
It could not be fixed on its own, which is why it stayed. errorHandling.integration.test.ts used this route's looseness as its way of making a handler reject: tightening the parse would have left that test green while removing the thing it tests. So the test now fails a database call directly, with a spy on pool.query against a route that makes one. That is the failure the error middleware actually exists for, and it does not depend on any route declining to validate — the previous comment's own conclusion, that moving the trigger to cart.ts would only move the wart.
Two things fixed along the way that the issue asked about but that switching to readId would not have delivered on its own.
readId was not as strict as its name suggests. Number reads 5.0, 1e2, 0x10 and +5 as 5, 100, 16 and 5 — every one a positive integer, so every check readId made passed and the route fetched a real row for a URL nobody wrote. /items/5.0 answered with item 5. This never raised an error and so never announced itself; the issue noticed it only because #308 converted the comparison to a real integer. An id is a string of digits, so it is matched against digits before being parsed.
readId is also now bounded at the top of a 32-bit serial. Above that Postgres raises 22003 rather than returning nothing, which is the same wrong answer to the caller as the 22P02 the function was written to prevent — a 500 for an id that identifies nothing.
The leak assertion was passing for the wrong reason. It checks the response does not contain "syntax" or "items", and the error it was checking against happened to contain both only by accident of which route was used. The injected failure now contains both words deliberately, and the whole message is asserted against as well, so a future error format cannot slip through by wording itself differently.
Verified: tsc clean, lint 0 errors with no new warnings, 485 unit tests passing across 33 suites — seven of them new, covering the inputs above. The integration suite cannot run on this machine, so whether the rewritten error test passes is for CI to say.
Item 4, coverage, is not closed by this and cannot be closed yet: the SonarQube scan step has been skipped on every recent run because it is gated on the earlier steps succeeding, and those steps were failing. The dashboard is therefore stale. Reported on the issue rather than guessed at.
Refs #307
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes item 5 of #307. Item 4 is not closed and cannot be yet — see the end.
The circular dependency that kept this open
GET /api/items/:idwas the last route reading its id with a bareNumber(), so an unreadable id reached Postgres and came back as a 500 for an item that cannot exist. It answers 404 now, like every other id-taking route since #207.It could not be fixed on its own, which is exactly why it stayed.
errorHandling.integration.test.tsused this route's looseness as its way of making a handler reject — so tightening the parse would have left that test green while removing the thing it tests.The test now fails a database call directly, spying on
pool.queryagainst a route that makes one. That is the failure the error middleware actually exists for, and it depends on no route declining to validate. The previous comment on the issue reached this conclusion itself: moving the trigger tocart.ts"only moves the wart".Two things switching to
readIdwould not have fixedreadIdwas not as strict as its name suggests.Numberreads these as positive integers, so every check it made passed:Number()readId5.0null1e2null0x10null+5nullSo
/items/5.0answered with item 5. This never raised an error and so never announced itself — the issue noticed it only because #308 converted the comparison to a real integer. Had I switched the route toreadIdand stopped there, the issue's own stated concern would still be live. An id is a string of digits, so it is now matched against digits before being parsed.readIdis also bounded at the top of a 32-bit serial. Above that Postgres raises 22003 rather than returning nothing — the same wrong answer to the caller as the 22P02 the function exists to prevent.The leak assertion was passing for the wrong reason
It checks the response contains neither
syntaxnoritems. The error it checked against happened to contain both only by accident of which route was used. The injected failure now contains both words deliberately, and the whole message is asserted against too, so a future error format cannot slip through by wording itself differently.Verification
tscclean · lint 0 errors, no new warnings · 485 unit tests across 33 suites, seven of them new and covering the inputs above.The integration suite cannot run on this machine, so whether the rewritten error test passes is for CI to say.
Item 4 (coverage) is blocked, not skipped
The SonarQube Scan step has been skipped on every recent run — it is gated on the earlier steps succeeding, and those steps were failing. So the dashboard is stale and the duplication question item 4 asks about cannot be answered from it yet.
What the test steps themselves reported on run 875:
Item 4 needs one run to go green end to end before it can be closed. Reported on the issue rather than guessed at.
Refs #307
🤖 Generated with Claude Code