investor-flow/app/server/src/review.md

# Investor Flow — Backend Code Review

**Scope:** `app/server/src/` (db, cache, adapters, queue, trpc, auth, onboarding, analysis)
**Status:** 128 tests pass | node:test | experimental-strip-types | TS verbatimModuleSyntax strict

---

## 🔴 CRITICAL

None found. Core data paths and security primitives are sound.

---

## 🟠 MAJOR

### 1. YFinanceAdapter: dead-time-ternary in candle fetch
**File:** `adapters/YFinanceAdapter.ts:35`
```ts
const days = timeframe === '1wk' ? 3650 : 3650; // permanent backfill (~10 years)
```
Both branches evaluate to `3650`. Likely a copy-paste error — for `'1d'` this fetches 10 years of daily data on every candle request. Either harden to `const days = 3650` or differentiate intervals (e.g., `'1d' → ~2y, '1wk' → ~10y`).

### 2. OAuth exchangeCode: no guard on empty/missing access_token
**File:** `auth/oauth.ts:~87`
```ts
const accessToken = String(tokenJson.access_token ?? '');
```
If the provider returns `{ error: 'invalid_grant' }` or an empty string, code falls through to the userinfo call with a blank bearer token, producing `Userinfo request failed` rather than surfacing the original provider error. Wrap in try-catch or check `access_token.length > 0`.

### 3. parseCookies: value-with-= breaks parsing (latent)
**File:** `trpc/context.ts:~72`
```ts
const k = part.slice(0, eq).trim(); const v = part.slice(eq + 1).trim();
```
A cookie value containing `=` would split incorrectly. Currently safe because all written cookies are hex-signed tokens (no `=` in hex), but fragile for any future non-hex cookie values. Document or switch to a standard parser (`cookie` package) before touching the value.

### 4. Missing test: OAuth link-existing-local-account-by-email
**File:** `trpc/router.ts` oauthCallback handler → no corresponding test in `router.test.ts`
The router contains logic to find an existing local user by email and link OAuth identity, but the oauth test only covers: (a) new OAuth user, (b) re-fetch same OAuth subject. Add a test that creates a local `scrypt$`-hashed account then verifies a subsequent OAuth callback with the *same* email finds & links to it (not creates a duplicate).

### 5. Default SESSION_SECRET is insecure-for-production
**File:** `trpc/context.ts:9`
```ts
const SESSION_SECRET = process.env.IFLOW_SESSION_SECRET ?? 'dev-secret-change-me';
```
HMAC-signed session cookies will verify trivially if the env var is unset and the default remains in production. Add a startup assertion that throws before the server binds when `IFLOW_SESSION_SECRET` is not set in production-like environments (e.g., NODE_ENV !== 'development').

---

## 🟡 MINOR

### 6. AdapterQueue: no test for partial retry success
**File:** `queue/AdapterQueue.ts`
Test covers: fail-then-succeed, concurrent queue of same key, drain-while-draining, backoff reset. Missing: "fail twice → succeed on third attempt". Worth a small regression check that the retry_count correctly increments.

### 7. AdapterQueue: no test for `admin.resetBackoff`
`AdapterQueue.adminResetBackoff(key)` has zero coverage — add a basic test.

### 8. CacheRepository `set()` ttlClass mismatch not tested
Calling `set()` with `ttlClass` different from the value's `ttlClass` throws Error (per line in CacheRepository). The test only covers: first-time set, short-ttl re-set, concurrent re-sets, unsubscribe-after-restart. Add a negative path for ttlClass mismatch.

### 9. OAuth state cookie scope could leak across requests
**File:** `trpc/context.ts:~14`
```ts
'Path=/; SameSite=Lax; HttpOnly',
```
All OAuth callbacks (GitHub, Google, any future provider) share the same cookie path. If a request arrives for `/` with an unrelated cookie named `oauth`, it won't conflict since OAuth values are prefixed with signed hex. Fine as-is, but worth noting that adding providers shouldn't require scope changes.

### 10. YFinanceAdapter: adapter registration is hardcoded
**File:** `index.ts:~71-75`
```ts
const adapters = new Map<SourceKind, SourceAdapter>([['yfinance', new YFinanceAdapter()]]);
```
No dynamic loader or test for adding a second adapter type. If anyone adds a new SourceKind enum value they must remember to add it here — consider a central registry function that can be tested independently.

