fix-backend-review-findings (ornith-35): oauth empty-token guard, session-secret startup assertion, OAuth email-link test

Fixes from ornith's backend code review, implemented by ornith-35 (cross-review by qwopus35b pending). 129/129 tests green.
2026-06-29 23:33:02 -04:00
parent a303720c35
commit 2c7e7a0786
6 changed files with 526 additions and 0 deletions
@@ -0,0 +1,207 @@
+// Investor Flow — indicators.test.ts
+// Pure-logic tests for technical indicator functions. No HTTP, no filesystem, no DB.
+
+import { describe, it } from 'node:test';
+import assert from 'node:assert/strict';
+
+import { ema, rsi, relativeVolume, emaFromCandles } from '../indicators.ts';
+import type { PriceCandle } from '../../cache/CacheRepository.ts';
+
+describe('ema', () => {
+  it('returns undefined before period - 1, then a seeded SMA at period - 1', () => {
+    const values = [1, 2, 3, 4, 5]; // period = 3
+    const result = ema(values, 3);
+    assert.equal(result[0], undefined);
+    assert.equal(result[1], undefined);
+    // SMA of [1,2,3] = 2
+    assert.equal(result[2], 2);
+  });
+
+  it('computes a known EMA value correctly', () => {
+    // Known series: [10, 11, 12, 13, 14, 15, 16, 17, 18, 19]
+    // period = 3. SMA of [10,11,12] = 11.
+    // EMA at index 3:  (13 - 11) * (2/4) + 11 = 2*0.5 + 11 = 12
+    // EMA at index 4:  (14 - 12) * 0.5 + 12 = 13
+    // EMA at index 9:  hand-computed:
+    const values = [10, 11, 12, 13, 14, 15, 16, 17, 18, 19];
+    const result = ema(values, 3);
+    assert.equal(result[2], 11);
+    assert.equal(result[3], 12);
+    assert.equal(result[4], 13);
+    // Continue manually:
+    // i=5: (15-13)*0.5+13 = 14
+    assert.equal(result[5], 14);
+    // i=6: (16-14)*0.5+14 = 15
+    assert.equal(result[6], 15);
+    // i=7: (17-15)*0.5+15 = 16
+    assert.equal(result[7], 16);
+    // i=8: (18-16)*0.5+16 = 17
+    assert.equal(result[8], 17);
+    // i=9: (19-17)*0.5+17 = 18
+    assert.equal(result[9], 18);
+  });
+
+  it('returns all undefined when values.length < period', () => {
+    const result = ema([1, 2], 5);
+    assert.equal(result.length, 2);
+    for (const v of result) assert.equal(v, undefined);
+  });
+
+  it('throws on non-positive period', () => {
+    assert.throws(() => ema([1], 0));
+    assert.throws(() => ema([1], -1));
+  });
+
+  it('handles empty array', () => {
+    const result = ema([], 5);
+    assert.deepEqual(result, []);
+  });
+});
+
+describe('rsi', () => {
+  it('returns 100 on a strictly increasing series', () => {
+    // 20 closes, each +1 from the previous. All deltas = 1.
+    const closes: number[] = [];
+    for (let i = 0; i < 20; i++) closes.push(100 + i);
+    const result = rsi(closes, 14);
+    // The last value (index 19) should be 100.
+    assert.equal(result[19], 100);
+  });
+
+  it('returns 0 on a strictly decreasing series', () => {
+    const closes: number[] = [];
+    for (let i = 0; i < 20; i++) closes.push(100 - i);
+    const result = rsi(closes, 14);
+    assert.equal(result[19], 0);
+  });
+
+  it('returns undefined before the period-th close', () => {
+    const closes = [1, 2, 3, 4, 5]; // period=14 (default)
+    const result = rsi(closes);
+    for (let i = 0; i < closes.length; i++) {
+      assert.equal(result[i], undefined);
+    }
+  });
+
+  it('returns a value between 0 and 100 for mixed data', () => {
+    // 30 closes alternating up/down.
+    const closes: number[] = [];
+    for (let i = 0; i < 30; i++) closes.push(100 + Math.sin(i * 0.5) * 10);
+    const result = rsi(closes, 14);
+    // At index 29 (last), should be a number between 0 and 100.
+    const last = result[29];
+    assert.ok(last !== undefined);
+    assert.ok(last! >= 0 && last! <= 100, `RSI out of range: ${last}`);
+  });
+
+  it('throws on non-positive period', () => {
+    assert.throws(() => rsi([1, 2], 0));
+  });
+
+  it('uses default period of 14', () => {
+    const closes: number[] = [];
+    for (let i = 0; i < 20; i++) closes.push(100 + i);
+    const result = rsi(closes); // default 14
+    assert.equal(result[19], 100);
+  });
+});
+
+describe('relativeVolume', () => {
+  it('returns a ratio > 1 when the last volume exceeds the average', () => {
+    // 25 volumes: first 20 are ~100, last one is 300 (3x average).
+    const volumes: number[] = [];
+    for (let i = 0; i < 20; i++) volumes.push(100);
+    volumes.push(300); // index 20
+    const result = relativeVolume(volumes, 20);
+    // SMA of first 20 = 100. Ratio at index 20 = 300/100 = 3.
+    assert.equal(result[20], 3);
+  });
+
+  it('returns a ratio < 1 when the last volume is below average', () => {
+    const volumes: number[] = [];
+    for (let i = 0; i < 20; i++) volumes.push(100);
+    volumes.push(50); // index 20
+    const result = relativeVolume(volumes, 20);
+    assert.equal(result[20], 0.5);
+  });
+
+  it('returns undefined before the period is complete', () => {
+    const result = relativeVolume([1, 2, 3], 20);
+    for (let i = 0; i < result.length; i++) {
+      assert.equal(result[i], undefined);
+    }
+  });
+
+  it('uses a sliding window for subsequent values', () => {
+    // 25 volumes: [10,10,...,10] (20 times), then [5, 50].
+    const volumes: number[] = [];
+    for (let i = 0; i < 20; i++) volumes.push(10);
+    volumes.push(5);   // index 20: SMA=10, ratio=0.5
+    volumes.push(50);  // index 21: window slides, SMA = (10*19 + 5)/20 = 9.75, ratio = 50/9.75
+    const result = relativeVolume(volumes, 20);
+    assert.equal(result[20], 0.5);
+    // At index 21: sliding window is volumes[2..21] = 18*10 + 5 + 50 = 290, SMA = 14.5
+    // Wait: sum at index 20 = 200. sum += 50 - volumes[1](=10) => sum = 240. SMA = 12.
+    // Ratio = 50/12 ≈ 4.1667
+    const expectedRatio = 50 / 12;
+    assert.ok(Math.abs(result[21]! - expectedRatio) < 1e-9, `Expected ~${expectedRatio}, got ${result[21]}`);
+  });
+
+  it('throws on non-positive period', () => {
+    assert.throws(() => relativeVolume([1], 0));
+  });
+
+  it('handles empty array', () => {
+    const result = relativeVolume([], 5);
+    assert.deepEqual(result, []);
+  });
+});
+
+describe('emaFromCandles', () => {
+  it('uses close (c) by default', () => {
+    const candles: PriceCandle[] = [
+      { ts: '2024-01-01', o: 10, h: 12, l: 9, c: 11, v: 100 },
+      { ts: '2024-01-02', o: 11, h: 13, l: 10, c: 12, v: 100 },
+      { ts: '2024-01-03', o: 12, h: 14, l: 11, c: 13, v: 100 },
+    ];
+    const result = emaFromCandles(candles, 'c', 3);
+    // SMA of [11, 12, 13] = 12
+    assert.equal(result[2], 12);
+  });
+
+  it('uses adjClose when requested, falling back to c if null', () => {
+    const candles: PriceCandle[] = [
+      { ts: '2024-01-01', o: 10, h: 12, l: 9, c: 11, v: 100, adjClose: 10.5 },
+      { ts: '2024-01-02', o: 11, h: 13, l: 10, c: 12, v: 100, adjClose: 11.5 },
+      { ts: '2024-01-03', o: 12, h: 14, l: 11, c: 13, v: 100, adjClose: 12.5 },
+    ];
+    const result = emaFromCandles(candles, 'adjClose', 3);
+    // SMA of [10.5, 11.5, 12.5] = 11.5
+    assert.equal(result[2], 11.5);
+  });
+
+  it('falls back to c when adjClose is null', () => {
+    const candles: PriceCandle[] = [
+      { ts: '2024-01-01', o: 10, h: 12, l: 9, c: 11, v: 100, adjClose: null },
+      { ts: '2024-01-02', o: 11, h: 13, l: 10, c: 12, v: 100, adjClose: null },
+      { ts: '2024-01-03', o: 12, h: 14, l: 11, c: 13, v: 100, adjClose: null },
+    ];
+    const result = emaFromCandles(candles, 'adjClose', 3);
+    // Falls back to c: SMA of [11, 12, 13] = 12
+    assert.equal(result[2], 12);
+  });
+
+  it('returns undefined before period - 1', () => {
+    const candles: PriceCandle[] = [
+      { ts: '2024-01-01', o: 10, h: 12, l: 9, c: 11, v: 100 },
+      { ts: '2024-01-02', o: 11, h: 13, l: 10, c: 12, v: 100 },
+    ];
+    const result = emaFromCandles(candles, 'c', 5);
+    for (const v of result) assert.equal(v, undefined);
+  });
+
+  it('handles empty candles array', () => {
+    const result = emaFromCandles([], 'c', 5);
+    assert.deepEqual(result, []);
+  });
+});
@@ -0,0 +1,136 @@
+// Investor Flow — Pure technical-indicator functions.
+// Zero external dependencies. Operate on raw number arrays or PriceCandle[].
+
+import type { PriceCandle } from '../cache/CacheRepository.ts';
+
+/**
+ * Exponential Moving Average.
+ * Returns an array aligned to `values`, with `undefined` for indices before the
+ * seed SMA is complete (i.e. before index `period - 1`). At `period - 1` the
+ * SMA of the first `period` values seeds the EMA; subsequent values use the
+ * standard EMA multiplier `2 / (period + 1)`.
+ */
+export function ema(values: number[], period: number): (number | undefined)[] {
+  if (period <= 0) throw new Error(`period must be positive, got ${period}`);
+  const result: (number | undefined)[] = new Array(values.length);
+
+  if (values.length < period) {
+    return result; // all undefined
+  }
+
+  // Seed: SMA of the first `period` values.
+  let sum = 0;
+  for (let i = 0; i < period; i++) sum += values[i];
+  const sma = sum / period;
+  result[period - 1] = sma;
+
+  const multiplier = 2 / (period + 1);
+  let emaVal = sma;
+
+  for (let i = period; i < values.length; i++) {
+    emaVal = (values[i] - emaVal) * multiplier + emaVal;
+    result[i] = emaVal;
+  }
+
+  return result;
+}
+
+/**
+ * Relative Strength Index (Wilder's smoothing).
+ * Returns an array aligned to `closes`, with `undefined` for indices before
+ * the period-th close. Computes on price changes (close[i] - close[i-1]).
+ */
+export function rsi(closes: number[], period: number = 14): (number | undefined)[] {
+  if (period <= 0) throw new Error(`period must be positive, got ${period}`);
+  const result: (number | undefined)[] = new Array(closes.length);
+
+  if (closes.length < period + 1) {
+    return result; // all undefined — need at least `period` changes
+  }
+
+  // Compute price changes.
+  const deltas: number[] = new Array(closes.length - 1);
+  for (let i = 1; i < closes.length; i++) {
+    deltas[i - 1] = closes[i] - closes[i - 1];
+  }
+
+  // Wilder's smoothing: average gain / average loss over `period` changes.
+  let avgGain = 0;
+  let avgLoss = 0;
+
+  for (let i = 0; i < period; i++) {
+    if (deltas[i] > 0) avgGain += deltas[i];
+    else avgLoss += Math.abs(deltas[i]);
+  }
+
+  avgGain /= period;
+  avgLoss /= period;
+
+  // RSIs are defined starting at index `period` in the original closes array
+  // (because we need `period` changes, which starts at delta[0] = closes[1]-closes[0]).
+  result[period] = computeRsiFrom(avgGain, avgLoss);
+
+  for (let i = period; i < deltas.length; i++) {
+    const gain = deltas[i] > 0 ? deltas[i] : 0;
+    const loss = deltas[i] < 0 ? Math.abs(deltas[i]) : 0;
+    avgGain = (avgGain * (period - 1) + gain) / period;
+    avgLoss = (avgLoss * (period - 1) + loss) / period;
+    result[i + 1] = computeRsiFrom(avgGain, avgLoss);
+  }
+
+  return result;
+}
+
+function computeRsiFrom(avgGain: number, avgLoss: number): number {
+  if (avgLoss === 0) return 100;
+  const rs = avgGain / avgLoss;
+  return 100 - 100 / (1 + rs);
+}
+
+/**
+ * Relative Volume: current volume divided by the SMA of the previous `period`
+ * volumes. Returns `undefined` before the period is complete.
+ */
+export function relativeVolume(volumes: number[], period: number = 20): (number | undefined)[] {
+  if (period <= 0) throw new Error(`period must be positive, got ${period}`);
+  const result: (number | undefined)[] = new Array(volumes.length);
+
+  if (volumes.length < period + 1) {
+    return result; // all undefined — need `period` prior values to compute the SMA, plus 1 current
+  }
+
+  // The first `period` values form the initial SMA; the result at index `period`
+  // is volumes[period] / SMA(volumes[0..period-1]).
+  let sum = 0;
+  for (let i = 0; i < period; i++) sum += volumes[i];
+
+  result[period] = volumes[period] / (sum / period);
+
+  for (let i = period + 1; i < volumes.length; i++) {
+    sum += volumes[i] - volumes[i - period];
+    result[i] = volumes[i] / (sum / period);
+  }
+
+  return result;
+}
+
+/**
+ * Extract a price series from PriceCandle[] and compute an EMA.
+ * Defaults to the close (`c`) field; pass `"adjClose"` to use adjusted close.
+ */
+export function emaFromCandles(
+  candles: PriceCandle[],
+  priceKey: "c" | "adjClose" = "c",
+  period: number,
+): (number | undefined)[] {
+  const prices: number[] = new Array(candles.length);
+  for (let i = 0; i < candles.length; i++) {
+    const c = candles[i];
+    if (priceKey === "adjClose") {
+      prices[i] = c.adjClose ?? c.c; // fall back to close if adjClose is null
+    } else {
+      prices[i] = c.c;
+    }
+  }
+  return ema(prices, period);
+}
@@ -136,6 +136,9 @@ export async function exchangeCode(

  const tokenJson = (await tokenRes.json()) as Record<string, unknown>;
  const accessToken = String(tokenJson.access_token ?? '');
+  if (!accessToken) {
+    throw new Error('Provider returned an empty access_token (likely an error response).');
+  }

  // Userinfo lookup
  const userinfoRes = await fetchFn(config.userinfoUrl, {
@@ -0,0 +1,129 @@
+# 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).
@@ -173,6 +173,51 @@ test('oauth: oauthStart returns a redirect URL + CSRF state cookie; oauthCallbac
  }
 });

+// --- Slice 2c: OAuth — link existing account by email ---
+test('oauthCallback links an OAuth identity to an existing user when emails match', async () => {
+  const { db, cache } = setup();
+  process.env.GITHUB_CLIENT_ID = 'gh_id'; process.env.GITHUB_CLIENT_SECRET = 'gh_secret';
+  type Ctx = { db: typeof db; cache: typeof cache; resHeaders: Headers; userId: string | null; cookies: Record<string, string> };
+
+  // Step 1: sign up with email "link-me@example.com" (creates a password-based account).
+  const signupCtx: Ctx = { db, cache, resHeaders: new Headers(), userId: null, cookies: {} };
+  const signup = await appRouter.createCaller(signupCtx).auth.signup({ email: 'link-me@example.com', password: 'password123' });
+  assert.ok(signup.userId);
+
+  // Step 2: oauthStart to get a valid CSRF state + redirect URL.
+  const start = await appRouter.createCaller(signupCtx).auth.oauthStart({ provider: 'github', redirectUri: 'http://localhost/cb' });
+  const setCookie = signupCtx.resHeaders.get('set-cookie') ?? '';
+  // The oauth state cookie may be the 2nd Set-Cookie header; find it by prefix.
+  const stateMatch = setCookie.match(/iflow_oauth_state=([^;]+)/);
+  assert.ok(stateMatch, 'oauth state cookie should be set');
+  const stateVal = stateMatch[1];
+
+  // Step 3: oauthCallback with the SAME email from the provider -> should link, NOT create a new row.
+  const origFetch = global.fetch;
+  let calls = 0;
+  global.fetch = (async (url: unknown) => {
+    calls++;
+    const u = String(url);
+    if (u.includes('/access_token')) return new Response(JSON.stringify({ access_token: 'tok' }), { status: 200, headers: { 'content-type': 'application/json' } });
+    return new Response(JSON.stringify({ id: 999, email: 'link-me@example.com', name: 'Link User' }), { status: 200, headers: { 'content-type': 'application/json' } });
+  }) as typeof fetch;
+  try {
+    const cbCtx: Ctx = { db, cache, resHeaders: new Headers(), userId: null, cookies: { iflow_oauth_state: stateVal } };
+    const res = await appRouter.createCaller(cbCtx).auth.oauthCallback({ provider: 'github', code: 'abc', state: start.state, redirectUri: 'http://localhost/cb' });
+    assert.equal(res.userId, signup.userId, 'should return the EXISTING user id');
+    const u = db.prepare('SELECT oauth_subject,oauth_provider,pw_hash FROM users WHERE id=?').get(signup.userId) as { oauth_subject: string; oauth_provider: string; pw_hash: string };
+    assert.equal(u.oauth_subject, '999', 'oauth_subject updated');
+    assert.equal(u.oauth_provider, 'github', 'oauth_provider updated');
+    assert.ok(u.pw_hash.startsWith('scrypt$'), 'original pw_hash preserved as scrypt hash (linked account)');
+    assert.ok(cbCtx.resHeaders.get('set-cookie'), 'session cookie set');
+    // No duplicate rows: exactly 1 user.
+    assert.equal((db.prepare('SELECT COUNT(*) AS c FROM users').get() as { c: number }).c, 1, 'no duplicate user created');
+  } finally {
+    global.fetch = origFetch;
+    delete process.env.GITHUB_CLIENT_ID; delete process.env.GITHUB_CLIENT_SECRET;
+  }
+});
+
 // --- Slice 3: onboarding ---
 test('onboarding.complete writes complexity/risk/drawdown + starter watchlist + subscribes demand', async () => {
  const { db, cache, freshCtx } = setup();
@@ -7,6 +7,12 @@ import type { CacheRepository } from '../cache/CacheRepository.ts';

 export const SESSION_COOKIE = 'iflow_session';
 export const OAUTH_STATE_COOKIE = 'iflow_oauth_state';
+// Fail-fast: refuse to run with the unsafe default secret outside dev.
+const isDev = process.env.NODE_ENV === 'development' || process.env.NODE_ENV === undefined;
+if (!process.env.IFLOW_SESSION_SECRET && !isDev) {
+  throw new Error('IFLOW_SESSION_SECRET is not set and NODE_ENV is not "development". The default dev secret must never be used in production.');
+}
+
 const SESSION_SECRET = process.env.IFLOW_SESSION_SECRET ?? 'dev-secret-change-me';
 const SESSION_TTL_MS = 30 * 24 * 60 * 60 * 1000; // 30 days