feat(server): codify the { success, error, code } response envelope in ok()/fail() helpers (#3962)

CLAUDE.md

@@ -34,6 +34,22 @@
 - **Never push directly to main. Always use a branch.**
 - After bulk find-and-replace or sed, verify modified functions have correct `async`/`await` signatures. Route handlers and callbacks need `async` if `await` was added inside.
 
+### Response envelope
+API handlers use a shared envelope helper — `src/server/utils/apiResponse.ts`:
+- Success: `ok(res, data)` → `{ success: true, data }` (omit `data` for `{ success: true }`).
+- Error: `fail(res, status, code, message, extra?)` → `{ success: false, error, code, ...extra }`.
+
+**New or modified handlers must use these.** `code` is a SCREAMING_SNAKE machine
+code; reuse an existing one where it fits.
+
+**Gotcha:** the frontend `ApiService.request()` returns the raw JSON body and
+does **not** unwrap `data`. So `ok(res, x)` is only correct for handlers that
+already return `{ success: true, data }` — converting a bare-payload handler
+(`res.json(array)`) breaks its consumer. `fail()` is always safe: `ApiService`
+reads only `error`/`code`/`retryAfterSeconds` and ignores `success` on errors.
+Existing bare-`{error}` handlers convert opportunistically as they're touched
+(Phases 2/4); this is not a mass conversion.
+
 ## Multi-Source Architecture (4.x)
 
 MeshMonitor 4.x supports **N concurrent Meshtastic node connections** ("sources"). Pre-4.0 code that referenced a singleton `meshtasticManager` is now a `@deprecated` JSDoc-tagged compatibility shim at the bottom of `src/server/meshtasticManager.ts` — IDEs will strikethrough usages but `tsc` does not error.

docs/internal/dev-notes/REMEDIATION_EPIC.md

@@ -22,8 +22,8 @@ Each unchecked phase = one worktree → architect spec → implementation → re
 - [x] **0.5** — `@typescript-eslint/no-floating-promises` as `error` for non-test code; violations fixed or explicitly `void`/`.catch()`ed.
 - [x] **1.1** — `withSourceScope` fails closed: omitting `sourceId` throws; explicit `ALL_SOURCES` sentinel for the documented global-by-design consumers (channel decryption, estimated positions, automations); full call-site audit.
 - [x] **1.2** — Type-check the tests: `tsconfig.tests.json` (full strict — see phase log deviation), wired into CI non-blocking first; flip to blocking once clean.
-- [ ] **1.3** — Integration-grade route-test harness: in-memory SQLite + real Express app with real `requirePermission`/session/auth wiring; 3–5 representative route test files converted (source-scoped permission coverage); no mass conversion.
-- [ ] **1.4** — Lint ratchets: `no-explicit-any` → `error` with checked-in baseline; forbid raw `fetch(` in `src/components/**`/`src/pages/**` (baselined); `react-hooks/exhaustive-deps` + `prefer-const` → `error`.
+- [x] **1.3** — Integration-grade route-test harness: in-memory SQLite + real Express app with real `requirePermission`/session/auth wiring; 3–5 representative route test files converted (source-scoped permission coverage); no mass conversion.
+- [x] **1.4** — Lint ratchets: `no-explicit-any` → `error` with checked-in baseline; forbid raw `fetch(` in `src/components/**`/`src/pages/**` (baselined); `react-hooks/exhaustive-deps` + `prefer-const` → `error`.
 - [ ] **1.5** — Response-envelope convention: `ok(res, data)` / `fail(res, status, code, msg)` helper for the `{ success, error, code }` envelope; documented in CLAUDE.md; new/modified handlers must use it.
 - [ ] **1.6** — Schema-drift tripwire: CI test diffing `createTables()` schema vs full migration replay (001→latest), normalized `sqlite_master`, fail on divergence.
 
@@ -46,3 +46,5 @@ Record per-phase: PR link, deviations from plan, follow-ups.
 - **1.1** — PR #3976 merged (ALL_SOURCES unique-symbol sentinel; runtime throw + Tier-2 required params; 3 real leaks fixed: server.ts getNodeCount refresh, deleteNode→deleteNeighborInfoInvolvingNode, meshtasticManager pending-DM fetch). Review-loop correction: implementer's repo-body `?? ALL_SOURCES` normalization (silent fail-open) reverted; explicit per-call-site decisions instead. CI: Quick Tests outgrew its 15-min cap (#3385 redux) → raised to 25 min in this PR.
   **Follow-up findings (1.1 architect §10, epic backlog):** hand-rolled fail-open `if (sourceId)` filters bypass the helper — all of `meshcore.ts`, `notifications.ts` subscriptions, parts of `channels.ts`, `nodes.ts:getAllNodesSqlite`; legacy `*Sync`/`*Sqlite` twins partially covered (Phase 3.4 deletes them); facade `getNode(nodeNum)` single-source-assumption chain (revisit with Phase 2).
 - **1.2** — PR TBD (tsconfig.tests.json + `typecheck:tests` script + non-blocking CI steps in ci.yml/pr-tests.yml). Deviations: (a) kept FULL strict incl. noImplicitAny — plan's "noImplicitAny off" injects ~50 false-positive errors into prod src via evolving-any inference; (b) top-level `tests/` dir NOT included — its files pull @types/node into the program and poison frontend inference (~60 spurious errors). Baseline: 283 errors, all in src test files, 0 prod. Flip-to-blocking when count reaches 0 (burndown is a follow-up; top ~8 files hold ~150 errors).
+- **1.3** — PR #3986 merged (createRouteTestApp() real-middleware harness + 4 conversions). Ride-alongs: real prod bug fixed (v1/messages GET 500'd without sourceId under fail-closed — missing ?? ALL_SOURCES) + harness anonymous-user race fix. 3 CodeQL alerts on the test fixture left open for human triage (recommend dismiss as used-in-tests).
+- **1.4** — PR #3989 merged (count-based lint ratchet, eslint-baseline.json 410 files/2,515 violations; no-explicit-any/exhaustive-deps/prefer-const → error; raw-fetch ban in components/pages; CI lint now BLOCKING — closes the 0.5 deviation). Census correction: 727 pre-existing errors (468 config false-positives fixed at config level, not baselined).

src/server/routes/dataExchangeRoutes.test.ts

@@ -53,7 +53,7 @@ describe('GET /stats', () => {
     mockDb.messages.getMessageCount.mockRejectedValue(new Error('boom'));
     const res = await request(app).get('/stats');
     expect(res.status).toBe(500);
-    expect(res.body.error).toBe('Failed to fetch stats');
+    expect(res.body).toMatchObject({ success: false, error: 'Failed to fetch stats', code: 'STATS_FAILED' });
   });
 });
 
