fix(server): fix the quirks preserved by the exchange-rates DI migration

- the Frankfurter fetch now carries an AbortSignal timeout (10s) and a 1 MB
  response-size cap, and the body is boundary-validated through a type guard
  instead of an as-cast
- fetch failures are logged instead of silently swallowed (callers still get
  the graceful null)
- the inflight-coalescing cleanup moved into .finally so a rejected fetch
  can no longer leak the key
- the dead convertWithRates export (zero call sites) is dropped from the
  service and the bridge

Kept on purpose: the beyond-TTL stale-cache fallback, the >1-keys
empty-response heuristic and the || falsy-coercion defaults.

server/src/nest/budget/exchange-rates.bridge.ts

@@ -5,9 +5,11 @@ import { ExchangeRatesService } from './exchange-rates.service';
  * the Nest container (the legacy `services/budgetService.ts` FX-freeze/rebase
  * seams and the legacy `get_settlement_summary` registrar in
  * src/mcp/tools/budget.ts; the plugin RPC host injects ExchangeRatesService via
- * PluginHostDepsFactory). Exports the legacy services/exchangeRateService names
- * 1:1, so repointing a consumer is an import-path-only diff. Inside the
- * container, inject ExchangeRatesService instead.
+ * PluginHostDepsFactory). Exports only the legacy services/exchangeRateService
+ * names still consumed outside the container (getRates — the dead
+ * convertWithRates export was dropped in the post-fold quirk fixes), so
+ * repointing a consumer is an import-path-only diff. Inside the container,
+ * inject ExchangeRatesService instead.
  * Delete this file when the budgetService migration lands.
  *
  * Module-level construction is safe: the service has no dependencies and no
@@ -20,13 +22,3 @@ const fx = new ExchangeRatesService();
 export function getRates(base: string) {
   return fx.getRates(base);
 }
-
-/** Unconsumed today (no call sites) — re-exported for 1:1 legacy surface parity. */
-export function convertWithRates(
-  amount: number,
-  from: string | null | undefined,
-  base: string,
-  rates: Record<string, number> | null,
-): number {
-  return fx.convertWithRates(amount, from, base, rates);
-}

server/src/nest/budget/exchange-rates.service.ts

@@ -12,32 +12,53 @@ import { Injectable } from '@nestjs/common';
  * callers get `null`/identity conversion and amounts are treated as already in
  * the base currency rather than throwing.
  *
- * Moved 1:1 from the legacy `services/exchangeRateService.ts` (the budget-domain
- * fold): identical fetch, cache/coalescing behavior, `||` falsy-coercion
- * defaults and null degradation. The cache and inflight maps are deliberately
- * MODULE-scoped, not instance fields — the DI singleton and the
- * exchange-rates.bridge instance wrap the same upstream feed, so they must
- * share one cache (same reasoning as the permissions.service.ts cache).
+ * Moved from the legacy `services/exchangeRateService.ts` (the budget-domain
+ * fold): identical cache/coalescing behavior, `||` falsy-coercion defaults and
+ * null degradation. The cache and inflight maps are deliberately MODULE-scoped,
+ * not instance fields — the DI singleton and the exchange-rates.bridge instance
+ * wrap the same upstream feed, so they must share one cache (same reasoning as
+ * the permissions.service.ts cache).
+ *
+ * Post-migration fixes on top of the relocated legacy behavior: the outbound
+ * fetch carries an AbortSignal timeout and a response-size cap, the response is
+ * boundary-validated instead of `as`-cast, failures are logged instead of
+ * silently swallowed (callers still get null), and the inflight cleanup is
+ * rejection-proof. Kept on purpose: the stale-cache fallback on upstream
+ * failure (stale beats nothing) and the ">1 keys" empty-response heuristic.
  */
 
 const TTL_MS = 6 * 60 * 60 * 1000; // 6h
+const FETCH_TIMEOUT_MS = 10_000;
+// Frankfurter quotes a few dozen currencies (~2 KB); anything near this cap is
+// not the API we think it is.
+const MAX_RESPONSE_BYTES = 1024 * 1024;
 const cache = new Map<string, { rates: Record<string, number>; ts: number }>();
 const inflight = new Map<string, Promise<Record<string, number> | null>>();
 
