The standing follow-up after a run of feature work, kept as its own issue and its own PR rather than folded into a feature branch. Four items, none of them failing anything today.
1. routesAreWrapped.test.ts cannot see a handler factory
From #301. POST /items/:id/images/:imageId/rotate-left and rotate-right are both correctly wrapped in asyncRoute, but they are built by a rotationRoute(direction) factory, and the guard only inspects the text inside a router.post(...) call. There is no async token at those two registrations, so the test passed by finding nothing rather than by finding the wrapper.
This is the first handler factory in src/routes/, and the guard exists precisely because the convention was half-forgotten once already. It should either understand a factory that returns asyncRoute(...), or fail loudly on a registration whose handler it cannot resolve — the second is probably easier to get right and harder to fool.
2. Two new SonarJS warnings in the schema-drift test
From #305, in code that change introduced, at backend/tests/integration/schemaMirror.integration.test.ts:33:
sonarjs/super-linear-regex — the table/interface regex backtracks
sonarjs/concise-regex — [A-Za-z0-9_] should be \w
Both are advisory and npm run lint still reports 0 errors, which is why they were parked rather than fixed in the branch. The regex is parsing a generated file of known shape, so the backtracking has no adversarial input — but it is new debt in new code and belongs here.
3. The S2077 hotspots are still open, and are now the only ones left
backend/src/routes/admin.ts:146 and backend/src/routes/items.ts:95 still interpolate buildItemFilterSql's composed where into a query string. #294 removed the other seven sites by making each fixed-shape query a named constant; these two genuinely build SQL at run time and are the reason #216 wanted a builder at all.
They are safe, and the argument is written out at admin.ts:132-143. What #305 changed is only that the tool for retiring the argument is now Kysely rather than Drizzle — the conversion itself is the separate issue filed alongside this one, so what belongs here is deciding what to do with the hotspots in the meantime: leave them reviewed and watched, or mark them with the specific reasoning so the next scan does not re-raise them cold.
4. Coverage and duplication
The usual pass. Worth checking whether the new src/imageRotation.ts and the reconverted adminCategories.ts moved the numbers, and whether the two rotation endpoints' shared handler factory reads as duplication to the scanner.
Not in scope
Converting any query to Kysely. That is its own issue.
The standing follow-up after a run of feature work, kept as its own issue and its own PR rather than folded into a feature branch. Four items, none of them failing anything today.
## 1. `routesAreWrapped.test.ts` cannot see a handler factory
From #301. `POST /items/:id/images/:imageId/rotate-left` and `rotate-right` are both correctly wrapped in `asyncRoute`, but they are built by a `rotationRoute(direction)` factory, and the guard only inspects the text inside a `router.post(...)` call. There is no `async` token at those two registrations, so the test passed by finding nothing rather than by finding the wrapper.
This is the first handler factory in `src/routes/`, and the guard exists precisely because the convention was half-forgotten once already. It should either understand a factory that returns `asyncRoute(...)`, or fail loudly on a registration whose handler it cannot resolve — the second is probably easier to get right and harder to fool.
## 2. Two new SonarJS warnings in the schema-drift test
From #305, in code that change introduced, at `backend/tests/integration/schemaMirror.integration.test.ts:33`:
- `sonarjs/super-linear-regex` — the table/interface regex backtracks
- `sonarjs/concise-regex` — `[A-Za-z0-9_]` should be `\w`
Both are advisory and `npm run lint` still reports 0 errors, which is why they were parked rather than fixed in the branch. The regex is parsing a generated file of known shape, so the backtracking has no adversarial input — but it is new debt in new code and belongs here.
## 3. The S2077 hotspots are still open, and are now the only ones left
`backend/src/routes/admin.ts:146` and `backend/src/routes/items.ts:95` still interpolate `buildItemFilterSql`'s composed `where` into a query string. #294 removed the other seven sites by making each fixed-shape query a named constant; these two genuinely build SQL at run time and are the reason #216 wanted a builder at all.
They are safe, and the argument is written out at `admin.ts:132-143`. What #305 changed is only that the tool for retiring the argument is now Kysely rather than Drizzle — the conversion itself is the separate issue filed alongside this one, so what belongs *here* is deciding what to do with the hotspots in the meantime: leave them reviewed and watched, or mark them with the specific reasoning so the next scan does not re-raise them cold.
## 4. Coverage and duplication
The usual pass. Worth checking whether the new `src/imageRotation.ts` and the reconverted `adminCategories.ts` moved the numbers, and whether the two rotation endpoints' shared handler factory reads as duplication to the scanner.
## Not in scope
Converting any query to Kysely. That is its own issue.
Refs #301, #305, #294, #297
5. GET /api/items/:id reads its id with Number() rather than readId
#207 standardised every other id-taking route on readId, which parses strictly and answers 404 for anything unreadable. This one route does not, and after #308 converted it to a real integer comparison that matters slightly more than it used to: /api/items/5.0, /1e2 and /0x10 now resolve to items 5, 100 and 16, where the previous string comparison had Postgres reject them.
It was changed to readId during #308 and then deliberately reverted, and the reason is the thing that makes this worth its own change rather than a one-liner. errorHandling.integration.test.ts drives this exact route with a non-numeric id — not to assert what the route does with a bad id, but to prove that asyncRoute plus the error middleware answer 500 rather than leaving the request hanging. Its own comment says a hung request "renders as an empty storefront rather than a visible failure". A stricter parse here removes that test's mechanism, so it would go on passing while no longer testing anything.
So switching this over means giving that test another trigger in the same change. cart.ts still takes req.params.itemId as a raw string on a public route and would serve, though that only moves the wart — the more durable answer is a trigger that does not depend on some route's validation gap.
The code carries a comment saying all of this, so the next person does not repeat the round trip.
6. eb('i.status', 'in', []) would emit invalid SQL
In itemFilterExpressions. An empty status list compiles to where "i"."status" in (), which is a Postgres syntax error; the = ANY($n::text[]) it replaced was valid and matched nothing.
Unreachable today — parseItemFilters throws on a list that names nothing, and the storefront's defaults are never empty — which is why it was parked rather than fixed. Worth noting that the obvious guard is wrong: skipping the clause for an empty list makes the filter match everything, where the old behaviour matched nothing. If it is worth closing, eb.val(false) preserves the old semantics.
Also worth knowing, not a task
itemSelect.ts narrows items.status from the generated mirror's Generated<string> to ItemStatus via Omit<DB['items'], 'status'> & { status: Generated<ItemStatus> }. That re-adds the column unconditionally, so a migration that dropped or renamed items.status would not be caught by the type — the one drift class the schema mirror otherwise closes. Narrow, and reviewed as sound, but it is the kind of thing worth knowing about before someone touches that column.
Two more for this issue, both from #308.
## 5. `GET /api/items/:id` reads its id with `Number()` rather than `readId`
#207 standardised every other id-taking route on `readId`, which parses strictly and answers 404 for anything unreadable. This one route does not, and after #308 converted it to a real integer comparison that matters slightly more than it used to: `/api/items/5.0`, `/1e2` and `/0x10` now resolve to items 5, 100 and 16, where the previous string comparison had Postgres reject them.
It was changed to `readId` during #308 and then deliberately reverted, and the reason is the thing that makes this worth its own change rather than a one-liner. `errorHandling.integration.test.ts` drives this exact route with a non-numeric id — not to assert what the route does with a bad id, but to prove that `asyncRoute` plus the error middleware answer 500 rather than leaving the request hanging. Its own comment says a hung request "renders as an empty storefront rather than a visible failure". A stricter parse here removes that test's mechanism, so it would go on passing while no longer testing anything.
So switching this over means giving that test another trigger in the same change. `cart.ts` still takes `req.params.itemId` as a raw string on a public route and would serve, though that only moves the wart — the more durable answer is a trigger that does not depend on some route's validation gap.
The code carries a comment saying all of this, so the next person does not repeat the round trip.
## 6. `eb('i.status', 'in', [])` would emit invalid SQL
In `itemFilterExpressions`. An empty status list compiles to `where "i"."status" in ()`, which is a Postgres syntax error; the `= ANY($n::text[])` it replaced was valid and matched nothing.
Unreachable today — `parseItemFilters` throws on a list that names nothing, and the storefront's defaults are never empty — which is why it was parked rather than fixed. Worth noting that the obvious guard is wrong: skipping the clause for an empty list makes the filter match *everything*, where the old behaviour matched nothing. If it is worth closing, `eb.val(false)` preserves the old semantics.
## Also worth knowing, not a task
`itemSelect.ts` narrows `items.status` from the generated mirror's `Generated<string>` to `ItemStatus` via `Omit<DB['items'], 'status'> & { status: Generated<ItemStatus> }`. That re-adds the column unconditionally, so a migration that dropped or renamed `items.status` would not be caught by the type — the one drift class the schema mirror otherwise closes. Narrow, and reviewed as sound, but it is the kind of thing worth knowing about before someone touches that column.
Branch fix/307-sonar-cleanup, commit 528e49b. Items 1, 2 and 6 are done; 3 was already closed by #308; 4 and 5 are below and this issue should stay open for them.
Item 1 — the route guard's factory hole. Closed, and demonstrated rather than assumed.routesAreWrapped.test.ts now follows a registration's call to a function declared in the same file and reads that function's body the way it reads a registration. Unwrapping rotationRoute's handler in admin.ts makes the suite fail naming that file, where before it passed. Only same-file functions are followed: express.json(), cookieParser() and uploadsRouter() are imported, are not handler factories, and cannot be resolved from the file being read, so there is a case asserting those are left alone rather than turning one hole into a permanently red test.
Adding the factory reader as a near-copy of registrationAt would have been the wrong thing for a cleanup commit to do, and the copy brought its own cognitive-complexity and loop-counter warnings, so the brace and paren walking is now one function taking the delimiter pair.
Item 2 — the two regex warnings. Fixed.\s* where the generator emits exactly two spaces and one after the colon, and [A-Za-z0-9_] where \w says the same thing. Lint is down from 9 warnings to 7 with none added.
Item 3 — the S2077 hotspots. Already gone, retired by #308. Both call sites build through Kysely now and interpolate nothing; the remaining pool.query calls in those files are all fixed-shape named queries with bound parameters.
Item 6 — in []. Fixed as sql<SqlBool>\false`, preserving what = ANY($n::text[])` did. The obvious guard was wrong in the opposite direction: dropping the clause for an empty list makes the filter match every status rather than none.
Still open
Item 4 — coverage. Not assessable from here. The unit suite alone reports 22.8% statements, but jest.unit.config.js and jest.integration.config.js write to separate directories precisely because SonarQube merges them, so the unit figure on its own means nothing. The real number is on the dashboard, and this SonarQube is 9.9 Community without Bearer auth so the MCP cannot reach it. Worth checking there after this merges.
Item 5 — GET /api/items/:id reading its id with Number(). Unchanged, and it still needs the errorHandling.integration.test.ts trigger moved in the same change, for the reason in the previous comment. The code carries a comment pointing here.
One thing to watch, with no fix attached
During this work a single full unit run reported two failures whose stack traces pointed at mailOutcome.test.ts. It did not reproduce: three subsequent full runs and an isolated run of that file all pass, 468/468. My first guess — that the file leaks environment variables into other files sharing a Jest worker — is wrong; it snapshots process.env and restores it in afterEach. Four unit files do touch MAIL_ALLOWLIST and the SMTP variables (mailOutcome, mailAllowlist, composeEnvironment, envValidation), so a cross-file interaction is still the most plausible shape, but I have no reproduction and did not want to invent a fix for a cause I could not establish. Recording it because it is the same signature as #257 — passes alone, fails in a full run — and a second sighting would be worth more than this one.
Branch `fix/307-sonar-cleanup`, commit `528e49b`. Items 1, 2 and 6 are done; 3 was already closed by #308; 4 and 5 are below and this issue should stay open for them.
**Item 1 — the route guard's factory hole. Closed, and demonstrated rather than assumed.** `routesAreWrapped.test.ts` now follows a registration's call to a function declared in the same file and reads that function's body the way it reads a registration. Unwrapping `rotationRoute`'s handler in `admin.ts` makes the suite fail naming that file, where before it passed. Only same-file functions are followed: `express.json()`, `cookieParser()` and `uploadsRouter()` are imported, are not handler factories, and cannot be resolved from the file being read, so there is a case asserting those are left alone rather than turning one hole into a permanently red test.
Adding the factory reader as a near-copy of `registrationAt` would have been the wrong thing for a cleanup commit to do, and the copy brought its own cognitive-complexity and loop-counter warnings, so the brace and paren walking is now one function taking the delimiter pair.
**Item 2 — the two regex warnings. Fixed.** `\s*` where the generator emits exactly two spaces and one after the colon, and `[A-Za-z0-9_]` where `\w` says the same thing. Lint is down from 9 warnings to 7 with none added.
**Item 3 — the S2077 hotspots. Already gone**, retired by #308. Both call sites build through Kysely now and interpolate nothing; the remaining `pool.query` calls in those files are all fixed-shape named queries with bound parameters.
**Item 6 — `in []`. Fixed** as `sql<SqlBool>\`false\``, preserving what `= ANY($n::text[])` did. The obvious guard was wrong in the opposite direction: dropping the clause for an empty list makes the filter match every status rather than none.
## Still open
**Item 4 — coverage.** Not assessable from here. The unit suite alone reports 22.8% statements, but `jest.unit.config.js` and `jest.integration.config.js` write to separate directories precisely because SonarQube merges them, so the unit figure on its own means nothing. The real number is on the dashboard, and this SonarQube is 9.9 Community without Bearer auth so the MCP cannot reach it. Worth checking there after this merges.
**Item 5 — `GET /api/items/:id` reading its id with `Number()`.** Unchanged, and it still needs the `errorHandling.integration.test.ts` trigger moved in the same change, for the reason in the previous comment. The code carries a comment pointing here.
## One thing to watch, with no fix attached
During this work a single full unit run reported two failures whose stack traces pointed at `mailOutcome.test.ts`. It did not reproduce: three subsequent full runs and an isolated run of that file all pass, 468/468. My first guess — that the file leaks environment variables into other files sharing a Jest worker — is wrong; it snapshots `process.env` and restores it in `afterEach`. Four unit files do touch `MAIL_ALLOWLIST` and the SMTP variables (`mailOutcome`, `mailAllowlist`, `composeEnvironment`, `envValidation`), so a cross-file interaction is still the most plausible shape, but I have no reproduction and did not want to invent a fix for a cause I could not establish. Recording it because it is the same signature as #257 — passes alone, fails in a full run — and a second sighting would be worth more than this one.
Item 5 done in PR #322 (8956b9f). Item 4 is blocked rather than outstanding — the reason is below and it is not a matter of nobody having looked.
Item 5 — the id, and the trigger that held it hostage
GET /api/items/:id reads with readId now, and errorHandling.integration.test.ts no longer needs a route that declines to validate. It fails a database call directly, spying on pool.query against a route that makes one — the failure the error middleware actually exists for. That is the previous comment's own conclusion followed through: moving the trigger to cart.ts would only have moved the wart.
readId did not do what this issue assumed it did
Worth recording, because switching the route over and stopping there would have looked like a fix and closed nothing.
The concern here was that /api/items/5.0, /1e2 and /0x10 resolve to items 5, 100 and 16. readId accepted all three.Number reads each as a positive integer, so every check readId made passed:
Input
Old readId
Now
5.0
5
null
1e2
100
null
0x10
16
null
+5
5
null
Its unit test refused 1.5, -1, 0 and NaN, which look like the same class but are not — those fail Number.isInteger or the > 0 check, and these do not. So the guard read as strict while three of the exact inputs this issue names went straight through it.
An id is a string of digits, so it is matched against digits before parsing. Also 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 readId was written to prevent.
An assertion that was passing by accident
errorHandling's leak test checks the response contains neither syntax nor items. The error it checked against contained both only because of which route happened to be the trigger. The injected failure now contains both words deliberately, and the full message is asserted against as well.
Item 4 — coverage, and why it cannot be answered yet
The SonarQube Scan step has been skipped on every recent run. It is gated on the earlier steps succeeding, and the integration and end-to-end steps have been failing since #56 merged. So the dashboard has not been updated and the duplication question — whether the rotation endpoints' shared handler factory reads as duplication to the scanner — cannot be answered from it.
The previous comment said the real number "is on the dashboard, worth checking there after this merges". It has not been updated since, so that instruction was not followed through so much as prevented.
That answers the coverage half without the dashboard. The duplication half needs one run to go green end to end. #320 fixed the failures that were blocking it, so the next clean run should publish a scan — and ncloc is the number to check first, per the standing note that a green SonarQube job is weak evidence.
Suggestion: keep this issue open for item 4 alone, and close it once a scan actually publishes. It is one check, not a piece of work.
**Item 5 done in PR #322 (`8956b9f`). Item 4 is blocked rather than outstanding** — the reason is below and it is not a matter of nobody having looked.
## Item 5 — the id, and the trigger that held it hostage
`GET /api/items/:id` reads with `readId` now, and `errorHandling.integration.test.ts` no longer needs a route that declines to validate. It fails a database call directly, spying on `pool.query` against a route that makes one — the failure the error middleware actually exists for. That is the previous comment's own conclusion followed through: moving the trigger to `cart.ts` would only have moved the wart.
## `readId` did not do what this issue assumed it did
Worth recording, because switching the route over and stopping there would have looked like a fix and closed nothing.
The concern here was that `/api/items/5.0`, `/1e2` and `/0x10` resolve to items 5, 100 and 16. **`readId` accepted all three.** `Number` reads each as a positive integer, so every check `readId` made passed:
| Input | Old `readId` | Now |
| --- | --- | --- |
| `5.0` | **5** | `null` |
| `1e2` | **100** | `null` |
| `0x10` | **16** | `null` |
| `+5` | **5** | `null` |
Its unit test refused `1.5`, `-1`, `0` and `NaN`, which look like the same class but are not — those fail `Number.isInteger` or the `> 0` check, and these do not. So the guard read as strict while three of the exact inputs this issue names went straight through it.
An id is a string of digits, so it is matched against digits before parsing. Also 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 `readId` was written to prevent.
## An assertion that was passing by accident
`errorHandling`'s leak test checks the response contains neither `syntax` nor `items`. The error it checked against contained both **only because of which route happened to be the trigger**. The injected failure now contains both words deliberately, and the full message is asserted against as well.
## Item 4 — coverage, and why it cannot be answered yet
**The SonarQube Scan step has been skipped on every recent run.** It is gated on the earlier steps succeeding, and the integration and end-to-end steps have been failing since #56 merged. So the dashboard has not been updated and the duplication question — whether the rotation endpoints' shared handler factory reads as duplication to the scanner — cannot be answered from it.
The previous comment said the real number "is on the dashboard, worth checking there after this merges". It has not been updated since, so that instruction was not followed through so much as prevented.
What the test steps reported directly 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% |
That answers the coverage half without the dashboard. The duplication half needs one run to go green end to end. #320 fixed the failures that were blocking it, so the next clean run should publish a scan — and `ncloc` is the number to check first, per the standing note that a green SonarQube job is weak evidence.
**Suggestion:** keep this issue open for item 4 alone, and close it once a scan actually publishes. It is one check, not a piece of work.
Closing. Items 1, 2, 3, 5 and 6 are done and merged. Item 4 is a single check that cannot be performed yet, and is recorded below rather than left as an open issue for it.
GET /api/items/:id reads with readId, and errorHandling.integration.test.ts no longer depends on a route declining to validate — it fails a pool.query call directly. Verified: the integration step passed in run 893.
Two things found along the way that the issue asked about but that switching to readId would not have delivered:
readId accepted 5.0, 1e2, 0x10 and +5 — the exact inputs this issue named. Number reads each as a positive integer, so every check it made passed and /items/5.0 fetched item 5. Never an error, so it never announced itself. Now matched against digits before parsing.
readId is 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 it was written to prevent.
A follow-on fix (#323) was needed because the new trigger did not typecheck: pg overloads query, so jest.spyOn inferred the rejection value as never. That surfaced as a suite that failed to run while the summary line read 446 passed, 0 failed — the step red with nothing reported failing.
The reason it reached main is worth keeping: the backend has a tsconfig.test.json that nothing was running.build compiles src alone. It is now npm run typecheck:tests and reproduces that class of failure in seconds without a database — which matters, because "the integration suite needs Postgres" had become the standing explanation for regressions that were in fact findable locally all along.
Item 4 — coverage, and why it stays unanswered
The SonarQube Scan step has not completed on any recent run. It is gated on the earlier steps succeeding, and those have failed since #56 merged — most recently in run 893, where the job was killed mid-run and every remaining step failed with it.
So the dashboard is still stale, and the duplication question — whether the rotation endpoints' shared handler factory reads as duplication to the scanner — cannot be answered from it.
The coverage half is answerable without the dashboard, from the test steps themselves (run 875):
Statements
Branches
Functions
Lines
Backend
75.94%
59.37%
74.42%
77.47%
Frontend
77.59%
69.70%
72.68%
78.55%
What remains is one look at the dashboard after the next green run, checking ncloc first — a green SonarQube job is weak evidence, since it exits success on a partially-failed analysis. That is a check, not work, and does not need an issue held open for it. If the scan shows something worth acting on, that is its own issue.
Closing. Items 1, 2, 3, 5 and 6 are done and merged. Item 4 is a single check that cannot be performed yet, and is recorded below rather than left as an open issue for it.
## Item 5 — done (#322, #323)
`GET /api/items/:id` reads with `readId`, and `errorHandling.integration.test.ts` no longer depends on a route declining to validate — it fails a `pool.query` call directly. **Verified: the integration step passed in [run 893](https://gitea.bermudalamb.synology.me/bermudalamb/redefined-designs/actions/runs/893).**
Two things found along the way that the issue asked about but that switching to `readId` would not have delivered:
- **`readId` accepted `5.0`, `1e2`, `0x10` and `+5`** — the exact inputs this issue named. `Number` reads each as a positive integer, so every check it made passed and `/items/5.0` fetched item 5. Never an error, so it never announced itself. Now matched against digits before parsing.
- **`readId` is 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 it was written to prevent.
A follow-on fix (#323) was needed because the new trigger did not typecheck: `pg` overloads `query`, so `jest.spyOn` inferred the rejection value as `never`. That surfaced as a **suite that failed to run** while the summary line read `446 passed, 0 failed` — the step red with nothing reported failing.
The reason it reached `main` is worth keeping: **the backend has a `tsconfig.test.json` that nothing was running.** `build` compiles `src` alone. It is now `npm run typecheck:tests` and reproduces that class of failure in seconds without a database — which matters, because "the integration suite needs Postgres" had become the standing explanation for regressions that were in fact findable locally all along.
## Item 4 — coverage, and why it stays unanswered
The **SonarQube Scan step has not completed on any recent run.** It is gated on the earlier steps succeeding, and those have failed since #56 merged — most recently in run 893, where the job was killed mid-run and every remaining step failed with it.
So the dashboard is still stale, and the duplication question — whether the rotation endpoints' shared handler factory reads as duplication to the scanner — cannot be answered from it.
The coverage half is answerable without the dashboard, from the test steps themselves ([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% |
**What remains is one look at the dashboard after the next green run**, checking `ncloc` first — a green SonarQube job is weak evidence, since it exits success on a partially-failed analysis. That is a check, not work, and does not need an issue held open for it. If the scan shows something worth acting on, that is its own issue.
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.
The standing follow-up after a run of feature work, kept as its own issue and its own PR rather than folded into a feature branch. Four items, none of them failing anything today.
1.
routesAreWrapped.test.tscannot see a handler factoryFrom #301.
POST /items/:id/images/:imageId/rotate-leftandrotate-rightare both correctly wrapped inasyncRoute, but they are built by arotationRoute(direction)factory, and the guard only inspects the text inside arouter.post(...)call. There is noasynctoken at those two registrations, so the test passed by finding nothing rather than by finding the wrapper.This is the first handler factory in
src/routes/, and the guard exists precisely because the convention was half-forgotten once already. It should either understand a factory that returnsasyncRoute(...), or fail loudly on a registration whose handler it cannot resolve — the second is probably easier to get right and harder to fool.2. Two new SonarJS warnings in the schema-drift test
From #305, in code that change introduced, at
backend/tests/integration/schemaMirror.integration.test.ts:33:sonarjs/super-linear-regex— the table/interface regex backtrackssonarjs/concise-regex—[A-Za-z0-9_]should be\wBoth are advisory and
npm run lintstill reports 0 errors, which is why they were parked rather than fixed in the branch. The regex is parsing a generated file of known shape, so the backtracking has no adversarial input — but it is new debt in new code and belongs here.3. The S2077 hotspots are still open, and are now the only ones left
backend/src/routes/admin.ts:146andbackend/src/routes/items.ts:95still interpolatebuildItemFilterSql's composedwhereinto a query string. #294 removed the other seven sites by making each fixed-shape query a named constant; these two genuinely build SQL at run time and are the reason #216 wanted a builder at all.They are safe, and the argument is written out at
admin.ts:132-143. What #305 changed is only that the tool for retiring the argument is now Kysely rather than Drizzle — the conversion itself is the separate issue filed alongside this one, so what belongs here is deciding what to do with the hotspots in the meantime: leave them reviewed and watched, or mark them with the specific reasoning so the next scan does not re-raise them cold.4. Coverage and duplication
The usual pass. Worth checking whether the new
src/imageRotation.tsand the reconvertedadminCategories.tsmoved the numbers, and whether the two rotation endpoints' shared handler factory reads as duplication to the scanner.Not in scope
Converting any query to Kysely. That is its own issue.
Refs #301, #305, #294, #297
Two more for this issue, both from #308.
5.
GET /api/items/:idreads its id withNumber()rather thanreadId#207 standardised every other id-taking route on
readId, which parses strictly and answers 404 for anything unreadable. This one route does not, and after #308 converted it to a real integer comparison that matters slightly more than it used to:/api/items/5.0,/1e2and/0x10now resolve to items 5, 100 and 16, where the previous string comparison had Postgres reject them.It was changed to
readIdduring #308 and then deliberately reverted, and the reason is the thing that makes this worth its own change rather than a one-liner.errorHandling.integration.test.tsdrives this exact route with a non-numeric id — not to assert what the route does with a bad id, but to prove thatasyncRouteplus the error middleware answer 500 rather than leaving the request hanging. Its own comment says a hung request "renders as an empty storefront rather than a visible failure". A stricter parse here removes that test's mechanism, so it would go on passing while no longer testing anything.So switching this over means giving that test another trigger in the same change.
cart.tsstill takesreq.params.itemIdas a raw string on a public route and would serve, though that only moves the wart — the more durable answer is a trigger that does not depend on some route's validation gap.The code carries a comment saying all of this, so the next person does not repeat the round trip.
6.
eb('i.status', 'in', [])would emit invalid SQLIn
itemFilterExpressions. An empty status list compiles towhere "i"."status" in (), which is a Postgres syntax error; the= ANY($n::text[])it replaced was valid and matched nothing.Unreachable today —
parseItemFiltersthrows on a list that names nothing, and the storefront's defaults are never empty — which is why it was parked rather than fixed. Worth noting that the obvious guard is wrong: skipping the clause for an empty list makes the filter match everything, where the old behaviour matched nothing. If it is worth closing,eb.val(false)preserves the old semantics.Also worth knowing, not a task
itemSelect.tsnarrowsitems.statusfrom the generated mirror'sGenerated<string>toItemStatusviaOmit<DB['items'], 'status'> & { status: Generated<ItemStatus> }. That re-adds the column unconditionally, so a migration that dropped or renameditems.statuswould not be caught by the type — the one drift class the schema mirror otherwise closes. Narrow, and reviewed as sound, but it is the kind of thing worth knowing about before someone touches that column.Branch
fix/307-sonar-cleanup, commit528e49b. Items 1, 2 and 6 are done; 3 was already closed by #308; 4 and 5 are below and this issue should stay open for them.Item 1 — the route guard's factory hole. Closed, and demonstrated rather than assumed.
routesAreWrapped.test.tsnow follows a registration's call to a function declared in the same file and reads that function's body the way it reads a registration. UnwrappingrotationRoute's handler inadmin.tsmakes the suite fail naming that file, where before it passed. Only same-file functions are followed:express.json(),cookieParser()anduploadsRouter()are imported, are not handler factories, and cannot be resolved from the file being read, so there is a case asserting those are left alone rather than turning one hole into a permanently red test.Adding the factory reader as a near-copy of
registrationAtwould have been the wrong thing for a cleanup commit to do, and the copy brought its own cognitive-complexity and loop-counter warnings, so the brace and paren walking is now one function taking the delimiter pair.Item 2 — the two regex warnings. Fixed.
\s*where the generator emits exactly two spaces and one after the colon, and[A-Za-z0-9_]where\wsays the same thing. Lint is down from 9 warnings to 7 with none added.Item 3 — the S2077 hotspots. Already gone, retired by #308. Both call sites build through Kysely now and interpolate nothing; the remaining
pool.querycalls in those files are all fixed-shape named queries with bound parameters.Item 6 —
in []. Fixed assql<SqlBool>\false`, preserving what= ANY($n::text[])` did. The obvious guard was wrong in the opposite direction: dropping the clause for an empty list makes the filter match every status rather than none.Still open
Item 4 — coverage. Not assessable from here. The unit suite alone reports 22.8% statements, but
jest.unit.config.jsandjest.integration.config.jswrite to separate directories precisely because SonarQube merges them, so the unit figure on its own means nothing. The real number is on the dashboard, and this SonarQube is 9.9 Community without Bearer auth so the MCP cannot reach it. Worth checking there after this merges.Item 5 —
GET /api/items/:idreading its id withNumber(). Unchanged, and it still needs theerrorHandling.integration.test.tstrigger moved in the same change, for the reason in the previous comment. The code carries a comment pointing here.One thing to watch, with no fix attached
During this work a single full unit run reported two failures whose stack traces pointed at
mailOutcome.test.ts. It did not reproduce: three subsequent full runs and an isolated run of that file all pass, 468/468. My first guess — that the file leaks environment variables into other files sharing a Jest worker — is wrong; it snapshotsprocess.envand restores it inafterEach. Four unit files do touchMAIL_ALLOWLISTand the SMTP variables (mailOutcome,mailAllowlist,composeEnvironment,envValidation), so a cross-file interaction is still the most plausible shape, but I have no reproduction and did not want to invent a fix for a cause I could not establish. Recording it because it is the same signature as #257 — passes alone, fails in a full run — and a second sighting would be worth more than this one.Item 5 done in PR #322 (
8956b9f). Item 4 is blocked rather than outstanding — the reason is below and it is not a matter of nobody having looked.Item 5 — the id, and the trigger that held it hostage
GET /api/items/:idreads withreadIdnow, anderrorHandling.integration.test.tsno longer needs a route that declines to validate. It fails a database call directly, spying onpool.queryagainst a route that makes one — the failure the error middleware actually exists for. That is the previous comment's own conclusion followed through: moving the trigger tocart.tswould only have moved the wart.readIddid not do what this issue assumed it didWorth recording, because switching the route over and stopping there would have looked like a fix and closed nothing.
The concern here was that
/api/items/5.0,/1e2and/0x10resolve to items 5, 100 and 16.readIdaccepted all three.Numberreads each as a positive integer, so every checkreadIdmade passed:readId5.0null1e2null0x10null+5nullIts unit test refused
1.5,-1,0andNaN, which look like the same class but are not — those failNumber.isIntegeror the> 0check, and these do not. So the guard read as strict while three of the exact inputs this issue names went straight through it.An id is a string of digits, so it is matched against digits before parsing. Also 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
readIdwas written to prevent.An assertion that was passing by accident
errorHandling's leak test checks the response contains neithersyntaxnoritems. The error it checked against contained both only because of which route happened to be the trigger. The injected failure now contains both words deliberately, and the full message is asserted against as well.Item 4 — coverage, and why it cannot be answered yet
The SonarQube Scan step has been skipped on every recent run. It is gated on the earlier steps succeeding, and the integration and end-to-end steps have been failing since #56 merged. So the dashboard has not been updated and the duplication question — whether the rotation endpoints' shared handler factory reads as duplication to the scanner — cannot be answered from it.
The previous comment said the real number "is on the dashboard, worth checking there after this merges". It has not been updated since, so that instruction was not followed through so much as prevented.
What the test steps reported directly on run 875:
That answers the coverage half without the dashboard. The duplication half needs one run to go green end to end. #320 fixed the failures that were blocking it, so the next clean run should publish a scan — and
nclocis the number to check first, per the standing note that a green SonarQube job is weak evidence.Suggestion: keep this issue open for item 4 alone, and close it once a scan actually publishes. It is one check, not a piece of work.
Closing. Items 1, 2, 3, 5 and 6 are done and merged. Item 4 is a single check that cannot be performed yet, and is recorded below rather than left as an open issue for it.
Item 5 — done (#322, #323)
GET /api/items/:idreads withreadId, anderrorHandling.integration.test.tsno longer depends on a route declining to validate — it fails apool.querycall directly. Verified: the integration step passed in run 893.Two things found along the way that the issue asked about but that switching to
readIdwould not have delivered:readIdaccepted5.0,1e2,0x10and+5— the exact inputs this issue named.Numberreads each as a positive integer, so every check it made passed and/items/5.0fetched item 5. Never an error, so it never announced itself. Now matched against digits before parsing.readIdis 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 it was written to prevent.A follow-on fix (#323) was needed because the new trigger did not typecheck:
pgoverloadsquery, sojest.spyOninferred the rejection value asnever. That surfaced as a suite that failed to run while the summary line read446 passed, 0 failed— the step red with nothing reported failing.The reason it reached
mainis worth keeping: the backend has atsconfig.test.jsonthat nothing was running.buildcompilessrcalone. It is nownpm run typecheck:testsand reproduces that class of failure in seconds without a database — which matters, because "the integration suite needs Postgres" had become the standing explanation for regressions that were in fact findable locally all along.Item 4 — coverage, and why it stays unanswered
The SonarQube Scan step has not completed on any recent run. It is gated on the earlier steps succeeding, and those have failed since #56 merged — most recently in run 893, where the job was killed mid-run and every remaining step failed with it.
So the dashboard is still stale, and the duplication question — whether the rotation endpoints' shared handler factory reads as duplication to the scanner — cannot be answered from it.
The coverage half is answerable without the dashboard, from the test steps themselves (run 875):
What remains is one look at the dashboard after the next green run, checking
nclocfirst — a green SonarQube job is weak evidence, since it exits success on a partially-failed analysis. That is a check, not work, and does not need an issue held open for it. If the scan shows something worth acting on, that is its own issue.