DC-018: claim for Hermes — Logger.error() swallows writeErrorLog promise
CI / Test & Lint (push) Has been cancelled
CI / Security audit (push) Has been cancelled

This commit is contained in:
Hermes
2026-06-27 06:37:59 -07:00
parent ab0ef9cfa1
commit 923e1ad6f9
+5
View File
@@ -123,6 +123,11 @@
- **details:** After DC-005 path-fix (commit c39c80b) shipped 67 broken-require repairs across 21 depth-2 route files, two test gaps remained: (1) no test imported any depth-2 route module, so future refactors could reintroduce class A/B/C broken paths undetected; (2) no test verified that PUBLIC_ROUTES entries (in src/utilities/middleware.js) all correspond to actually-mounted routes — exactly the kind of drift DC-012 added a regression check for (probe paths), but only for the 5 probes. The full ~27-entry PUBLIC_ROUTES list could silently go stale.
- **result:** Added 3 files, fixed 1 test helper, no production code changed. New: `__tests__/depth2-routes-smoke.test.js` discovers every .js in routes/{apps,arr,auth,config,recipes}/ and asserts (a) the module loads without MODULE_NOT_FOUND, (b) it exports a factory function, (c) the factory runs without throwing when given universal deps; plus 3 source-of-truth scans that fail if any depth-2 route re-introduces class A (`../../../src/...`), class B (`../src/...`), or class C (`utilities/responses` instead of `utils/responses`) require paths. New: `__tests__/public-routes-drift.test.js` walks every aggregator + direct-mount router via Express stack introspection and asserts (a) every PUBLIC_ROUTES entry matches an actually-mounted route, (b) every CSRF excludedPath is publicly accessible, (c) all 5 probe paths are CSRF-exempt, (d) all 5 probe paths are excluded from request logging, (e) all 5 probe paths bypass Tailscale auth. New: `__tests__/test-helpers/universal-deps.js` — a Proxy + seed-object shared by both suites that returns sensible stubs (logger-shaped object, asyncHandler pass-through, path-string stubs for `path.dirname()` calls) for any property access; supports Object.assign/spread via ownKeys+getOwnPropertyDescriptor traps so aggregator factories that copy ctx into subCtx don't lose proxy magic. Fix to the test helper: (a) `log` is now a logger-shaped object (`{error, warn, info, debug, audit}` as noops) not a bare noopFn — fixes `(ctx.log || console).error(...)` in routes/apps/index.js; (b) `asyncHandler` seeded as own enumerable property — survives Object.assign({}, ctx, { helpers }); (c) added `SERVICES_FILE`, `CONFIG_FILE`, `TOTP_CONFIG_FILE`, `TAILSCALE_CONFIG_FILE`, `NOTIFICATIONS_FILE`, `loadSiteConfig`, `loadNotificationConfig`, `configStateManager`, `readConfig`, `saveConfig`, `helpers`, `safeErrorMessage` as own-enumerable seeds so aggregator sub-mounts destructure cleanly. Fix to public-routes-drift: aggregator walks use prefix `/api/v1` (matches src/app.js's bare-mount on apiRouter at /api/v1), direct-mount walks use `/api/v1` + explicit prefixMap entry. Added `routes/themes.js` and `routes/license.js` to directMounts (themes bare-mounted, license on `/license`). Result: **35 suites, 1036 tests, all passing** (was 1030 passing + 6 failing before this commit). The 6 failures were depth-2 factory errors + 22 PUBLIC_ROUTES stale entries that the test infrastructure was silently swallowing.
### DC-018: Logger.error() swallows writeErrorLog promise — error.log writes are fire-and-forget (flaky test + lost logs in prod)
- **status:** in-progress
- **owner:** hermes
- **details:** `Logger.error()` in `src/utils/logging.js:256` calls `this._log('error', ...)` but does NOT return the result. `_log('error', ...)` returns the promise from `writeErrorLog(...)` (the async disk write to error.log). Because `error()` drops the return value, every `await logError(...)` / `await log.error(...)` caller is actually awaiting `undefined` — the file write becomes fire-and-forget. Symptoms: (1) `__tests__/logging.test.js` "captures request context when req is passed" fails intermittently in the full suite (passes in isolation) — the test reads error.log before the un-awaited appendFile completes. (2) In production, 6 route handlers (`routes/apps/deploy.js`, `routes/apps/removal.js`, `routes/health.js`, `routes/arr/config.js`, `routes/updates.js`) plus the global `boundAsyncHandler` error catcher all `await logError(...)` expecting the write to flush; error entries can be lost if the process exits/restarts immediately after. Latent since the original "unify logger" commit f71e5c5. Fix: add `return` to `Logger.error()` so the `writeErrorLog` promise propagates to callers. No behavior change for `debug/info/warn` (they never returned a promise and don't write to disk).
## Coordination Rules
1. **Always `git pull` before starting work.**