refactor(drawbridge): extract the v1 router so it can be tested - #819
Merged
Conversation
drawbridge-api.ts built its Express app inside main(), which the module calls at import time — so importing it booted the server, connected to Gatekeeper and Redis, and bound a port. That left its 28 v1 routes untestable and uncounted, the same shape #705/#706 fixed for gatekeeper and keymaster. Extract createV1Router() into v1-router.ts, with createRequireAdminKey() in v1-admin.ts and the options interface in v1-router-types.ts, mirroring the gatekeeper layout. drawbridge-api.ts now builds its dependencies and mounts the router. Behaviour-preserving: the route block was moved programmatically rather than retyped, and diffed against the original. The only change is serviceVersion -> getServiceVersion(), which is required — serviceVersion is a module-scope `let` reassigned after an async package.json read, so capturing it by value at factory time would pin /version to "unknown" forever. Verified: the route set is identical (28 before, 28 after) and `tsc -p tsconfig.json` is clean for the service. ARCHON_ADMIN_HEADER is used both by the admin middleware and by the herald proxy for upstream auth, so it is exported from v1-admin.ts rather than duplicated. Tests: 18 for the extracted router (capabilities derivation, DIDComm endpoint including the null case, admin gating across 401/403/pass, resolve-option passthrough, numeric-vs-hash blockId parsing, a table-driven 502 check across every proxy route, mid-stream read failure, and the lightning proxy's 501/200/502 paths), plus 11 for lightning-mediator-client, which the router test transitively pulled into the coverage denominator at 5.9%. Three one-line L402 delegation lines are deliberately left uncovered: driving those routes with a stub options object leaves one handler never responding, which hangs Jest (CI runs without --forceExit). Their handlers are already at 100% via l402-auth.test.ts. A comment in the test records this. drawbridge/server: 6 files instrumented -> 10 at 99.0% (509/514). Repo total 96.47% -> 96.52%, despite ~200 lines entering the denominator. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
macterra
enabled auto-merge (squash)
August 1, 2026 18:27
macterra
added a commit
that referenced
this pull request
Aug 1, 2026
index.ts built its Express app at module scope and called app.listen() at import time, so importing it booted the server. Its 34 routes were untestable and uncounted — the last instance of the pattern #705, #706 and #819 fixed for the other services. Split into three files: - config.ts holds the environment-derived constants. Extracting these first meant the 38 constant references in the moved code needed no rewriting at all. - routes.ts exports createHeraldRoutes(ctx), returning the router and startDmailPollLoop (the only symbol the bootstrap still needs). - index.ts (1,490 -> ~210 lines) owns bootstrap only: app creation, session setup, service identity, database and keymaster wiring, listen. keymaster, db, emailBridge and serviceDID are assigned after the routes register, so they are read lazily through a HeraldContext object the bootstrap populates. Behaviour-preserving, verified rather than assumed: the block was moved programmatically and diffed against the original with the same transformations applied — it matches exactly, modulo indentation from eslint --fix. The route set is identical (34 before, 34 after) and the service's own `tsc -p tsconfig.json` is clean. Two hazards worth recording. A naive \bkeymaster\b rewrite corrupted import specifiers ('@didcid/keymaster' -> '@didcid/ctx.keymaster'), so string literals are stashed before the identifier rewrite. And the object shorthand `serviceDID,` became invalid syntax as `ctx.serviceDID,`; tsc caught it, which is why the diff alone is not sufficient verification. multer is declared in root devDependencies at herald's own ^2.2.0, so routes.ts resolves in CI — services/* are not workspaces, so `npm ci` never installs their dependencies. 42 tests cover the public endpoints, auth and owner gates, name validation and the full name lifecycle (assign, credential issue vs update, delete with revocation), webfinger, session login, check-auth, profile and credential. routes.ts reaches 45%. Repo total 96.52% -> 92.91%: 632 previously-invisible lines entered the denominator and 297 of them are now covered. The remaining handlers (SendGrid inbound-email, IPNS publishing, the dmail poll loop, LNURLp callback) need real fixture work and follow separately. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
drawbridge-api.tsbuilt its Express app insidemain(), which the module calls at import time. Importing it booted the server — connecting to Gatekeeper and Redis and binding a port — so its 28 v1 routes were untestable and uncounted. Same shape #705/#706 fixed for gatekeeper and keymaster.What
v1-router.tsexportingcreateV1Router(options)v1-admin.ts(createRequireAdminKey) andv1-router-types.ts, mirroring the gatekeeper layoutdrawbridge-api.tsbuilds its dependencies and mounts the routerBehaviour-preserving — and verified, not assumed
The route block was moved programmatically rather than retyped, then diffed against the original. The only difference:
That change is required, not cosmetic.
serviceVersionis a module-scopeletreassigned after an asyncreadFileof package.json; capturing it by value when the factory runs would pin/versionto"unknown"forever. The getter preserves the original late-binding.Checks:
tsc -p tsconfig.jsonclean for the serviceARCHON_ADMIN_HEADERis used by the admin middleware and by the herald proxy for upstream auth, so it is exported fromv1-admin.tsrather than duplicatedTests
18 for the extracted router — capabilities derived from configured URLs, DIDComm endpoint including the null case, admin gating across 401 / 403-when-unconfigured / pass-through, resolve options passed only when present, numeric-vs-hash
blockIdparsing, a table-driven 502 check across every proxy route, a mid-stream read failure, and the lightning proxy's 501 / 200 / 502 paths.11 for
lightning-mediator-client— the router test transitively pulled that module into the coverage denominator at 5.9%, so leaving it would have dragged the number down for nothing. Now covered: request shaping, admin header, error mapping toLightningUnavailableError, the 404-means-null case, and percent-encoding of payment hashes.Result
v1-router.tsv1-admin.tslightning-mediator-client.tsTotal rose despite ~200 lines entering the denominator. Suite: 1,778 passing, 0 eslint errors (1 pre-existing warning in
l402-auth.ts, present on main).One thing worth a second opinion
Three one-line L402 delegation routes are deliberately left uncovered. Driving them with a stub
l402Optionsleaves one handler never sending a response, which hangs Jest — and since CI runs without--forceExit, that is a silent workflow hang rather than a failed test. Their handlers are already at 100% vial402-auth.test.ts, and a comment in the test records why.Separately: a handler that silently never responds is arguably a production issue too — a malformed-options request would hang the client instead of erroring. I have not chased which handler or whether it is reachable with real config; it may well be unreachable. Flagging rather than fixing, as it is outside this refactor.
Follow-up
services/herald/server/src/index.ts(1,490 lines) has the same module-scope-app problem and is the remaining blocker for covering herald. Worth its own PR.🤖 Generated with Claude Code