+const isRateEntry = (v: unknown): v is { quote: string; rate: number } =>
+  typeof v === 'object' && v !== null &&
+  typeof (v as { quote?: unknown }).quote === 'string' &&
+  typeof (v as { rate?: unknown }).rate === 'number';
+
 async function fetchRates(base: string): Promise<Record<string, number> | null> {
   try {
-    const res = await fetch(`https://api.frankfurter.dev/v2/rates?base=${encodeURIComponent(base)}`);
+    const res = await fetch(`https://api.frankfurter.dev/v2/rates?base=${encodeURIComponent(base)}`, {
+      signal: AbortSignal.timeout(FETCH_TIMEOUT_MS),
+    });
     if (!res.ok) return null;
+    const text = await res.text();
+    if (text.length > MAX_RESPONSE_BYTES) return null;
     // Frankfurter returns an array of { date, base, quote, rate } and omits the
     // base's own self-rate, so seed the map with `base = 1` then index by quote.
-    const data = (await res.json()) as Array<{ quote?: string; rate?: number }>;
+    const data: unknown = JSON.parse(text);
     if (!Array.isArray(data)) return null;
     const rates: Record<string, number> = { [base.toUpperCase()]: 1 };
     for (const r of data) {
-      if (r && typeof r.quote === 'string' && typeof r.rate === 'number') rates[r.quote] = r.rate;
+      if (isRateEntry(r)) rates[r.quote] = r.rate;
     }
     return Object.keys(rates).length > 1 ? rates : null;
-  } catch {
+  } catch (err) {
+    console.error('[exchange-rates] rates fetch failed for', base, err);
     return null;
   }
 }
@@ -54,11 +75,14 @@ export class ExchangeRatesService {
     // Coalesce concurrent fetches for the same base.
     let p = inflight.get(key);
     if (!p) {
-      p = fetchRates(key).then(rates => {
-        if (rates) cache.set(key, { rates, ts: Date.now() });
-        inflight.delete(key);
-        return rates;
-      });
+      p = fetchRates(key)
+        .then(rates => {
+          if (rates) cache.set(key, { rates, ts: Date.now() });
+          return rates;
+        })
+        .finally(() => {
+          inflight.delete(key);
+        });
       inflight.set(key, p);
     }
     const rates = await p;
@@ -66,22 +90,4 @@ export class ExchangeRatesService {
     if (!rates && hit) return hit.rates;
     return rates;
   }
-
-  /**
-   * Convert `amount` from `from` currency into `base` using a rates map obtained
-   * from getRates(base). Identity when same currency or the rate is missing.
-   */
-  convertWithRates(
-    amount: number,
-    from: string | null | undefined,
-    base: string,
-    rates: Record<string, number> | null,
-  ): number {
-    const fromCur = (from || base).toUpperCase();
-    const baseCur = base.toUpperCase();
-    if (fromCur === baseCur || !rates) return amount;
-    const r = rates[fromCur];
-    if (!r || r <= 0) return amount;
-    return amount / r;
-  }
 }

server/tests/unit/nest/exchange-rates.service.test.ts

@@ -1,50 +1,55 @@
 /**
  * Unit tests for the DI-native ExchangeRatesService — FX-SVC-001 through
- * FX-SVC-020. New suite: the legacy services/exchangeRateService.ts had no
- * dedicated tests; the budget-domain fold moves it inside the src/nest/**
- * coverage gate. 001–018 pin the fetch/cache/convert behavior byte-for-byte
- * (including the parity quirks: `|| 'EUR'` falsy coercion, stale-cache
- * fallback, the `>1 keys` failure heuristic, silent catch → null); 019–020 pin
- * the exchange-rates.bridge delegation and the module-scoped cache shared
- * between the DI instance and the bridge instance.
+ * FX-SVC-022. New suite: the legacy services/exchangeRateService.ts had no
+ * dedicated tests; the budget-domain fold moved it inside the src/nest/**
+ * coverage gate. 001–012 pin the fetch/cache behavior (including the parity
+ * quirks kept on purpose: `|| 'EUR'` falsy coercion, stale-cache fallback, the
+ * `>1 keys` failure heuristic); 019 pins the exchange-rates.bridge delegation
+ * and the module-scoped cache shared between the DI instance and the bridge
+ * instance; 021–022 pin the post-fold quirk fixes (AbortSignal timeout,
+ * response-size cap, logged failures). 013–018 and 020 covered the dead
+ * convertWithRates export and were removed with it in the quirk fixes.
  *
  * The rate cache is deliberately MODULE-scoped, so it persists across tests in
  * this file — every case uses its own base currency to stay isolated.
  */
 import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
 import { ExchangeRatesService } from '../../../src/nest/budget/exchange-rates.service';
-import { getRates as bridgeGetRates, convertWithRates as bridgeConvertWithRates } from '../../../src/nest/budget/exchange-rates.bridge';
+import { getRates as bridgeGetRates } from '../../../src/nest/budget/exchange-rates.bridge';
 
 const TTL_MS = 6 * 60 * 60 * 1000; // mirrors the service's 6h TTL
 
 // A minimal Frankfurter-shaped success response (array of { quote, rate }).
-const okResponse = (data: unknown) => ({ ok: true, json: async () => data });
+const okResponse = (data: unknown) => ({ ok: true, text: async () => JSON.stringify(data) });
 
 const svc = new ExchangeRatesService();
 
 let fetchMock: ReturnType<typeof vi.fn>;
