From b076736851bb593cd30b293e6f489d31d3512099 Mon Sep 17 00:00:00 2001
From: "Aaron K. Clark" <akclark@thenetwerk.net>
Date: Tue, 19 May 2026 13:47:16 -0500
Subject: [PATCH] test(error-handler): cover the requestId field on 5xx
 responses
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

#336 added `requestId` to the OpenAPI Error component schema as an
optional field on every error response. The runtime side
(`app/middleware/error-handler.js`) was already emitting it when
the request reached the request-id middleware, but there was no
behavioral test pinning the contract — a future refactor could
silently stop emitting the field and the spec-shape assertion
wouldn't catch the runtime divergence.

Add two cases:
  - 5xx response carries requestId when req.id is set (stands up
    a minimal app where an id-tagging middleware fires before the
    handler, mirroring the production order from server.js).
  - 5xx response OMITS requestId when no id is available (catches
    a future regression that synthesizes an empty string or null
    instead of leaving the field off — operators rely on the
    field's presence to distinguish "no correlator" from "empty
    correlator").

Test count: 785 → 787.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---
 tests/api/error-handler.test.js | 34 +++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/tests/api/error-handler.test.js b/tests/api/error-handler.test.js
index 021d8f4..8430cfe 100644
--- a/tests/api/error-handler.test.js
+++ b/tests/api/error-handler.test.js
@@ -113,6 +113,40 @@ describe('global error handler', () => {
         expect(text).not.toMatch(/column.*X/);
         expect(res.body.message).toMatch(/bad request/i);
     });
+
+    test('5xx response carries requestId when one is available', async () => {
+        // The OpenAPI Error schema (#336) declares `requestId` as an
+        // optional field. The handler reads it from `req.id` or
+        // `req.log.bindings().reqId` (pino-http's binding). Stand up
+        // a minimal app where the middleware order matches server.js
+        // — id-tagging middleware FIRST, then the explosion route —
+        // and assert the value lands in the 500 body verbatim so
+        // operators can correlate it to the structured log line.
+        const idApp = express();
+        idApp.use(express.json());
+        idApp.use((req, res, next) => {
+            req.id = 'test-req-id-correlator-1234';
+            next();
+        });
+        idApp.get('/boom', (req, res, next) => {
+            next(new Error('something failed deep inside'));
+        });
+        idApp.use(errorHandler);
+        const res = await request(idApp).get('/boom');
+        expect(res.status).toBe(500);
+        expect(res.body.message).toBe('Error!');
+        expect(res.body.requestId).toBe('test-req-id-correlator-1234');
+    });
+
+    test('5xx response omits requestId when none is available (no synthetic empty string)', async () => {
+        // The original /explode/500 route doesn't set req.id, so the
+        // handler should leave the field off entirely rather than
+        // emit `requestId: ""` or `requestId: null` — clients can
+        // distinguish "no correlator" from "empty correlator".
+        const res = await request(app).get('/explode/500');
+        expect(res.status).toBe(500);
+        expect(res.body.requestId).toBeUndefined();
+    });
 });
 
 describe('404 fallthrough', () => {