@@ -69,7 +69,7 @@ describe('POST /export', () => {
     mockDb.exportDataAsync.mockRejectedValue(new Error('fail'));
     const res = await request(app).post('/export').send({});
     expect(res.status).toBe(500);
-    expect(res.body.error).toBe('Failed to export data');
+    expect(res.body).toMatchObject({ success: false, error: 'Failed to export data', code: 'EXPORT_FAILED' });
   });
 });
 
@@ -87,6 +87,6 @@ describe('POST /import', () => {
     mockDb.importDataAsync.mockRejectedValue(new Error('fail'));
     const res = await request(app).post('/import').send({});
     expect(res.status).toBe(500);
-    expect(res.body.error).toBe('Failed to import data');
+    expect(res.body).toMatchObject({ success: false, error: 'Failed to import data', code: 'IMPORT_FAILED' });
   });
 });

src/server/routes/dataExchangeRoutes.ts

@@ -3,6 +3,7 @@ import { requirePermission, requireAdmin } from '../auth/authMiddleware.js';
 import databaseService from '../../services/database.js';
 import { ALL_SOURCES } from '../../db/repositories/index.js';
 import { logger } from '../../utils/logger.js';
+import { ok, fail } from '../utils/apiResponse.js';
 
 const router: Router = Router();
 
