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

[Yeraze]

Summary

Task 1.5 of the remediation plan (#3962, Phase 1). Deliberately tiny.

• New src/server/utils/apiResponse.ts — ok<T>(res, data?) → { success: true, data? } and fail(res, status, code, message, extra?) → { success: false, error, code, ...extra }, matching what ApiService already parses. The JSDoc pins the load-bearing constraint discovered during investigation: the frontend returns raw JSON and does not unwrap data, so fail() is always safe to introduce but ok() may only replace handlers already emitting { success: true } — never bare payloads.
• Two exemplar conversions proving the ergonomics: ignoredNodeRoutes.ts (four fail()s with codes + details; deletes a local one-off error interface) and dataExchangeRoutes.ts (ok(res) + three fail()s). Bare-payload success responses deliberately untouched.
• CLAUDE.md documents the convention: new/modified handlers must use the helpers; existing bare-{error} handlers convert as they're touched in Phases 2/4. Not a mass conversion (per plan).
• Epic status doc updated through 1.4.

Verification

• 8 helper unit tests + updated route-test assertions; independent orchestrator run: full suite 8199 passed, 0 failed, 0 suite failures (success: true); typecheck clean; test-typecheck at baseline (283); npm run lint:ci green under the new blocking ratchet.

Refs #3962

🤖 Generated with Claude Code

https://claude.ai/code/session_01AphLfgUJWaFNAgU5UXdJ4n

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review: feat(server): codify response envelope helpers

Tasks:

• Gather context and understand the request
• Review apiResponse.ts implementation
• Review apiResponse.test.ts coverage
• Review exemplar route conversions
• Review updated route tests
• Summarize findings

---

Summary

This is a clean, deliberately-scoped PR. The implementation is correct, the JSDoc warning about data unwrapping is the right call, and the tests are solid. A few minor observations below.

---

src/server/utils/apiResponse.ts

Good: The ok<T> generic signature, the data === undefined check (correctly allows null as a real payload), and the extra? spread on fail() are all well-reasoned.

Minor – code is untyped string: The parameter accepts any string, so a typo like 'internal_error' (lowercase) would compile fine and silently violate the SCREAMING_SNAKE convention documented in the JSDoc. A union type or a const enum of known codes would catch this at compile time. That said, given this is a Phase 1 helper meant to be simple and incrementally adopted, leaving it as string is a reasonable tradeoff; just worth being aware of.

Minor – status is untyped number: Similarly, status: 200 on an error path would compile. A branded type or a HttpErrorStatus = 400 | 401 | 403 | 404 | 409 | 422 | 429 | 500 | 503 union could prevent accidental misuse, but again this is a Phase 1 helper — not a blocking issue.

---

src/server/routes/ignoredNodeRoutes.ts

Observation (line 59) — DELETE /:nodeId success response is NOT using ok():

res.json({ success: true, nodeNum, sourceId: deleteSourceId });

This is intentional and correctly left as-is per the PR description ("bare-payload success responses deliberately untouched"), since ok() would wrap nodeNum and sourceId under a data key and break any consumer expecting those fields at the top level. Worth confirming no frontend consumer reads nodeNum/sourceId from that response (or if it does, that it's not silently broken). Not a bug in this PR, just something to track when that handler gets a full conversion.

Good: All four fail() conversions are correct. The removal of the local ErrorWithDetails interface is a nice cleanup.

---

src/server/routes/dataExchangeRoutes.ts

Good: ok(res) on the import success path is the right conversion (the handler previously returned { success: true } — confirmed in the test at line 82). The GET /stats and POST /export success paths are correctly left as raw res.json() because they return bare payloads.

Good: fail() codes (STATS_FAILED, EXPORT_FAILED, IMPORT_FAILED) follow the SCREAMING_SNAKE convention.

---

Test Coverage

apiResponse.test.ts: 8 tests cover the contract well, including the null vs undefined distinction for data, the chaining return value, and the extra spread. One gap:

• Missing: fail() called with an extra that overlaps a built-in field (e.g. { success: ..., error: ..., code: ... }). The spread ...extra would silently overwrite success, error, or code. This is an edge case that's unlikely in practice but worth a defensive test or a note that callers must not pass those keys in extra. Example:

  fail(res, 400, 'X', 'msg', { success: true }); // would emit { success: true, error: 'msg', code: 'X' }

ignoredNodeRoutes.test.ts: Coverage of all error branches (invalid format, missing source, DB error, and the tolerated setNodeIgnoredAsync failure) is thorough.

dataExchangeRoutes.test.ts: Coverage is good. The test at line 34 confirms GET /stats returns a bare object (not { success, data }), which validates that the success path was correctly left unconverted.

---

Note on Route Test Pattern

Both route test files use the legacy vi.mock('../../services/database.js', ...) pattern rather than the createRouteTestApp() harness introduced in #3986. Per CLAUDE.md, the harness is the preferred pattern for new/changed route tests. This might be worth addressing if these files are touched again, but since neither file introduced new mock patterns (and the PR description calls out that these are exemplar conversions, not infrastructure changes), it's acceptable as-is for now.

---

Overall

The PR does exactly what it says: establishes the shared envelope helpers with a clear contract, adds targeted exemplar conversions with tests, and documents the gotcha around data unwrapping prominently. No bugs found. The minor items above are observations, not blockers.