Split the token server so the routes are readable again - #26
Merged
Alex Arguello (alex-arguello) merged 4 commits intoAug 12, 2026
Merged
Conversation
Alex Arguello (alex-arguello)
force-pushed
the
local-token-server/split-into-modules
branch
2 times, most recently
from
August 12, 2026 02:38
f1e10a8 to
5f08d5d
Compare
Base automatically changed from
local-token-server/environment-and-endpoint-guard
to
main
August 12, 2026 02:45
server.mjs was 360 lines when it landed in May and 610 now, because every new capability added its machinery to the same file. What a reader wants from it, the routes, sat under 480 lines of env parsing, host checking, token caching and redaction. No behaviour changes. The blocks moved as they were, into the same six modules the Android demo server now uses, so the two stay one thing to learn. server.mjs is 122 lines: the imports, the error handler, the five routes and the listen banner. Verified by running rather than by reading: every route answers as before against qa, a declined per-device lookup is still reported in unavailable, and a PAYABLI_ENV_FILE naming a file that does not exist still exits 1. All declarations that were in server.mjs are still present across the modules.
Alex Arguello (alex-arguello)
force-pushed
the
local-token-server/split-into-modules
branch
from
August 12, 2026 02:49
5f08d5d to
5068060
Compare
Copilot started reviewing on behalf of
Alex Arguello (alex-arguello)
August 12, 2026 02:55
View session
There was a problem hiding this comment.
Pull request overview
Refactors the demo token server into responsibility-focused ESM modules while retaining routes in server.mjs.
Changes:
- Extracts configuration, HTTP, error, and upstream helpers.
- Separates token exchange and Payabli API logic.
- Moves Tap to Pay device operations into a dedicated module.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
server.mjs |
Retains routing and server startup. |
lib/settings.mjs |
Loads environment configuration. |
lib/errors.mjs |
Defines errors and redaction helpers. |
lib/upstream.mjs |
Validates and normalizes upstream URLs. |
lib/tokens.mjs |
Handles credential exchange and token caching. |
lib/payabli-api.mjs |
Performs authenticated Payabli requests. |
lib/card-present.mjs |
Handles device listing and activation. |
lib/http.mjs |
Provides request, response, and CORS helpers. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| if (configuredCorsOrigins.size > 0) { | ||
| return false; | ||
| server.listen(port, bindHost, () => { |
Collaborator
Author
There was a problem hiding this comment.
Fixed, reason corrected · 11aea5c
The duplicate is real and came from rebuilding this branch onto main after the parent squash-merged:
the block was appended to a file that already had it.
It does not crash, though, and that was measured rather than assumed. Both callbacks fire and every
route answers, because the second listen runs in the same tick as the first, before the server has
bound, so ERR_SERVER_ALREADY_LISTEN is never reached:
first listen callback
second listen callback
The symptom is the startup banner printing twice. That banner names the upstream, the env file and the
entry point, and it is the one place a reader checks which environment is being served, so a doubled
one is worth removing on its own.
Change One listen block.
Test None automated; this server has no suite. The banner now prints once and every route still
answers against qa.
Rebuilding this branch on main appended a listen block the file already had, so the startup banner printed twice. The banner names the upstream, the env file and the entry point, which is the one place a reader checks which environment is being served, and printing it twice is exactly where that stops being trustworthy. Not a crash. Measured: both listen callbacks fire and every route answers, because the second call runs in the same tick as the first, before the server has bound, so ERR_SERVER_ALREADY_LISTEN is never reached. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The header claimed nothing else touched process.env, which was false the moment it was written: upstream.mjs read the insecure-upstream flag and tokens.mjs reads the access token and the client credentials. PAYABLI_ALLOW_INSECURE_UPSTREAM is a plain setting, so it moves here and upstream.mjs now reads no environment at all. The credential reads stay in tokens.mjs, because a request can override each of them and exporting them would put the secret in a module every other one imports. The header says that, so it is now a claim a reader can check with one grep. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot started reviewing on behalf of
Alex Arguello (alex-arguello)
August 12, 2026 03:01
View session
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Example/PayabliDemo/LocalTokenServer/lib/settings.mjs:28
- The environment file has already been loaded on line 26. This second call synchronously reads and parses the same file again at startup, contrary to the module's resolved-once responsibility. Remove the duplicate call.
loadEnv(envFilePath);
Extracting blocks carried each one's leading comment with it, so a comment attached to two blocks was written twice. loadEnv ran twice in settings.mjs. The second call reparsed the same file and could change nothing, because loadEnv skips keys already in process.env. Four comment blocks were duplicated: the device-status constants, the pending device note and the activation note in card-present.mjs, the default upstream in settings.mjs, and the endpoint guard in upstream.mjs. Found by sweeping for the shape review reported on the sibling, payabli/sdk-android#38, which named only the repeated loadEnv here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot started reviewing on behalf of
Alex Arguello (alex-arguello)
August 12, 2026 03:18
View session
Alex Arguello (alex-arguello)
deleted the
local-token-server/split-into-modules
branch
August 12, 2026 03:31
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.
Stacked on the security round. The base is that branch, so this diff is the move alone; GitHub
retargets it to
mainwhen the parent merges.Why
server.mjsarrived at 360 lines with three routes and reached 624 with five, because everycapability added its machinery to the same file. The routes, which are the thing a reader opens it
for, sat under 490 lines of environment parsing, host checking, token caching and redaction.
What this is
No behaviour changes. The blocks moved as they were, into six modules by responsibility:
lib/settings.mjsprocess.envlib/errors.mjslib/upstream.mjslib/tokens.mjslib/payabli-api.mjslib/card-present.mjslib/http.mjsserver.mjsis the imports, the error handler, the five routes and the listen banner.No dependency is added; these are plain ESM modules on Node's own loader.
Verification
There is no test suite for this server, so it was verified by running it, before and after:
/health, a minted token, 30 devices, an activation code,the credential exchange, and 404 on an unknown path
PAYABLI_ENV_FILEnaming a file that does not exist still exits 1, and.env.sandboxstill selectssandbox
Two defects the split introduced were caught that way and fixed before this commit: two modules used a
value they had not imported.
node --checkpasses both, because it only validates syntax, so themodules were also swept for cross-module references that resolve to nothing.