@@ -23,7 +24,7 @@ router.get('/stats', requirePermission('dashboard', 'read'), async (req: Request
     });
   } catch (error) {
     logger.error('Error fetching stats:', error);
-    res.status(500).json({ error: 'Failed to fetch stats' });
+    fail(res, 500, 'STATS_FAILED', 'Failed to fetch stats');
   }
 });
 
@@ -33,18 +34,18 @@ router.post('/export', requireAdmin(), async (_req: Request, res: Response) => {
     res.json(data);
   } catch (error) {
     logger.error('Error exporting data:', error);
-    res.status(500).json({ error: 'Failed to export data' });
+    fail(res, 500, 'EXPORT_FAILED', 'Failed to export data');
   }
 });
 
 router.post('/import', requireAdmin(), async (req: Request, res: Response) => {
   try {
     const data = req.body;
     await databaseService.importDataAsync(data);
-    res.json({ success: true });
+    ok(res);
   } catch (error) {
     logger.error('Error importing data:', error);
-    res.status(500).json({ error: 'Failed to import data' });
+    fail(res, 500, 'IMPORT_FAILED', 'Failed to import data');
   }
 });
 

src/server/routes/ignoredNodeRoutes.test.ts

@@ -48,7 +48,8 @@ describe('GET /', () => {
     const res = await request(app).get('/');
 
     expect(res.status).toBe(400);
-    expect(res.body.code).toBe('MISSING_SOURCE_ID');
+    expect(res.body).toMatchObject({ success: false, code: 'MISSING_SOURCE_ID', error: 'No permitted source' });
+    expect(typeof res.body.details).toBe('string');
   });
 
   it('returns 500 on database error', async () => {
@@ -57,6 +58,7 @@ describe('GET /', () => {
     const res = await request(app).get('/');
 
     expect(res.status).toBe(500);
+    expect(res.body).toMatchObject({ success: false, code: 'INTERNAL_ERROR' });
   });
 });
 
