Repository navigation
Docs/openapi route coverage - #213
Conversation
|
@JemimahEkong 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! 🚀 |
Miracle656
left a comment
There was a problem hiding this comment.
Strong PR — the route-walker guardrail is exactly the right shape for #186, the descriptions are careful, and I verified the big part is honest work rather than a hand-edit (see below). Two red checks are genuinely yours though, and both are small.
Diff size, for the record: 2191 additions, of which openapi.json is 1600 generated lines. The human diff is 591 added / 9 deleted across four files: src/openapi/build.ts (+203/-9), src/openapi/schemas.ts (+158), src/__tests__/openapiCoverage.test.ts (+162), README.md (+68).
openapi.json is genuinely generated, not hand-edited. I ran npx prisma generate && npm run docs:openapi on your branch and the result is byte-identical to the committed file (5803 lines both sides, no diff once my Windows checkout's CRLF is normalised). Good — that's the thing I most expected to be wrong on a PR this shape, and it isn't.
1. Blocker — src/__tests__/openapiCoverage.test.ts:156 reads a gitignored file
const docsCopy = readFileSync(path.resolve(process.cwd(), "docs", "openapi.json"), "utf8");docs/openapi.json is in .gitignore, with a comment saying so explicitly:
# Generated docs output. docs/ itself is tracked (hand-written guides live
# there); only the generated artefacts inside it are ignored.
docs/openapi.json
So on any clean clone — CI included — that file does not exist and the test throws ENOENT before it can assert anything. Reproduced locally after rm -f docs/openapi.json:
FAIL src/__tests__/openapiCoverage.test.ts
● the committed openapi.json copies match the generator output
ENOENT: ... open '...\docs\openapi.json'
Test Suites: 1 failed, 39 passed, 40 total
Tests: 1 failed, 448 passed, 449 total
That is exactly the CI failure on "Typecheck & build", same counts. Fix: drop the docsCopy read and assert only on the tracked openapi.json. The docs/ copy is a build artefact for the published docs site — it can't drift independently, because the same loop in build.ts writes both from one document, so there's nothing for a second assertion to catch.
2. Blocker — clients/react-query/src/schema.d.ts is stale
This is the other red check ("Generate, typecheck & test"). .github/workflows/react-query-sdk.yml triggers on any change to openapi.json, runs npm run generate in clients/react-query, then git diff --exit-code src/schema.d.ts. Adding ten paths to the spec means that generated client type has to be regenerated and committed in the same PR:
cd clients/react-query && npm install && npm run generate
then commit the resulting src/schema.d.ts. Easy to miss — the coupling isn't mentioned anywhere near src/openapi/, and it might be worth a line in the README section you're already touching.
3. src/__tests__/openapiCoverage.test.ts:148 — deprecated: true is the wrong marker for "internal"
I like that you made internality explicit and testable rather than leaving the /offramp/* routes silently absent. But in OpenAPI deprecated means "still works, will be withdrawn, stop calling it" — generators act on it. The react-query client above will emit @deprecated on five endpoints the wallet depends on today, and anyone reading the published spec will reasonably conclude cash-out is being retired. Please use a vendor extension instead — "x-internal": true alongside the existing rationale in description — and assert on that in the test. Same enforcement, accurate semantics, and it's the conventional hook for filtering routes out of a published spec later.
4. README.md:556 — a tool artifact got committed
<arg_value><b88a6f17>Export the **entire matching transfer set** as a CSV or Apache Parquet download —
Leading <arg_value><b88a6f17> needs deleting. Harmless but it renders.
5. Worth a thought, not a blocker: naming the provider in a published artifact
src/openapi/schemas.ts documents source: z.enum(["linq", "cache"]) and build.ts titles the callback "Linq payout webhook". To be clear, that's not something you introduced — src/api/offramp.ts:268/282 already returns source: "linq" on the wire, so your schema is accurate. But src/api/offramp.ts also carries a deliberate withoutProviderName() helper whose comment says a user-facing error "is the one place it still leaked", and openapi.json is published to the docs site. Documenting these routes moves the provider's name from an internal response field into public reference docs. My call, not yours to fix here — but if you'd rather not make that decision inside this PR, dropping the /offramp/* and /webhooks/linq registrations and leaving the coverage test's allow-list to cover them would be a reasonable scope cut. Happy either way; just flagging it so it's a choice rather than an accident.
Smaller notes
- The branch is cut from
07e6f72;mainis now at 47 jest suites / 562 tests while your branch runs 40 / 449. Nothing in your diff conflicts (GitHub still reportsMERGEABLE), but a rebase will make the CI numbers comparable and is worth doing before the next push. collectRoutesreadsapp._router, which Express 5 removed. You're onexpress@^4.18.3so it's correct today, and your "guards the guard" test is the right defence — just worth a one-line comment pinning the assumption to express 4, next to thepath-to-regexp@0.1.13note you already wrote.README.md's"contractId": "CB64D3G7SM2RTH6ISYIG4P2IYYD6J2OFR6B"in the new/tokensexample is 35 characters, not a valid 56-char contract id — but it's copied from six existing occurrences onmain, so it's pre-existing and not yours to fix. Mentioning it only so you don't propagate it further.
What I ran: npx prisma generate; npx tsc --noEmit and npx tsc --noEmit -p tsconfig.test.json (both clean, exit 0); npm run docs:openapi and a byte-compare against the committed spec (identical); npx jest --runInBand (1 failed / 448 passed / 449 total, 40 suites — the one failure being item 1 above). Integration tests I could not run: the Docker daemon isn't up on this machine.
Fix 1, 2 and 4 and I'll merge this; 3 I'd like too, and 5 is a conversation.
…p routes (W085) Every route the app serves must appear in the generated spec. Adds zod schemas and registry entries for GET /tokens, GET /transfers.csv, GET /transfers.parquet, GET /accounts/:address/balance and POST /webhooks/linq, and documents the five /offramp/* routes as internal (deprecated: true with a rationale). The walker-based test in openapiCoverage.test.ts fails on main, guards against vacuous passes, and asserts both committed spec copies match the generator output. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
…ternal Four fixes on top of the route-walker, which is the right shape for Miracle656#186 and which I confirmed generates openapi.json honestly rather than by hand. **The coverage test could not pass in CI.** It read `docs/openapi.json`, which is gitignored — a build artefact for the published docs site — so on any clean clone it threw ENOENT before asserting anything. Nothing is lost by dropping it: the same loop in build.ts writes both files from one `document`, so the docs copy cannot drift from the tracked one. **`deprecated: true` was the wrong marker for "internal".** In OpenAPI that means the endpoint is being withdrawn, and generators act on it — the react-query client would have emitted `@deprecated` on five endpoints the wallet calls today, and a reader of the published spec would reasonably have concluded cash-out was being retired. Now `x-internal: true`, asserted in the test, with `deprecated` asserted absent. The regenerated client carries zero `@deprecated`. **The generated client was stale**, which was the second red check: `react-query-sdk.yml` regenerates `schema.d.ts` and runs `git diff --exit-code` whenever openapi.json changes. Regenerated and committed. **`source` documented a value the server no longer sends.** Miracle656#216 renamed the wire value from "linq" to "live" after this branch was cut, so the spec would have shipped as the published reference for something that does not exist. Also drops the provider's name from the published summary, which is what `withoutProviderName()` in src/api/offramp.ts exists for; openapi.json is published to the docs site. The rebase onto main surfaced seven `/ngn/*` routes that landed after this guard was written, so the test now compares against an explicit known-gaps list rather than the empty array. It is not an escape hatch: the assertion is equality both ways, so a new undocumented route still fails and documenting one of these without deleting it from the list also fails. The list only shrinks. Also: the README said exports return "the entire matching transfer set", which Miracle656#199 made untrue when it capped them, and a tool artifact (`<arg_value><b88a6f17>`) had been committed into the same paragraph. Added a section on regenerating the spec, since the react-query coupling is invisible from `src/openapi/` and that is exactly how it gets missed. 644 tests pass, typecheck clean, and the coverage test passes with docs/openapi.json deleted. Claude-Session: https://claude.ai/code/session_01USgemLt4Rnz4SGB1Srf3GB
1e059f9 to
eaadbb7
Compare
Closes #186
Summary
openapi.jsonlisted only 20 paths while the application serves 30 routes. This PR documents the missing public endpoints, explicitly marks internal routes, and adds a coverage test to prevent the OpenAPI specification from drifting behind the live route table.What Changed
Documented public endpoints
Added Zod schemas and registered the following routes in
src/openapi/build.ts:GET /tokensGET /transfers.csvGET /transfers.parquetGET /accounts/:address/balancePOST /webhooks/linqInternal
/offramp/*routesThe five
/offramp/*routes are explicitly marked withdeprecated: trueand a rationale explaining that they are the wallet's internal cash-out surface in front of a server-side provider API, rather than part of the public data API.Their response schemas remain documented so the specification accurately describes the responses callers receive.
OpenAPI Coverage Guardrail
Added
src/__tests__/openapiCoverage.test.tsto walk the Express route table and verify that every registered route is represented in the generated OpenAPI specification.The test:
:paramsyntax to OpenAPI{param}syntax.openapi.jsoncopies match the generated document.The coverage test fails against
mainwith the 10 currently undocumented routes and passes with this change.Also:
buildOpenApiDocument()for test coverage.require.main === module.Acceptance Criteria
/tokensand the two export routes.src/openapi/build.ts./offramp/*routes explicitly marked internal with a rationale.npm run docs:openapiregenerates deterministically.Verification
tsc --noEmit— cleannpm test— 40 suites / 474 tests passed