Files

T

RainbowBird c627bce9c9 refactor(server): split services into domain/adapter layers, drop dead code

Why
- src/services/ was an unordered mix of single-file services and module
  directories with no shared classification axis, plus several long-dead
  admin batch helpers that survived the move to the simpler synchronous
  admin-flux-grants flow.

What
- services/ now has two top-level layers:
    domain/   — DB state + business rules (billing, characters, chats,
                flux, flux-transaction, llm-router, providers, request-log,
                stripe, user-deletion, admin/{flux-grants,router-config})
    adapters/ — thin wrappers over external SDKs / infra (config-kv, email,
                posthog, tts/)
- admin/* moved under domain/admin/ with consistent plural names
  (flux-grants, router-config).
- tts-adapters/ collapsed to adapters/tts/ (no redundant -adapters suffix
  once nested under adapters/).
- 63 src files + scripts/e2e-llm-router.ts + tests/verifications/_harness.ts
  had relative imports rewritten; git mv preserves blame.
- apps/server/CLAUDE.md and docs/ai-context/*.md updated to match new paths.

Dead code removed
- services/admin-flux-grant-batches/ (service + worker + tests, 1090 LOC) —
  superseded by admin-flux-grants and never wired into app.ts.
- routes/admin/flux-grant-batches/ — same.
- utils/redis-compressed.ts + test — zero production call sites.
- llm-router/index.ts re-exports trimmed from 26 to 6; only symbols with
  external consumers are kept.

Intentionally kept
- schemas/flux-grant-batch.ts and its schemas/index.ts export remain so the
  drizzle-kit generate diff stays empty. Removing them is a separate PR
  that owns the drop-table migration for flux_grant_batch /
  flux_grant_batch_recipient.

Verification
- pnpm -F @proj-airi/server typecheck: passes.
- pnpm exec eslint apps/server: 49 errors, identical to main baseline
  (all are pre-existing node/prefer-global/buffer in envelope-crypto and
  scripts/e2e-llm-router; untouched by this change).
- Vitest passes per-file; the 6 mockDB hook timeouts under full-parallel
  run are the known pushSchema-per-worker infra cost, not a regression.

2026-05-18 23:36:45 +08:00

5.2 KiB

Raw Blame History

LLM/TTS router codex review — deferred follow-ups

Codex independent review (2026-05-15) on commit 1bb0aab2f returned 12 findings. Applied 3 HIGH inline (see commits after 1bb0aab2f). The 9 remaining findings are deferred — each evaluated through the AGENTS.md "外部建议 3 问过滤" and tracked here per "拒绝时留痕" rule.

Applied inline (HIGH severity)

Client abort signal not threaded into llmRouter.route() — fixed in apps/server/src/routes/openai/v1/index.ts. c.req.raw.signal now flows into both the router and the legacy fetch() fallback. Prevents the "client disconnects but upstream keeps generating + burning paid quota" leak.
Failed upstream response bodies not drained — fixed in apps/server/src/services/domain/llm-router/router.ts. Every non-2xx fallback path now calls response.body?.cancel() before continuing. Prevents socket-pool exhaustion under fallback storms.
SSML voice attribute injection — fixed in apps/server/src/services/adapters/tts/azure.ts. Voice id is regex-validated (^[a-z0-9-]+$/i) before SSML interpolation; invalid values throw BAD_REQUEST. Prevents attribute-context breakout under the server's Azure credential.

Deferred — rationale per finding

#4: in-flight `loadFresh()` can repopulate cache after `invalidate()`

Severity: MEDIUM. Decision: defer to v1.x. Rationale: race window is bounded by the Pub/Sub-driven invalidate firing during a concurrent TTL-driven reload. In practice the operator revokes a key, the active in-flight load was already reading the pre-revocation config and so writes back the old key state. Worst case the stale config lives until next TTL expiry (5s). Acceptable for v1 given the 5s SLO target — not a security hole, just a propagation hiccup. Fix shape (generation counter on loadFresh) is well-known and cheap; do it the next time someone touches config-loader.ts.

#5: no single-flight on concurrent cache miss

Severity: MEDIUM. Decision: defer. Rationale: configKV's underlying Redis read is single-digit-ms in practice; even a 10-request stampede is 10 cheap reads. The plan's adversarial reviewer flagged this; we accepted because the cure (promise-dedup) adds state with its own race window and rarely matters at airi's current QPS. Revisit if airi.gen_ai.gateway.config.reload spikes after a Pub/Sub burst.

#6: `fullChainTimeoutMs` not enforced as a hard cap

Severity: MEDIUM. Decision: apply in next iteration. Rationale: legit bug — per-attempt × N keys can exceed the configured 60s cap. Worth fixing but requires a route-level abort controller that composes with the per-attempt one; not a 5-minute change. Track here so it doesn't get lost.

#7: 200-with-error-envelope not detected

Severity: MEDIUM. Decision: reject for v1. Rationale: codex flagged this without citing real evidence of any v1 provider doing it. The four configured providers (OpenRouter, Azure, DashScope, Volcengine) return proper HTTP status codes for errors. Adding body-shape detection across LLM + 3 TTS adapters adds complexity that guards against a hypothetical. "外部建议 3 问过滤" (c): would create duplicated parsing logic in every adapter. Revisit if a provider is added that does return 200-with-error.

#8: incomplete plaintext zeroization in `encryptKey()`

Severity: MEDIUM. Decision: partial accept — fix in next iteration. Rationale: codex is right that encryptKey leaves plaintextBytes uncleared. Fix is try/finally { plaintextBytes.fill(0) } — apply in v1.x. Note: rendered Authorization: Bearer <key> strings cannot be zeroized once V8 has interned them as JS strings; that limitation is fundamental and worth documenting in envelope-crypto.ts JSDoc.

#9: provider API keys via argv in `seed-router-config.ts`

Severity: MEDIUM. Decision: apply. Rationale: legit security issue — keys appear in ps output and shell history. Switch to env var input (OPENROUTER_KEY etc.) and remove the --openrouter-key flag. Will apply in next iteration to the seed script; the script is operator-only and runs locally so impact is bounded but the principle is right.

#10: `e2e-llm-router.ts` debug fetch logs auth header prefix

Severity: MEDIUM. Decision: apply now (trivial). Rationale: legit. The 30-char prefix can identify accounts. Fix shape: replace ${auth.slice(0, 30)}... with <set>. Apply in next commit.

#11: identical `current` / `previous` master key not rejected

Severity: LOW. Decision: apply. Rationale: defensive guard, ~3 lines. Will apply in next iteration.

#12: Pub/Sub handler unbounded JSON parse

Severity: LOW. Decision: defer. Rationale: Redis is on the trusted private network in our deployment model; HMAC was already deferred from the plan (P2 finding from ce-doc-review). Size guard is small but its security value is contingent on the same untrusted-Redis threat we explicitly accepted. Mark as follow-up when HMAC is added in the same iteration.

Tracked

v1.x cleanup pass (issues #6, #8, #9, #10, #11): bundled commit before next ship.
v1.x or v2 (issues #4, #5, #7, #12): revisit when ops data shows the underlying assumption broke.

5.2 KiB Raw Blame History Unescape Escape