@@ -78,7 +80,8 @@ describe('DELETE /:nodeId', () => {
     const res = await request(app).delete('/!ZZZZ');
 
     expect(res.status).toBe(400);
-    expect(res.body.code).toBe('INVALID_NODE_ID');
+    expect(res.body).toMatchObject({ success: false, code: 'INVALID_NODE_ID', error: 'Invalid nodeId format' });
+    expect(typeof res.body.details).toBe('string');
   });
 
   it('returns 400 when source cannot be resolved', async () => {
@@ -87,7 +90,8 @@ describe('DELETE /:nodeId', () => {
     const res = await request(app).delete('/!aabbccdd');
 
     expect(res.status).toBe(400);
-    expect(res.body.code).toBe('MISSING_SOURCE_ID');
+    expect(res.body).toMatchObject({ success: false, code: 'MISSING_SOURCE_ID', error: 'No permitted source' });
+    expect(typeof res.body.details).toBe('string');
   });
 
   it('succeeds even if setNodeIgnoredAsync throws (node not in nodes table)', async () => {
@@ -106,5 +110,6 @@ describe('DELETE /:nodeId', () => {
     const res = await request(app).delete('/!aabbccdd');
 
     expect(res.status).toBe(500);
+    expect(res.body).toMatchObject({ success: false, code: 'INTERNAL_ERROR' });
   });
 });

src/server/routes/ignoredNodeRoutes.ts

@@ -3,37 +3,26 @@ import { requirePermission } from '../auth/authMiddleware.js';
 import databaseService from '../../services/database.js';
 import { logger } from '../../utils/logger.js';
 import { resolveRequestSourceId } from '../utils/sourceResolver.js';
-
-interface ApiErrorResponse {
-  error: string;
-  code: string;
-  details?: string;
-}
+import { fail } from '../utils/apiResponse.js';
 
 const router = Router();
 
 router.get('/', requirePermission('nodes', 'read'), async (req: Request, res: Response) => {
   try {
     const listSourceId = await resolveRequestSourceId(req, 'nodes', 'read');
     if (!listSourceId) {
-      const errorResponse: ApiErrorResponse = {
-        error: 'No permitted source',
-        code: 'MISSING_SOURCE_ID',
+      fail(res, 400, 'MISSING_SOURCE_ID', 'No permitted source', {
         details: 'Provide ?sourceId=, or ensure your account has nodes:read on at least one enabled source',
-      };
-      res.status(400).json(errorResponse);
+      });
       return;
     }
     const ignoredNodes = await databaseService.ignoredNodes.getIgnoredNodesAsync(listSourceId);
     res.json(ignoredNodes);
   } catch (error) {
     logger.error('Error fetching ignored nodes:', error);
-    const errorResponse: ApiErrorResponse = {
-      error: 'Failed to fetch ignored nodes',
-      code: 'INTERNAL_ERROR',
+    fail(res, 500, 'INTERNAL_ERROR', 'Failed to fetch ignored nodes', {
       details: error instanceof Error ? error.message : 'Unknown error occurred',
-    };
-    res.status(500).json(errorResponse);
+    });
   }
 });
 
@@ -44,23 +33,17 @@ router.delete('/:nodeId', requirePermission('nodes', 'write'), async (req: Reque
     const nodeNumStr = nodeId.replace('!', '');
 
     if (!/^[0-9a-fA-F]{8}$/.test(nodeNumStr)) {
-      const errorResponse: ApiErrorResponse = {
-        error: 'Invalid nodeId format',
-        code: 'INVALID_NODE_ID',
+      fail(res, 400, 'INVALID_NODE_ID', 'Invalid nodeId format', {
         details: 'nodeId must be in format !XXXXXXXX (8 hex characters)',
-      };
-      res.status(400).json(errorResponse);
+      });
       return;
     }
 
     const deleteSourceId = await resolveRequestSourceId(req, 'nodes', 'write');
     if (!deleteSourceId) {
-      const errorResponse: ApiErrorResponse = {
-        error: 'No permitted source',
-        code: 'MISSING_SOURCE_ID',
+      fail(res, 400, 'MISSING_SOURCE_ID', 'No permitted source', {
         details: 'Provide ?sourceId=, or ensure your account has nodes:write on at least one enabled source',
-      };
-      res.status(400).json(errorResponse);
+      });
       return;
     }
 
@@ -76,12 +59,9 @@ router.delete('/:nodeId', requirePermission('nodes', 'write'), async (req: Reque
     res.json({ success: true, nodeNum, sourceId: deleteSourceId });
   } catch (error) {
     logger.error('Error removing ignored node:', error);
-    const errorResponse: ApiErrorResponse = {
-      error: 'Failed to remove ignored node',
-      code: 'INTERNAL_ERROR',
+    fail(res, 500, 'INTERNAL_ERROR', 'Failed to remove ignored node', {
       details: error instanceof Error ? error.message : 'Unknown error occurred',
-    };
-    res.status(500).json(errorResponse);
+    });
   }
 });
 

src/server/utils/apiResponse.test.ts

@@ -0,0 +1,69 @@
+import { describe, it, expect, vi } from 'vitest';
+import { ok, fail } from './apiResponse.js';
+import type { Response } from 'express';
+
+function makeMockRes(): Response {
+  const res = {
+    json: vi.fn(),
+    status: vi.fn(),
+  } as unknown as Response;
+  // status() must return the same res for chaining
+  (res.status as ReturnType<typeof vi.fn>).mockReturnValue(res);
+  (res.json as ReturnType<typeof vi.fn>).mockReturnValue(res);
+  return res;
+}
+
+describe('ok()', () => {
+  it('wraps a payload under data', () => {
+    const res = makeMockRes();
+    const result = ok(res, { a: 1 });
+    expect(res.json).toHaveBeenCalledWith({ success: true, data: { a: 1 } });
+    expect(result).toBe(res);
+  });
+
+  it('emits { success: true } with no data key when called without a second argument', () => {
+    const res = makeMockRes();
+    ok(res);
+    const body = (res.json as ReturnType<typeof vi.fn>).mock.calls[0][0] as Record<string, unknown>;
+    expect(body).toEqual({ success: true });
+    expect('data' in body).toBe(false);
+  });
+
+  it('includes data: null when null is passed (null is a real payload, not absence)', () => {
+    const res = makeMockRes();
+    ok(res, null);
+    expect(res.json).toHaveBeenCalledWith({ success: true, data: null });
+  });
+
+  it('returns the res object for chaining', () => {
+    const res = makeMockRes();
+    expect(ok(res, 42)).toBe(res);
+  });
+});
+
+describe('fail()', () => {
+  it('sets the status and emits the error envelope', () => {
+    const res = makeMockRes();
+    const result = fail(res, 400, 'BAD', 'nope');
+    expect(res.status).toHaveBeenCalledWith(400);
+    expect(res.json).toHaveBeenCalledWith({ success: false, error: 'nope', code: 'BAD' });
+    expect(result).toBe(res);
+  });
+
+  it('spreads extra fields into the top-level body', () => {
+    const res = makeMockRes();
+    fail(res, 500, 'X', 'boom', { details: 'd', retryAfterSeconds: 3 });
+    expect(res.json).toHaveBeenCalledWith({
+      success: false,
+      error: 'boom',
+      code: 'X',
+      details: 'd',
+      retryAfterSeconds: 3,
+    });
+  });
+
+  it('returns the res object for chaining', () => {
+    const res = makeMockRes();
+    expect(fail(res, 500, 'E', 'err')).toBe(res);
+  });
+});

src/server/utils/apiResponse.ts

@@ -0,0 +1,39 @@
+import type { Response } from 'express';
+
+/**
+ * Standard MeshMonitor success envelope: `{ success: true, data? }`.
+ *
+ * IMPORTANT: the frontend `ApiService.request()` returns the raw JSON body and
+ * does NOT unwrap `data`. Only use `ok()` on a handler that already emits
+ * `{ success: true, data }` (or `{ success: true }`), or update the consumer in
+ * the same change. Do NOT convert handlers that return a bare payload
+ * (`res.json(array)`, `res.json({ deletedCount })`) — that changes the wire shape.
+ *
+ * @param res   Express response.
+ * @param data  Optional payload placed under `data`. Omit for `{ success: true }`.
+ */
+export function ok<T>(res: Response, data?: T): Response {
+  return res.json(data === undefined ? { success: true } : { success: true, data });
+}
+
+/**
+ * Standard MeshMonitor error envelope: `{ success: false, error, code, ...extra }`.
+ * Matches what `ApiService` parses: `error` (message), `code` (machine code),
+ * plus optional fields it forwards such as `retryAfterSeconds`.
+ *
+ * @param res      Express response.
+ * @param status   HTTP status (4xx/5xx).
+ * @param code     SCREAMING_SNAKE machine code (e.g. 'MISSING_SOURCE_ID').
+ * @param message  Human-readable message (becomes `ApiError.message` on the client).
+ * @param extra    Optional additional top-level fields (e.g. `{ details }`,
+ *                 `{ retryAfterSeconds }`). Kept minimal.
+ */
+export function fail(
+  res: Response,
+  status: number,
+  code: string,
+  message: string,
+  extra?: Record<string, unknown>,
+): Response {
+  return res.status(status).json({ success: false, error: message, code, ...extra });
+}