fix: store ai_insights with users.id and add FK + cascade delete

src/app/api/ai-insights/route.ts

@@ -2,18 +2,29 @@ import { NextResponse } from "next/server";
 import { getServerSession } from "next-auth";
 import { authOptions } from "@/lib/auth";
 import { supabaseAdmin } from "@/lib/supabase";
+import { resolveAppUser } from "@/lib/resolve-user";
 import { weeklyProductivityPrompt } from "@/lib/ai-prompts";
 import {
   analyzePatterns,
   computeTrends,
   DeveloperMetrics,
 } from "@/lib/ai-mentor";
+
 const aiInsightsRateLimit = new Map<
   string,
   { count: number; resetTime: number }
 >();
+
 export const dynamic = "force-dynamic";
 
+const VALID_INSIGHT_TYPES = new Set([
+  "weekly_summary",
+  "pattern",
+  "recommendation",
+] as const);
+
+type InsightType = "weekly_summary" | "pattern" | "recommendation";
+
 interface ContributionsApiResponse {
   data?: Record<string, number>;
   total?: number;
@@ -48,7 +59,18 @@ export async function GET(request: Request) {
     return NextResponse.json({ error: "Unauthorized" }, { status: 401 });
   }
 
-  const userId = session.githubId;
+  // Resolve the application user so that ai_insights rows are keyed on
+  // users.id (a stable UUID with a foreign-key constraint) rather than on
+  // session.githubId (a mutable external identifier with no FK). Using
+  // users.id ensures rows are removed via ON DELETE CASCADE when the account
+  // is deleted and prevents the orphaned-record scenario described in #1750.
+  const user = await resolveAppUser(session.githubId, session.githubLogin);
+  if (!user) {
+    return NextResponse.json({ error: "User not found" }, { status: 404 });
+  }
+
+  const userId = user.id;
+
   const currentTime = Date.now();
   const WINDOW_MS = 60 * 60 * 1000;
   const MAX_REQUESTS = 5;
@@ -73,8 +95,19 @@ export async function GET(request: Request) {
     );
   }
   existing.count += 1;
+
   const { searchParams } = new URL(request.url);
-  const type = searchParams.get("type") ?? "weekly_summary";
+  const rawType = searchParams.get("type") ?? "weekly_summary";
+
+  // Validate against the DB CHECK constraint allowlist so that an unrecognised
+  // type fails with a clear 400 instead of a Supabase constraint-violation 500.
+  if (!VALID_INSIGHT_TYPES.has(rawType as InsightType)) {
+    return NextResponse.json(
+      { error: `Invalid insight type. Must be one of: ${[...VALID_INSIGHT_TYPES].join(", ")}` },
+      { status: 400 }
+    );
+  }
+  const type = rawType as InsightType;
 
   const { data: cached } = await supabaseAdmin
     .from("ai_insights")

src/app/api/user/data-export/route.ts

