build(COMPASS-28): move to express 5

[timdawborn]

Ticket

COMPASS-28 — Bump spot's NodeJS minimum from 18 to 22

PR 5 of 9. Staying in draft until the whole chain is verified.

Stacked on #2721 → #2720 → #2719. Review those first — until they merge, the diff here includes their commits. This PR's own commit is 06791aa.

What

express ^4.19.2 → ^5.2.1, @types/express ^4.17.21 → ^5.0.6.

Most of express 5's breaking surface is not reachable here

Audited before bumping:

Breaking change	Exposure
path-to-regexp v8 route syntax	None — only literal /health and /validate, plus bare app.use
Default query parser extended → simple	None — req.query is never read; the mismatcher parses the query string itself via qs
res.send(status) removed	None — only res.send(object)
app.del, req.param() removed	None

The types caught a real behaviour change

Both servers built their readiness promise like this:

new Promise<void>(resolve => app.listen(port, resolve))

express 5's app.listen does server.once('error', done) on the callback it is handed:

// node_modules/express/lib/application.js
if (typeof args[args.length - 1] === 'function') {
  var done = args[args.length - 1] = once(args[args.length - 1])
  server.once('error', done)          // <- new in 5
}

So a port that cannot be bound now arrives at that callback. Passing resolve straight in settles the promise successfully, with the error as its resolved value — reporting a server that is not accepting requests as started, and letting defer()'s caller proceed.

Express 4 had no such line and never called back on a bind failure, which is why the old code was sound then and is not now. Confirmed empirically against both majors by starting two servers on one port:

	callback fires?	with
express 4.19.2	no	— (error event only)
express 5.2.1	yes	EADDRINUSE

Both call sites now reject instead:

app.listen(port, (error?: Error) => (error ? reject(error) : resolve()));

Why each fix carries a test

The compiler does not cover this. A callback written to ignore its argument type-checks cleanly and resolves anyway:

app.listen(port, () => resolve());   // 0 type errors, silently wrong

So validation-server/server.spec.ts and mock-server/server.spec.ts each occupy the port first and assert the rejection. Mutation-checked both — the snippet above produces 0 type errors and fails both tests.

How this was verified

Locally on Node 22.23.2, all exit 0:

Check	Result
pnpm build	0
pnpm test	0 — 54 suites, 555 tests (2 new), 44 snapshots
pnpm lint:check	0
pnpm build-docs	0
docker build + check-image-parity	0 — including the validation-server readiness and /health cases

Against the real CLI, since the validation server is one of the commands the image ships:

• spot validation-server prints the readiness line and answers /health with 200 on express 5.
• On an already-bound port it now exits through oclif with Error: listen EADDRINUSE / Code: EADDRINUSE — the fix working end to end, rather than reporting a false start.

Unrelated pre-existing behaviour, noted so it is not read as a regression: POST /validate with a malformed body ({}) returns HTTP 500 with Cannot read properties of undefined (reading 'path'). Byte-identical on express 4.19.2, so this bump does not touch it. Arguably it should be a 400 rather than a 500 leaking an internal TypeError — worth its own ticket.

[timdawborn]

Automated review — /pr-review-toolkit:review-pr

Agents run: code-reviewer, silent-failure-hunter, pr-test-analyzer, comment-analyzer. Scoped to this PR's own commit (06791aa).

This one found real gaps. Three findings are things this PR's description claims to have covered and hasn't.

🔴 Critical (2)

1. cli/src/commands/docs.ts:52-61 — a third app.listen call site left on exactly the broken pattern this PR fixes twice.

this.log(`Documentation server started on port ${port}`);
await server.listen(port);   // returns http.Server, not a Promise
} catch (err) { this.error(err as Error, { exit: 1 }); }

listen returns an http.Server, so the await resolves immediately and the catch is dead code. No callback is passed, so express registers nothing on 'error' and EADDRINUSE surfaces as an uncaught event — after the code has already printed "Documentation server started". The breaking-surface audit in the description enumerates neither this site nor its routes. My own "list every caller" check missed it.

2. The always-reject arm of the ternary is untested — the mutant survives the entire suite.

I verified this directly:

mutant: app.listen(port, (error?: Error) => reject(error))
  type errors: 0
  tests:       16 passed, 16 total

No test anywhere calls defer() and asserts it resolves. Both servers would exit(1) on every start and the suite stays green. My mutation check covered () => resolve() — one arm — and I described that as covering the fix. It doesn't. Fix: bind on port 0 and await expect(defer()).resolves.toBeUndefined().

🟡 Important (2)

3. Post-bind server errors are now silently swallowed — a regression introduced by this upgrade. mock-server/server.ts:114, validation-server/server.ts:72. Express 5 once()-wraps the callback, so after listening fires the 'error' listener remains attached but is a dead no-op. Verified: server.listenerCount('error') === 1 and server.emit('error', EMFILE) returns true and does nothing — process alive, nothing logged. On express 4 the same emit crashed loudly with the errno. Scenario: mock server under fd exhaustion stops accepting, prints nothing, callers hang. Fix: capture the handle (const server = app.listen(...)) and attach a durable server.on("error", …) after resolve.

4. app.listen's return value is discarded at both sites, so nothing can close() the server — which also blocks fixing #3 and any graceful shutdown.

🔵 Suggestions (3)

• Occupier ports are hardcoded with no 'error' handler (5908, 8099). Not a jest -w 4 self-collision hazard — both are below the ephemeral ranges and each appears in one file — but a pre-existing host process holding one makes occupier.listen(PORT, resolve) never fire, so the await hangs to timeout while an unhandled 'error' surfaces separately. Loud but misleading. listen(0) + address().port removes the constant and the hazard.
• The rationale comment is duplicated across four sites and will drift on the next express upgrade. Both defer bodies are now character-identical — a listenAsync(app, port) helper would let the claim and its test live once.
• The comment overclaims by omission and narrates the change. It reads as a standing error channel; it isn't (once()-wrapped, one event wins, later errors go nowhere). And "Passing resolve straight in settles the promise successfully" describes the pre-PR shape, which the repo's rules put in the commit message.