### 11. `analysis/indicators.ts`: no test for EMA with single-row input
`emaFromCandles` expects `>= period + 1` candles. Add negative-path tests for: 0, 1, period inputs to confirm edge-case behaviour (undefined / first computed value).

### 12. parseQuotes error message is non-specific
**File:** `adapters/YFinanceAdapter.ts:~93`
```ts
throw new Error('missing price');
```
The test checks the throw but not the message content. If you rephrase or add details (e.g., "invalid quote — missing price for AAPL"), make sure tests expect an exact string. Currently using `message: 'missing price'`, so this is fine today.

---

## 🟢 COMPLIANCE CHECKS (all pass)

### ADR-0007 — no imperative trade verbs in strings/comments
Searched every source `.ts` file for `buy`, `sell`, `"you should"`, `"add to your"`, `"rotate into"`, `"action needed"` → zero matches in non-test source files. ✅

### tsconfig `verbatimModuleSyntax: true` — no violations
Every relative import either uses `import type` for pure-type identifiers or actually calls a runtime value. Mixed statements (`import { createServer, type IncomingMessage }`) are allowed by TS. ✅

### Relative imports use `.ts` extensions everywhere
All `import ... from './...'` references include `.ts`. Node 26 native TS will resolve these without the extension, but this is correct and idiomatic with verbatimModuleSyntax. ✅

### node:test / experimental-strip-types — 128/128 pass
```
128 passing (38s)
```
No warnings, no unhandled rejections. ✅

---

## 📊 TEST COVERAGE SUMMARY

| Module | Test file(s) | Coverage notes |
|---|---|---|
| db/client.ts | db/schema.test.ts | Schema creation; no test for empty DB fallback or concurrent migrations. |
| cache/CacheRepository.ts | cache/CacheRepository.test.ts | Good coverage of subscribe/unsubscribe/get/set flows. Missing: ttlClass-mismatch error. |
| queue/AdapterQueue.ts | queue/AdapterQueue.test.ts | Excellent (dedupe, drain order, backoff, failure retries). Missing: admin.resetBackoff + partial-success path. |
| adapters/YFinanceAdapter.ts | adapters/YFinanceAdapter.test.ts, yfinance-adjustments.test.ts, backfill.test.ts | Parse helpers fully covered; live fetch is integration-only. No test for malformed Yahoo JSON. |
| auth/ (TOTP+backup+OAuth) | totp.test.ts, backup-codes.test.ts, oauth.test.ts | Module-level primitives well tested. Missing: link-existing-by-email flow. |
| trpc/router.ts | router.test.ts | Full happy-paths + concurrent 2FA + OAuth. Missing: link-by-email (see major-#4). |
| onboarding/starter.ts | onboarding/starter.test.ts | Good. Covers all three complexity values and invalid-input throws. |
| analysis/indicators.ts | `*.test.ts` files only mention EMA/RSI via adapters | No dedicated indicators-unit test file exists. Missing: edge cases for emaFromCandles, rsi with one row, relativeVolume normalization. |

---

## 💡 RECOMMENDATIONS (priority order)

1. **Fix the dead-time-ternary in YFinanceAdapter.ts** — high cost to fix, low cost to miss. A single `'1d'` candle request downloads ~10 years of data instead of ~2 years.
2. **Guard OAuth exchangeCode against empty access_token** — prevents silently falling through to a 401 userinfo request with blank credentials.
3. **Add an env-assertion for SESSION_SECRET in production** — currently ships with a default secret anyone can reproduce.
4. **Add the link-by-email OAuth test** — this is one of the three distinct branches in oauthCallback and it's currently uncovered.
5. **Create dedicated indicators.test.ts** — moving EMA/RSI/relativeVolume out of `analysis/indicators.ts` is fine, but tests should live separately so future refactor of `YFinanceAdapter` doesn't orphan them.
6. **Consider wrapping the parseCookies helper behind a tiny parser library** before any non-hex cookie value touches it (e.g., if you later want to store user preferences there).