Skip to content

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

Open
DevScoopee wants to merge 8 commits into
Divinity-Labs:mainfrom
DevScoopee:Errors-are-prose,-so-clients-match-on-sentences-

Conversation

@DevScoopee

Copy link
Copy Markdown
Contributor

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

@drips-wave

drips-wave Bot commented Oct 17, 2026

Copy link
Copy Markdown

@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! 🚀

Learn more about application limits

DevScoopee and others added 7 commits October 17, 2026 09:26
… 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 Miracle656 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 no ADMIN_TOKEN answers 503 admin_not_configured rather than telling an operator their correct token is invalid. Your copy returns false, so /admin/usage would answer 401 while /admin/keys answers 503 — the exact confusion that issue was about.
  • #574 added a structural onRequest hook 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:

  1. Error codes only; pagination in its own PR.
  2. Leave isAdminAuthorized/checkAdminAuth where it is and import it. If the goal was to decouple usage.ts from auth.ts, the honest version is to pass the check in, not to copy it.
  3. npx tsc --noEmit and npm test green locally.
  4. openapi.yaml is the source of truth and openapi.json is generated — run npm run openapi:gen and npx --yes openapi-typescript@7.13.0 openapi.yaml -o clients/typescript/src/schema.d.ts rather than editing the JSON. Both are CI-guarded byte comparisons.
  5. Worth a look at src/errors.ts as it stands on main now: it already owns INTERNAL_ERROR_CODE, correlation IDs and createInternalErrorBody, and docs/http-status-codes.md plus tests/httpStatusCodes.test.ts document 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Errors are prose, so clients match on sentences

2 participants