Repository navigation
Add a single enumerated error code set with a shared apiError helper,attach code to every { error } response, document the codes in OpenAPI, regenerate the spec, and add a guard test that all emitted codes stay in the set. - #437
DevScoopee wants to merge 8 commits into
Conversation
|
@DevScoopee 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! 🚀 |
… attach code to every { error } response, document the codes in OpenAPI, regenerate the spec, and add a guard test that all emitted codes stay in the set.
src/errors.ts defines ERROR_CODES, ErrorCode, and apiError(code, message) preserving existing error text. Routes, API handlers, middleware (network/x402), and the rate limiter now include the appropriate code. openapi.yaml adds ErrorCode enum and Error schema (PaymentRequired/TooManyRequests include code), openapi.json is regenerated. src/__tests__/errorCodes.test.ts guards that the OpenAPI enum matches ERROR_CODES and only enumerated codes are emitted.
Closes Divinity-Labs#261
Miracle656
left a comment
There was a problem hiding this comment.
The goal is right — "clients match on sentences" is a real problem and an enumerated code set with one apiError helper is the right answer. But this cannot go in as it stands, for three separate reasons.
1. It bundles two unrelated features
.changeset/api-cursor-pagination.md 22 +++
src/pagination.ts 116 +++
src/__tests__/pagination.test.ts 268 +++
Cursor pagination is not an error-code set. It is a change to the shape of every collection response, it needs its own openapi work, and it deserves its own review — bundled here it will be approved or rejected on the strength of the other half. Please split it out; I will review it on its own and it will land faster.
That leaves the error-code change touching ~25 files, which is a reasonable size for what it does.
2. It forks isAdminAuthorized into a second copy (blocking)
src/api/usage.ts now carries its own admin check:
function isAdminAuthorized(req: FastifyRequest): boolean {
const adminToken = process.env.ADMIN_TOKEN
if (!adminToken) return false
…
return a.length === b.length && require('crypto').timingSafeEqual(a, b)
}while the diff removes 64 lines from src/api/auth.ts. The doc comment on the original says why there should only ever be one:
This is the single implementation used by every admin route (keys and usage), so the two surfaces cannot drift apart.
They have now drifted, immediately and in a way that matters. Two PRs merged today changed what that check has to do:
- #566 replaced it with three-valued
checkAdminAuth, so a deployment with noADMIN_TOKENanswers 503admin_not_configuredrather than telling an operator their correct token is invalid. Your copy returnsfalse, so/admin/usagewould answer 401 while/admin/keysanswers 503 — the exact confusion that issue was about. - #574 added a structural
onRequesthook over everything under/admin/, so the per-handler check is now belt-and-braces rather than the gate. A second, weaker implementation sitting behind the hook is dead code that looks load-bearing.
Two smaller problems in the copy itself: FastifyRequest is used without being imported, and require('crypto') is a CommonJS call in a module the rest of the file treats as ESM. Both suggest it was moved rather than written.
Whatever else changes, isAdminAuthorized / checkAdminAuth must stay in src/api/auth.ts with one caller-visible behaviour.
3. The merge against main silently produces code that does not compile
git merge origin/main reports one conflict (src/app.ts) and then hands back a broken tree. Git merges line regions, not declarations, so where both sides edited the same function it kept both. In src/api/usage.ts:
app.get(
'/usage/me',
async (req, reply) => {
…
return reply.send(summary)
}
// The key presented determines the answer — never a parameter.
const summary = await getUsageSummary(apiKey.id)
return reply.send(summary)
}
app.get('/usage/me', handleCallerUsage)
app.get('/usage', handleCallerUsage)Your inline handler and main's extracted handleCallerUsage are both present, so the function is never closed. src/routes/basket.ts has the same shape. tsc stops reporting after a few syntax errors, so there are likely more behind those two.
This is the second PR today to merge "cleanly" into something that will not compile. The habit that catches it: after any merge or rebase, run npx tsc --noEmit before anything else. A clean merge is not evidence of a working tree.
What I would merge
Rebase on main — not merge — so you resolve against the current shape of each file rather than against a three-way result that kept both. Then:
- Error codes only; pagination in its own PR.
- Leave
isAdminAuthorized/checkAdminAuthwhere it is and import it. If the goal was to decoupleusage.tsfromauth.ts, the honest version is to pass the check in, not to copy it. npx tsc --noEmitandnpm testgreen locally.openapi.yamlis the source of truth andopenapi.jsonis generated — runnpm run openapi:genandnpx --yes openapi-typescript@7.13.0 openapi.yaml -o clients/typescript/src/schema.d.tsrather than editing the JSON. Both are CI-guarded byte comparisons.- Worth a look at
src/errors.tsas it stands onmainnow: it already ownsINTERNAL_ERROR_CODE, correlation IDs andcreateInternalErrorBody, anddocs/http-status-codes.mdplustests/httpStatusCodes.test.tsdocument the status contract. Your enumerated set should extend that rather than sit beside it — and a test asserting every code in the enum appears in the docs table would keep the two honest.
The core idea is worth having and I would like it in. Ping me when it is split and rebased.
src/errors.ts defines ERROR_CODES, ErrorCode, and apiError(code, message) preserving existing error text. Routes, API handlers, middleware (network/x402), and the rate limiter now include the appropriate code. openapi.yaml adds ErrorCode enum and Error schema (PaymentRequired/TooManyRequests include code), openapi.json is regenerated. src/tests/errorCodes.test.ts guards that the OpenAPI enum matches ERROR_CODES and only enumerated codes are emitted.
Closes #261