✅ Verified sound

• The central express-5 claim checks out against express/lib/application.js:598-606.
• The rest of the audit holds: matcher.ts does its own :param → regexp so path-to-regexp v8 is unreachable; no req.query read anywhere; body-parser 2's req.body === undefined is guarded at proxy.ts:45; res.send(object) still delegates to res.json; both CLI callers await defer() inside try/catch so the new rejection is handled and there's no unhandled rejection.
• Test cleanup is correct — the finally closes the occupier on both paths, and Node nulls the handle before emitting EADDRINUSE, so the failed express server leaves no dangling handle.

Recommended action

1. Fix docs.ts — same bug, third site.
2. Add the resolve-path test to both specs; the current pair only pins one arm.
3. Decide on Fix OpenAPI 2 generator to produces vallid OpenAPI 2 #3: either capture the handle and attach a durable error listener, or accept it and say so at the site with a follow-up ticket.
4. Then the comment/port suggestions.

[timdawborn]

Automated review, round 2 — /pr-review-toolkit:review-pr

Agents: code-reviewer, silent-failure-hunter, comment-analyzer, pr-test-analyzer. Scoped to the revised commit 4baaf95.

All four round-1 fixes verified sound, by running them rather than reading them. But extracting the helper opened a gap that the per-server tests it replaced used to cover.

🔴 Critical (0)

None.

🟡 Important (1)

The helper is well tested; the wiring is not. I created this in round 1 by moving coverage off the servers and onto the helper.

Nothing in the repo calls defer(). Verified with the realistic regression — drop the await, keep the call:

type errors:      0
eslint findings:  0
Tests:            65 passed, 7 suites — all green

Gutting both defer bodies to async () => undefined also passes; only eslint's no-unused-vars catches the orphaned import, and only because nothing else uses it. So someone can unwire defer from the helper entirely and the suite stays green. docs.ts has the same exposure — reverting it to await server.listen(port) type-checks and nothing fails.

One case per caller closes all three at once, occupier on port 0:

await expect(runValidationServer(port, contract).defer())
  .rejects.toMatchObject({ code: "EADDRINUSE" });

That asserts await-ness and helper wiring in a single assertion, which is what the deleted per-server tests were doing incidentally.

🔵 Suggestions (5)

• Case 1 of the helper spec is dominated by case 3. The only mutation it catches (always-reject) is also caught by case 3, and its distinguishing assertion is inert: hoisting resolve out of the callback still leaves server.listening === true, because libuv binds synchronously. Either drop its claim to cover the reject polarity, or make it load-bearing by round-tripping a real request.
• Teardown only runs on the resolve path. Under the always-reject mutation, two bound servers leaked and jest hung past two minutes — --forceExit was needed. A future regression there surfaces as a CI timeout rather than a failure. Close in afterEach off a tracked list. (Baseline is clean under --detectOpenHandles.)
• The occupier's error handler swallows, so the port promise never settles and an occupier bind failure becomes a bare 5s timeout. Reject on error instead.
• express-listen.ts:18 overclaims — "where it terminates the process instead". Node's default for an unhandled error is to throw, which terminates only absent an uncaughtException handler; and the spec observes throwing, not termination. Say "…is thrown rather than swallowed".
• express-listen.ts:17-18 says "dropping it" (singular) against an indiscriminate removeAllListeners("error"). Sound only because app.listen creates the server, so no caller can have attached one — worth naming, since the module now returns the Server and invites exactly that caller. Also the docblock never says the promise resolves with the Server.

✅ Verified sound

• Post-bind restoration works, measured not assumed. At listening the server carries exactly one error listener (express's once-wrapped done). After removeAllListeners: count 0, emit('error', …) throws, process exits 1 with message, code: 'EMFILE', errno, syscall and the Emitted 'error' event on Server instance at: trace. eventNames() afterwards is ['request','connection','listening'] — only the error slot cleared. Express 4 parity restored exactly.
• All three commands behave end-to-end on a busy port: exit 1, Code: EADDRINUSE, and no success line — including docs, which previously printed both.
• Nothing is dropped by the blanket removal. No caller can attach an error listener in the window, because listen() hands the Server back only after the removal. All three call sites discard it anyway.
• Terminating is the right outcome. A post-bind server error is bind/accept-level (EMFILE, ENFILE), never recoverable, and the process is serving nothing. Logging-plus-exit would only add attribution and would reintroduce the shape this PR removed — an attached listener a later edit can leave logging without exiting.
• docs.ts's ordering claim is compiler-backed: error(input, { exit }) returns never, so the success lines are provably unreachable on failure. No ordering spec needed.
• No sibling .listen( sites remain — only the helper and the spec's net occupier.
• The docblock's express-5 claims all check out against application.js:598-606, and no express-4 narration survives in code.

Out of scope, worth a ticket

validation-server/server.ts:57-59 — a broad catch returns a 500 and logs nothing. Reproduced live: POST /validate with {"garbage":true} returns Cannot read properties of undefined (reading 'path') and server stderr is empty. An internal TypeError is indistinguishable from a caller sending the wrong shape, and the operator of a CI sidecar has no record at all. Pre-existing, not in this diff.

Recommended action

1. Add the per-caller wiring case — it is the one thing round 1 made worse rather than better.
2. Fix the teardown so a regression fails rather than times out.
3. Then the comment precision items.