+let errorSpy: ReturnType<typeof vi.spyOn>;
 
 beforeEach(() => {
   fetchMock = vi.fn(async () => okResponse([{ quote: 'USD', rate: 1.08 }]));
   vi.stubGlobal('fetch', fetchMock);
+  errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
 });
 
 afterEach(() => {
   vi.unstubAllGlobals();
   vi.useRealTimers();
+  errorSpy.mockRestore();
 });
 
 describe('ExchangeRatesService.getRates', () => {
   it('FX-SVC-001: falls back to EUR for a falsy base (|| coercion, not ??)', async () => {
     const rates = await svc.getRates('');
-    expect(fetchMock).toHaveBeenCalledWith('https://api.frankfurter.dev/v2/rates?base=EUR');
+    expect(fetchMock.mock.calls[0][0]).toBe('https://api.frankfurter.dev/v2/rates?base=EUR');
     expect(rates).toEqual({ EUR: 1, USD: 1.08 });
   });
 
   it('FX-SVC-002: upper-cases the base for the request and the self-rate seed', async () => {
     fetchMock.mockResolvedValueOnce(okResponse([{ quote: 'GBP', rate: 0.85 }]));
     const rates = await svc.getRates('usd');
-    expect(fetchMock).toHaveBeenCalledWith('https://api.frankfurter.dev/v2/rates?base=USD');
+    expect(fetchMock.mock.calls[0][0]).toBe('https://api.frankfurter.dev/v2/rates?base=USD');
     expect(rates).toEqual({ USD: 1, GBP: 0.85 });
   });
 
@@ -70,9 +75,10 @@ describe('ExchangeRatesService.getRates', () => {
     expect(await svc.getRates('SEK')).toBeNull();
   });
 
-  it('FX-SVC-006: returns null when fetch throws (silent catch, no logging)', async () => {
+  it('FX-SVC-006: returns null when fetch throws — and logs the failure (quirk fix)', async () => {
     fetchMock.mockRejectedValueOnce(new Error('offline'));
     expect(await svc.getRates('DKK')).toBeNull();
+    expect(errorSpy).toHaveBeenCalledWith('[exchange-rates] rates fetch failed for', 'DKK', expect.any(Error));
   });
 
   it('FX-SVC-007: treats a response that yields only the self-rate as failure (>1 keys heuristic)', async () => {
@@ -120,36 +126,16 @@ describe('ExchangeRatesService.getRates', () => {
     fetchMock.mockResolvedValueOnce({ ok: false, json: async () => [] });
     expect(await svc.getRates('BGN')).toBeNull();
   });
-});
 
-describe('ExchangeRatesService.convertWithRates', () => {
-  const rates = { EUR: 1, USD: 1.08, BAD: 0, NEG: -2 };
-
-  it('FX-SVC-013: identity when from equals base (case-insensitive)', () => {
-    expect(svc.convertWithRates(10, 'eur', 'EUR', rates)).toBe(10);
+  it('FX-SVC-021: sends the request with an abort-timeout signal (quirk fix)', async () => {
+    await svc.getRates('MXN');
+    const opts = fetchMock.mock.calls[0][1] as { signal?: unknown } | undefined;
+    expect(opts?.signal).toBeInstanceOf(AbortSignal);
   });
 
-  it('FX-SVC-014: identity when the rates map is null', () => {
-    expect(svc.convertWithRates(10, 'USD', 'EUR', null)).toBe(10);
-  });
-
-  it('FX-SVC-015: a falsy from means "already in base" (|| coercion) → identity', () => {
-    expect(svc.convertWithRates(10, null, 'EUR', rates)).toBe(10);
-    expect(svc.convertWithRates(10, undefined, 'EUR', rates)).toBe(10);
-    expect(svc.convertWithRates(10, '', 'EUR', rates)).toBe(10);
-  });
-
-  it('FX-SVC-016: identity when the from-currency has no rate', () => {
-    expect(svc.convertWithRates(10, 'JPY', 'EUR', rates)).toBe(10);
-  });
-
-  it('FX-SVC-017: identity for a zero or negative rate', () => {
-    expect(svc.convertWithRates(10, 'BAD', 'EUR', rates)).toBe(10);
-    expect(svc.convertWithRates(10, 'NEG', 'EUR', rates)).toBe(10);
-  });
-
-  it('FX-SVC-018: divides by the rate ("units of X per 1 base")', () => {
-    expect(svc.convertWithRates(10.8, 'USD', 'EUR', rates)).toBeCloseTo(10);
+  it('FX-SVC-022: treats an oversized response body as failure (quirk fix)', async () => {
+    fetchMock.mockResolvedValueOnce({ ok: true, text: async () => 'x'.repeat(1024 * 1024 + 1) });
+    expect(await svc.getRates('BRL')).toBeNull();
   });
 });
 
@@ -162,9 +148,4 @@ describe('exchange-rates.bridge', () => {
     expect(await bridgeGetRates('AUD')).toBe(primed);
     expect(fetchMock).toHaveBeenCalledTimes(1);
   });
-
-  it('FX-SVC-020: convertWithRates delegates 1:1', () => {
-    expect(bridgeConvertWithRates(21.6, 'USD', 'EUR', { EUR: 1, USD: 1.08 })).toBeCloseTo(20);
-    expect(bridgeConvertWithRates(5, 'EUR', 'EUR', null)).toBe(5);
-  });
 });