@@ -319,6 +319,10 @@ export async function DELETE(req: NextRequest) {
 
   // Tables with a direct user_id foreign key, ordered to respect any
   // potential FK constraints (children before parents).
+  // ai_insights is included explicitly here even though ON DELETE CASCADE on
+  // the foreign key would remove those rows when the users row is deleted.
+  // The explicit delete is a defense-in-depth measure that works regardless
+  // of whether the FK migration has been applied to a given environment.
   const tablesToDelete = [
     "notifications",
     "ai_insights",

supabase/migrations/20260531000000_fix_ai_insights_user_id.sql

@@ -0,0 +1,52 @@
+-- Migration: fix ai_insights ownership and referential integrity
+--
+-- Problem
+-- -------
+-- ai_insights.user_id was populated with session.githubId (a numeric GitHub
+-- account ID stored as text, e.g. "12345678") instead of users.id (the
+-- application UUID, e.g. "550e8400-..."). As a result:
+--
+--   1. No foreign-key constraint existed, so rows could reference
+--      non-existent "users".
+--   2. ON DELETE CASCADE could not fire because there was no FK relationship,
+--      leaving ai_insights rows permanently orphaned after account deletion.
+--   3. The unique index on (user_id, insight_type) operated on GitHub IDs
+--      rather than application user IDs, making the constraint meaningless
+--      for ownership isolation.
+--
+-- This migration:
+--   1. Remaps existing user_id values from github_id → users.id using the
+--      users.github_id column that links the two identity spaces.
+--   2. Deletes any rows that cannot be mapped (accounts already deleted).
+--   3. Adds a proper FOREIGN KEY referencing users(id) ON DELETE CASCADE.
+--
+-- The unique index idx_ai_insights_user_type already covers (user_id,
+-- insight_type) and remains valid after the remap because every user has
+-- at most one insight row per type.
+
+BEGIN;
+
+-- Step 1: Remap github_id values to application UUIDs.
+--         ai_insights.user_id currently holds values equal to users.github_id.
+--         After this UPDATE each row will hold the corresponding users.id UUID.
+UPDATE ai_insights AS ai
+SET user_id = u.id
+FROM users AS u
+WHERE u.github_id = ai.user_id;
+
+-- Step 2: Remove rows that could not be matched (the originating user account
+--         no longer exists in the database). These are already orphaned records;
+--         deleting them here is correct and expected.
+DELETE FROM ai_insights
+WHERE user_id NOT IN (SELECT id FROM users);
+
+-- Step 3: Enforce referential integrity going forward.
+--         ON DELETE CASCADE ensures rows are removed automatically when their
+--         owning user account is deleted, preventing future orphans.
+ALTER TABLE ai_insights
+  ADD CONSTRAINT ai_insights_user_id_fkey
+  FOREIGN KEY (user_id)
+  REFERENCES users(id)
+  ON DELETE CASCADE;
+
+COMMIT;

test/ai-insights-ownership.test.ts

@@ -0,0 +1,247 @@
+import { beforeEach, describe, expect, it, vi } from "vitest";
+
+// ─── hoisted mocks ──────────────────────────────────────────────────────────
+
+const mocks = vi.hoisted(() => ({
+  getServerSession: vi.fn(),
+  resolveAppUser: vi.fn(),
+  supabaseFrom: vi.fn(),
+  fetch: vi.fn(),
+}));
+
+vi.mock("next-auth", () => ({ getServerSession: mocks.getServerSession }));
+vi.mock("@/lib/auth", () => ({ authOptions: {} }));
+vi.mock("@/lib/resolve-user", () => ({ resolveAppUser: mocks.resolveAppUser }));
+vi.mock("@/lib/supabase", () => ({
+  supabaseAdmin: { from: mocks.supabaseFrom },
+}));
+vi.mock("@/lib/ai-prompts", () => ({
+  weeklyProductivityPrompt: vi.fn().mockReturnValue("prompt"),
+}));
+vi.mock("@/lib/ai-mentor", () => ({
+  analyzePatterns: vi.fn().mockReturnValue([]),
+  computeTrends: vi.fn().mockReturnValue({ direction: "up", percentage: 5 }),
+}));
+
+vi.stubGlobal("fetch", mocks.fetch);
+
+// ─── helpers ────────────────────────────────────────────────────────────────
+
+const GITHUB_ID = "12345678";
+
+// Each test that exercises the request path uses a unique UUID so it gets its
+// own rate-limit bucket inside the module-level Map. This prevents the 5-req
+// per-hour cap from spilling across tests.
+let testCounter = 0;
+function freshUUID(): string {
+  testCounter += 1;
+  return `550e8400-e29b-41d4-a716-${String(testCounter).padStart(12, "0")}`;
+}
+
+function makeRequest(type = "weekly_summary"): Request {
+  return new Request(`http://localhost/api/ai-insights?type=${type}`, {
+    headers: { cookie: "next-auth.session-token=tok" },
+  });
+}
+
+function setupSupabaseCacheMiss() {
+  const upsertChain = vi.fn().mockResolvedValue({ error: null });
+
+  const maybeSingle = vi.fn().mockResolvedValue({ data: null, error: null });
+  const limit       = vi.fn().mockReturnValue({ maybeSingle });
+  const order       = vi.fn().mockReturnValue({ limit });
+  const gte         = vi.fn().mockReturnValue({ order });
+  const eqType      = vi.fn().mockReturnValue({ gte });
+  const eqUserId    = vi.fn().mockReturnValue({ eq: eqType });
+  const selectChain = vi.fn().mockReturnValue({ eq: eqUserId });
+
+  mocks.supabaseFrom.mockReturnValue({
+    select: selectChain,
+    upsert: upsertChain,
+  });
+
+  return { upsertChain, selectChain, eqUserId };
+}
+
+function setupSupabaseCacheHit(content: object) {
+  const maybeSingle = vi.fn().mockResolvedValue({ data: { content }, error: null });
+  const limit       = vi.fn().mockReturnValue({ maybeSingle });
+  const order       = vi.fn().mockReturnValue({ limit });
+  const gte         = vi.fn().mockReturnValue({ order });
+  const eqType      = vi.fn().mockReturnValue({ gte });
+  const eqUserId    = vi.fn().mockReturnValue({ eq: eqType });
+  const selectChain = vi.fn().mockReturnValue({ eq: eqUserId });
+
+  mocks.supabaseFrom.mockReturnValue({
+    select: selectChain,
+    upsert: vi.fn(),
+  });
+}
+
+function stubMetricsFetches() {
+  mocks.fetch.mockResolvedValue({
+    ok: true,
+    json: async () => ({}),
+    text: async () => "{}",
+    status: 200,
+  });
+}
+
+// ─── tests ───────────────────────────────────────────────────────────────────
+
+describe("GET /api/ai-insights — ownership model", () => {
+  beforeEach(() => {
+    vi.clearAllMocks();
+    stubMetricsFetches();
+
+    mocks.getServerSession.mockResolvedValue({
+      githubId: GITHUB_ID,
+      githubLogin: "alice",
+    });
+    // Default: resolved to a fresh UUID per test
+    mocks.resolveAppUser.mockResolvedValue({ id: freshUUID() });
+  });
+
+  // ── authentication ────────────────────────────────────────────────────────
+
+  it("returns 401 when there is no session", async () => {
+    mocks.getServerSession.mockResolvedValue(null);
+    const { GET } = await import("@/app/api/ai-insights/route");
+    const res = await GET(makeRequest());
+    expect(res.status).toBe(401);
+  });
+
+  it("returns 404 when resolveAppUser returns null", async () => {
+    mocks.resolveAppUser.mockResolvedValue(null);
+    const { GET } = await import("@/app/api/ai-insights/route");
+    const res = await GET(makeRequest());
+    expect(res.status).toBe(404);
+  });
+
+  // ── ownership: user.id not githubId ──────────────────────────────────────
+
+  it("calls resolveAppUser with the session githubId and githubLogin", async () => {
+    setupSupabaseCacheMiss();
+    const { GET } = await import("@/app/api/ai-insights/route");
+    await GET(makeRequest());
+    expect(mocks.resolveAppUser).toHaveBeenCalledWith(GITHUB_ID, "alice");
+  });
+
+  it("stores insights with users.id (UUID), not session.githubId (numeric string)", async () => {
+    const uuid = freshUUID();
+    mocks.resolveAppUser.mockResolvedValue({ id: uuid });
+    const { upsertChain } = setupSupabaseCacheMiss();
+    const { GET } = await import("@/app/api/ai-insights/route");
+    await GET(makeRequest());
+
+    expect(upsertChain).toHaveBeenCalledWith(
+      expect.objectContaining({ user_id: uuid }),
+      expect.any(Object)
+    );
+    // Must NOT use the GitHub numeric ID as the user_id
+    expect(upsertChain).not.toHaveBeenCalledWith(
+      expect.objectContaining({ user_id: GITHUB_ID }),
+      expect.any(Object)
+    );
+  });
+
+  it("queries the cache with users.id, not githubId", async () => {
+    const uuid = freshUUID();
+    mocks.resolveAppUser.mockResolvedValue({ id: uuid });
+    const { eqUserId } = setupSupabaseCacheMiss();
+    const { GET } = await import("@/app/api/ai-insights/route");
+    await GET(makeRequest());
+
+    // The first .eq() in the select chain filters by user_id
+    expect(eqUserId).toHaveBeenCalledWith("user_id", uuid);
+  });
+
+  // ── regression: orphaned-record scenario (#1750) ──────────────────────────
+
+  it("does not use githubId as the user_id in any DB operation", async () => {
+    const uuid = freshUUID();
+    mocks.resolveAppUser.mockResolvedValue({ id: uuid });
+    const { upsertChain } = setupSupabaseCacheMiss();
+    const { GET } = await import("@/app/api/ai-insights/route");
+    await GET(makeRequest());
+
+    const upsertArgs = upsertChain.mock.calls.flatMap((call) => call);
+    const upsertedIds = upsertArgs
+      .filter((a) => a && typeof a === "object" && "user_id" in a)
+      .map((a: any) => a.user_id);
+
+    expect(upsertedIds).not.toContain(GITHUB_ID);
+    upsertedIds.forEach((id: string) => expect(id).toBe(uuid));
+  });
+
+  // ── cache-hit path ────────────────────────────────────────────────────────
+
+  it("returns cached insight when a fresh record exists", async () => {
+    const cachedContent = { insights: [], trend: {}, aiSummary: null };
+    setupSupabaseCacheHit(cachedContent);
+    const { GET } = await import("@/app/api/ai-insights/route");
+    const res = await GET(makeRequest());
+    const body = await res.json();
+
+    expect(res.status).toBe(200);
+    expect(body.cached).toBe(true);
+    expect(body.data).toEqual(cachedContent);
+  });
+
+  // ── insight_type validation ───────────────────────────────────────────────
+
+  it("returns 400 for an unrecognised insight type", async () => {
+    setupSupabaseCacheMiss();
+    const { GET } = await import("@/app/api/ai-insights/route");
+    const res = await GET(makeRequest("totally_wrong"));
+    expect(res.status).toBe(400);
+    const body = await res.json();
+    expect(body.error).toMatch(/invalid insight type/i);
+  });
+
+  it("accepts weekly_summary as a valid type", async () => {
+    setupSupabaseCacheMiss();
+    const { GET } = await import("@/app/api/ai-insights/route");
+    const res = await GET(makeRequest("weekly_summary"));
+    expect(res.status).toBe(200);
+  });
+
+  it("accepts pattern as a valid type", async () => {
+    setupSupabaseCacheMiss();
+    const { GET } = await import("@/app/api/ai-insights/route");
+    const res = await GET(makeRequest("pattern"));
+    expect(res.status).toBe(200);
+  });
+
+  it("accepts recommendation as a valid type", async () => {
+    setupSupabaseCacheMiss();
+    const { GET } = await import("@/app/api/ai-insights/route");
+    const res = await GET(makeRequest("recommendation"));
+    expect(res.status).toBe(200);
+  });
+
+  // ── upsert conflict key ───────────────────────────────────────────────────
+
+  it("upserts with user_id,insight_type conflict target", async () => {
+    const { upsertChain } = setupSupabaseCacheMiss();
+    const { GET } = await import("@/app/api/ai-insights/route");
+    await GET(makeRequest());
+
+    expect(upsertChain).toHaveBeenCalledWith(
+      expect.any(Object),
+      { onConflict: "user_id,insight_type" }
+    );
+  });
+
+  // ── full cache-miss response shape ────────────────────────────────────────
+
+  it("returns { data, cached: false } on a cache miss", async () => {
+    setupSupabaseCacheMiss();
+    const { GET } = await import("@/app/api/ai-insights/route");
+    const res = await GET(makeRequest());
+    const body = await res.json();
+    expect(res.status).toBe(200);
+    expect(body.cached).toBe(false);
+    expect(body.data).toBeDefined();
+  });
+});