Dedupe relay response-parsing + operator calls; add relay unit tests

Collapse three byte-identical response-parsing blocks (getRelay, postRelay,
and the tts error path) into one handleRelayResponse helper. pingBalance is
deliberately left out: it records a relay error even on a parsed envelope,
a different contract from the other three (updateRelayState clears lastError
while recordRelayError sets it), so folding it in would change observable
state.

Collapse the six operator-to-relay calls into operatorPost / operatorGet,
preserving their intentional split: writes (tier grant, invoice, order)
throw on misconfig or non-OK so the operator action surfaces the failure;
reads (tier, plans, expiring subs) return null so the caller falls back to
a default. Per-function signatures, body shapes, error messages, and the
throw-vs-null behavior are unchanged.

Add server/test/relay.test.js (first fetch-mock harness for relay.js):
14 tests covering the tts error-path control flow, handleRelayResponse's
envelope parsing and error-recording rule, and the operator throw-vs-null
contract including the missing-config branch. 158 tests pass.

ROADMAP gains the deferred refactor-survey items (subscription engine,
/api/process pipeline, sweep middleware, transcript coalescers) and notes
the relay-test coverage against the existing known-debt entries.
This commit is contained in:
Keysat
2026-06-20 06:07:21 -05:00
parent f2188fa797
commit 890d671bf2
3 changed files with 370 additions and 124 deletions
+40 -2
View File
@@ -57,15 +57,53 @@ their own copy — kept in sync; the meta `theme-color` stays a literal `#0a0e1a
Real but not release-blocking for self-host. The P0/P1 findings from the same eval were fixed 2026-06-15 (see git log + `EVALUATION.md`).
- **Operator-internal strings leak to cloud users at the SSE error boundary** (Parakeet/Gemma/CUDA/LAN IPs) — no scrub exists, violating the scrub convention in `AGENTS.md`. `server/index.js:3432,3003,4246` + `providers/relay.js:135-144`. (Sharp edge: `index.js:3419` *detects* these strings, then forwards them anyway.)
- **Operator-internal strings leak to cloud users at the SSE error boundary** (Parakeet/Gemma/CUDA/LAN IPs) — no scrub exists, violating the scrub convention in `AGENTS.md`. `server/index.js:3432,3003,4246` + `providers/relay.js:134-143` (now centralized in `handleRelayResponse`, with a copy in `pingBalance` at `:834` — the 2026-06-19 dedup makes the relay-side scrub a single-point fix). (Sharp edge: `index.js:3419` *detects* these strings, then forwards them anyway.)
- **Credit over-spend TOCTOU on licensed installs** — N parallel requests pass the `total>0` check before any blind `debitOne` lands. Make check+debit atomic (reserve up front, refund on failure). `index.js:2497-2550` vs `:3158,:4197`.
- **Multi-mode tenant can spend the operator's server Gemini key** via `transcriptionProvider:"gemini"` + empty key (bypasses relay metering) — `providers/index.js:104-114`. Refuse the operator-key fallback for non-admin tenants.
- **`GET /api/history` parses every full session file** (transcript+summary, MB each) just to list ~8 metadata fields — cache them into `_meta.json` on save. `history.js:418-437`.
- **Dependency CVEs** — nodemailer 6.10.1 (high; low practical reach here), ws/qs/express/protobufjs (moderate). `npm audit fix` (nodemailer is a major bump).
- **No tests on the riskiest files** (`/api/process` gating, `relay.js`, `tenant-auth.js`, billing) — the real summarize→save→debit path can't run end-to-end without a key/credits. Add an integration test as the regression net.
- **No tests on the riskiest files** (`/api/process` gating, `tenant-auth.js`, billing; `relay.js`'s response handling + operator helpers now have unit coverage via `test/relay.test.js`, but its transcribe/summarize/analyze paths still don't) — the real summarize→save→debit path can't run end-to-end without a key/credits. Add an integration test as the regression net.
- **Smaller hardening:** unsanitized IDs persisted to `_meta.json` (array-form library import + `PUT /api/history/move`) — no file-path escape (read-time `safeFilename` guards the load), but sanitize at write too; `PUT /api/history/meta` accepts arbitrary JSON shapes with no schema; `index.js` is 4351 lines mixing routing/pipeline/yt-dlp/SSE.
- **Doc drift (high-value):** AGENTS.md credit-gate order omits the "paid cloud user" bypass state (`:77` vs `index.js:2464-2472`); operator-facing `startos-registry/.../INSTRUCTIONS.md` + `assets/ABOUT.md` are stale Gemini-first (relay is the default provider).
## Refactoring backlog (from the 2026-06-19 refactor-scout survey)
The two low-risk wins from the same survey are already DONE (relay.js response-parsing
+ operator-call dedup — extracted `handleRelayResponse` / `operatorPost` / `operatorGet`,
with a new `server/test/relay.test.js` covering the tts error-path control flow, envelope
parsing, and the operator throw-vs-null split; **158 tests pass**, +14). The rest are
deferred behind a **missing test net**`server/index.js`
(4,376 lines) is the god file (router + pipeline + subscription engine + podcast RSS), and
its riskiest logic has no integration coverage, so these extractions need a characterization
test FIRST. Listed by descending value; the highest-value one is the subscription engine.
- **Extract the subscription engine.** Move the subscription-discovery helpers
(`isPodcastFeedUrl`, `isAppleShowUrl`, `resolveAppleShowToFeed`, `parsePodcastRSS`,
`fetchDatesFromRSS`, `getChannelId`, `fetchChannelName`, `listChannelVideosFast`,
`fetchUploadDates`; `index.js:~891-1008`) plus `checkScopeSubscriptions` /
`_checkSubscriptionsInner` (`index.js:~1421`, ~800 lines total) into
`server/subscription-engine.js`. None depend on the Express `app` — they're trivially
testable in isolation but currently unreachable by any test. Write characterization tests
against fixture XML + mock yt-dlp output first.
- **Extract the `/api/process` pipeline.** The handler is a single ~1,800-line function
(`index.js:~2484-4281`): credit gate, provider resolution, URL resolution, relay
unified-pipeline branch, captions fast-path, relay-URL fast-path, audio-download-with-retry,
chunked-transcription, analysis-with-fallback, dispatch. Split into `server/pipeline.js`,
one step at a time. RISK: the many early-return paths with `releaseFreeSlot()` + `res.end()`
make blind extraction dangerous — write a characterization test (mock transcriber +
analyzer, ≥3 paths: relay mode, captions fast-path, chunked fallback) BEFORE touching it.
- **Extract `sweepAndRefreshTrial(req, res)` middleware.** A ~35-line block (sweep unapplied
purchases for `req.user`/`req.trial`, re-read the trial row, update `req.trial`) is
copy-pasted between `/api/account/whoami` (`index.js:~280-420`) and `/api/relay/status`
(`index.js:~561-675`), differing only by log prefix. MED risk (auth/billing path; moving it
changes when `req.trial` mutates relative to other middleware) — write an integration test
for both endpoints first.
- **Move the pure transcript coalescers to `util.js`.** `coalesceTranscriptEntries` +
`coalesceForAnalysis` (`index.js:~2167-2270`) are pure functions (no I/O, no state) buried
in the god file and currently untested, with non-trivial logic (median-duration guard,
bucket sizing, `indexMap`). Low-risk move; add unit tests alongside per the repo's
test-with-the-change convention.
## Deferred hardening & cleanup (P3, from the 2026-06-14 full-eval — `EVALUATION.md`)
Low-severity; batch when convenient. None block release. (P0/P1 work queue and P2 known debt live in `AGENTS.md` → Current state.)