Merge pull request #1812 from quantified-uncertainty/claude/session-id-fk-consolidation

OAGr · web-flow · commit ef729c6152b2 · 2026-03-06T23:06:58.000-08:00
feat(sessions): add session_id FK to link agent_sessions to session logs (Phase 1 of #1668)
diff --git a/.squawk.toml b/.squawk.toml
@@ -22,12 +22,13 @@ pg_version = "16"
 #   by ensuring migrations only run once.
 #
 # require-concurrent-index-creation, disallowed-unique-constraint, constraint-missing-not-valid,
-# adding-not-nullable-field:
+# adding-not-nullable-field, adding-foreign-key-constraint:
 #   Drizzle's migrator runs ALL statements in a single transaction (see pg-core/dialect.js
-#   line 60). CREATE INDEX CONCURRENTLY cannot run inside a transaction, so these online-safe
-#   patterns are impossible with Drizzle. Tables are small (~700 rows) so brief locking is
-#   acceptable. If large-table migrations are needed in the future, use a separate raw SQL
-#   runner outside Drizzle.
+#   line 60). CREATE INDEX CONCURRENTLY cannot run inside a transaction, and the NOT VALID
+#   two-step pattern for FK constraints requires two separate transactions — both impossible
+#   with Drizzle. Tables are small (hundreds of rows) so brief locking is acceptable.
+#   If large-table migrations are needed in the future, use a separate raw SQL runner
+#   outside Drizzle (see .claude/rules/database-migrations.md for the manual migration pattern).
 excluded_rules = [
   "prefer-bigint-over-int",
   "prefer-identity",
@@ -38,4 +39,5 @@ excluded_rules = [
   "disallowed-unique-constraint",
   "constraint-missing-not-valid",
   "adding-not-nullable-field",
+  "adding-foreign-key-constraint",
 ]
diff --git a/apps/web/src/app/internal/agent-sessions/agent-sessions-content.tsx b/apps/web/src/app/internal/agent-sessions/agent-sessions-content.tsx
@@ -49,18 +49,25 @@ async function loadFromApi(): Promise<FetchResult<AgentSessionRow[]>> {
     revalidate: 60,
   });
 
-  // Build a branch → session log map for enrichment
+  // Build lookup maps for enrichment:
+  // 1. session_id → session log (direct FK, preferred for newer records)
+  // 2. branch → session log (fallback heuristic for older records without session_id)
+  const logsById = new Map<number, SessionRow>();
   const logsByBranch = new Map<string, SessionRow>();
   if (logsResult.ok) {
     for (const log of logsResult.data.items) {
+      logsById.set(log.id, log);
       if (log.branch) {
         logsByBranch.set(log.branch, log);
       }
     }
   }
 
   const rows: AgentSessionRow[] = agentResult.data.sessions.map((s): AgentSessionRow => {
-    const log = logsByBranch.get(s.branch);
+    // Prefer FK-linked session log; only fall back to branch heuristic when sessionId is absent
+    const log = s.sessionId != null
+      ? logsById.get(s.sessionId)
+      : logsByBranch.get(s.branch);
     return {
       id: s.id,
       branch: s.branch,
@@ -72,7 +79,7 @@ async function loadFromApi(): Promise<FetchResult<AgentSessionRow[]>> {
       startedAt: s.startedAt,
       completedAt: s.completedAt,
       // Prefer prUrl from agent_sessions (set by `crux issues done --pr=URL`),
-      // fall back to session log join on branch for older records.
+      // fall back to session log for older records.
       prUrl: s.prUrl ?? log?.prUrl ?? null,
       prOutcome: s.prOutcome ?? null,
       fixesPrUrl: s.fixesPrUrl ?? null,
diff --git a/apps/wiki-server/drizzle/0063_add_session_id_to_agent_sessions.sql b/apps/wiki-server/drizzle/0063_add_session_id_to_agent_sessions.sql
@@ -0,0 +1,20 @@
+-- Add session_id FK to agent_sessions, linking live tracking records to their
+-- corresponding historical session log.
+--
+-- This replaces the fragile branch-name join currently used by the Agent Sessions
+-- dashboard to enrich agent_sessions with cost/model data from the sessions table.
+-- With this FK, the dashboard can use a direct JOIN instead of a heuristic match.
+--
+-- The column is nullable because:
+-- 1. Older records predate this migration and won't have a session_id
+-- 2. Some agent sessions may never produce a session log (e.g., abandoned sessions)
+-- 3. Session logs can be created after agent sessions (at PR time)
+--
+-- Note: adding-foreign-key-constraint is excluded in .squawk.toml because Drizzle's
+-- migrator runs in a single transaction, making the two-step NOT VALID / VALIDATE
+-- pattern impossible. agent_sessions is a small table (hundreds of rows), so the
+-- brief SHARE ROW EXCLUSIVE lock is acceptable.
+
+ALTER TABLE "agent_sessions" ADD COLUMN IF NOT EXISTS "session_id" bigint REFERENCES "sessions"("id") ON DELETE SET NULL;
+
+CREATE INDEX IF NOT EXISTS "idx_as_session_id" ON "agent_sessions" ("session_id");
diff --git a/apps/wiki-server/drizzle/meta/_journal.json b/apps/wiki-server/drizzle/meta/_journal.json
@@ -442,6 +442,13 @@
       "when": 1773993600000,
       "tag": "0062_add_numeric_cost_duration_to_sessions",
       "breakpoints": true
+    },
+    {
+      "idx": 63,
+      "version": "7",
+      "when": 1774080000000,
+      "tag": "0063_add_session_id_to_agent_sessions",
+      "breakpoints": true
     }
   ]
 }
diff --git a/apps/wiki-server/src/__tests__/agent-sessions.test.ts b/apps/wiki-server/src/__tests__/agent-sessions.test.ts
@@ -6,6 +6,11 @@ import {
   postJson,
 } from "./test-utils";
 
+// ---- In-memory sessions fixture (simulates FK target) ----
+// Pre-seeded IDs that are valid FK targets for session_id.
+// Tests that use sessionId: 42 must find it here; others must not.
+const VALID_SESSION_IDS = new Set<number>([42]);
+
 // ---- In-memory store simulating agent_sessions table ----
 
 let nextId = 1;
@@ -16,6 +21,7 @@ let store: Array<{
   session_type: string;
   issue_number: number | null;
   checklist_md: string;
+  session_id: number | null;
   status: string;
   started_at: Date;
   completed_at: Date | null;
@@ -48,6 +54,7 @@ const dispatch: SqlDispatcher = (query, params) => {
       session_type: params[2] as string,
       issue_number: params[3] as number | null,
       checklist_md: params[4] as string,
+      session_id: null,
       status: "active",
       started_at: new Date(),
       completed_at: null,
@@ -88,6 +95,17 @@ const dispatch: SqlDispatcher = (query, params) => {
           case "checklist_md":
             store[idx].checklist_md = params[pIdx] as string;
             break;
+          case "session_id": {
+            const sid = params[pIdx] as number | null;
+            if (sid !== null && !VALID_SESSION_IDS.has(sid)) {
+              // Simulate FK constraint violation — the route translates this to 400.
+              throw new Error(
+                `insert or update on table "agent_sessions" violates foreign key constraint`
+              );
+            }
+            store[idx].session_id = sid;
+            break;
+          }
           case "status":
             store[idx].status = params[pIdx] as string;
             break;
@@ -486,6 +504,61 @@ describe("Agent Sessions API", () => {
       });
       expect(res.status).toBe(400);
     });
+
+    it("sets sessionId FK link to session log", async () => {
+      await postJson(app, "/api/agent-sessions", sampleSession);
+
+      const res = await patchJson(app, "/api/agent-sessions/1", {
+        sessionId: 42,
+      });
+      expect(res.status).toBe(200);
+      const body = await res.json();
+      expect(body.sessionId).toBe(42);
+    });
+
+    it("clears sessionId with null", async () => {
+      await postJson(app, "/api/agent-sessions", sampleSession);
+
+      // Set it first
+      await patchJson(app, "/api/agent-sessions/1", { sessionId: 42 });
+
+      // Now clear it
+      const res = await patchJson(app, "/api/agent-sessions/1", {
+        sessionId: null,
+      });
+      expect(res.status).toBe(200);
+      const body = await res.json();
+      expect(body.sessionId).toBeNull();
+    });
+
+    it("rejects non-integer sessionId", async () => {
+      await postJson(app, "/api/agent-sessions", sampleSession);
+
+      const res = await patchJson(app, "/api/agent-sessions/1", {
+        sessionId: 1.5,
+      });
+      expect(res.status).toBe(400);
+    });
+
+    it("rejects zero sessionId", async () => {
+      await postJson(app, "/api/agent-sessions", sampleSession);
+
+      const res = await patchJson(app, "/api/agent-sessions/1", {
+        sessionId: 0,
+      });
+      expect(res.status).toBe(400);
+    });
+
+    it("returns 400 invalid_reference for non-existent sessionId", async () => {
+      await postJson(app, "/api/agent-sessions", sampleSession);
+
+      const res = await patchJson(app, "/api/agent-sessions/1", {
+        sessionId: 9999, // not in VALID_SESSION_IDS — FK violation
+      });
+      expect(res.status).toBe(400);
+      const body = await res.json();
+      expect(body.error).toBe("invalid_reference");
+    });
   });
 
   // ================================================================
diff --git a/apps/wiki-server/src/api-types.ts b/apps/wiki-server/src/api-types.ts
@@ -1174,6 +1174,8 @@ export const UpdateAgentSessionSchema = z.object({
   prUrl: z.string().url().max(1000).nullable().optional(),
   prOutcome: z.enum(PR_OUTCOMES).nullable().optional(),
   fixesPrUrl: z.string().url().max(1000).nullable().optional(),
+  /** FK to the sessions table — set when the session log is synced to the DB */
+  sessionId: z.number().int().positive().nullable().optional(),
 });
 export type UpdateAgentSession = z.infer<typeof UpdateAgentSessionSchema>;
 
diff --git a/apps/wiki-server/src/routes/agent-sessions.ts b/apps/wiki-server/src/routes/agent-sessions.ts
@@ -115,15 +115,16 @@ const agentSessionsApp = new Hono()
     const parsed = UpdateAgentSessionSchema.safeParse(body);
     if (!parsed.success) return validationError(c, parsed.error.message);
 
-    const { checklistMd, status, prUrl, prOutcome, fixesPrUrl } = parsed.data;
+    const { checklistMd, status, prUrl, prOutcome, fixesPrUrl, sessionId } = parsed.data;
     if (
       checklistMd === undefined &&
       status === undefined &&
       prUrl === undefined &&
       prOutcome === undefined &&
-      fixesPrUrl === undefined
+      fixesPrUrl === undefined &&
+      sessionId === undefined
     ) {
-      return validationError(c, "At least one of checklistMd, status, prUrl, prOutcome, or fixesPrUrl must be provided");
+      return validationError(c, "At least one of checklistMd, status, prUrl, prOutcome, fixesPrUrl, or sessionId must be provided");
     }
 
     const updates: Record<string, unknown> = { updatedAt: new Date() };
@@ -137,13 +138,34 @@ const agentSessionsApp = new Hono()
     if (prUrl !== undefined) updates.prUrl = prUrl;
     if (prOutcome !== undefined) updates.prOutcome = prOutcome;
     if (fixesPrUrl !== undefined) updates.fixesPrUrl = fixesPrUrl;
+    if (sessionId !== undefined) updates.sessionId = sessionId;
 
     const db = getDrizzleDb();
-    const result = await db
-      .update(agentSessions)
-      .set(updates)
-      .where(eq(agentSessions.id, id))
-      .returning();
+    let result;
+    try {
+      result = await db
+        .update(agentSessions)
+        .set(updates)
+        .where(eq(agentSessions.id, id))
+        .returning();
+    } catch (e: unknown) {
+      // Drizzle wraps DB errors in DrizzleQueryError; the FK message lives in e.cause.
+      const msg = e instanceof Error ? e.message : String(e);
+      const causeMsg =
+        e instanceof Error && e.cause instanceof Error ? e.cause.message : "";
+      const isFkViolation =
+        msg.includes("foreign key") ||
+        msg.includes("violates foreign key") ||
+        causeMsg.includes("foreign key") ||
+        causeMsg.includes("violates foreign key");
+      if (isFkViolation) {
+        return c.json(
+          { error: "invalid_reference", message: "sessionId references a non-existent session" },
+          400,
+        );
+      }
+      throw e;
+    }
 
     if (result.length === 0) {
       return c.json({ error: "not_found", message: `No session with id: ${id}` }, 404);
diff --git a/apps/wiki-server/src/schema.ts b/apps/wiki-server/src/schema.ts
@@ -762,6 +762,7 @@ export const agentSessions = pgTable(
     prUrl: text("pr_url"), // PR URL recorded when crux issues done --pr=URL is called
     prOutcome: text("pr_outcome"), // Outcome: merged | merged_with_revisions | reverted | closed_without_merge
     fixesPrUrl: text("fixes_pr_url"), // URL of the PR this session is fixing (enables fix-chain tracking)
+    sessionId: bigint("session_id", { mode: "number" }).references(() => sessions.id, { onDelete: "set null" }), // FK to session log — set when session completes and log is synced
     status: text("status").notNull().default("active"),
     startedAt: timestamp("started_at", { withTimezone: true })
       .notNull()
@@ -779,6 +780,7 @@ export const agentSessions = pgTable(
     index("idx_as_status").on(table.status),
     index("idx_as_issue").on(table.issueNumber),
     index("idx_as_started_at").on(table.startedAt),
+    index("idx_as_session_id").on(table.sessionId),
   ]
 );
 
diff --git a/crux/pr-patrol/index.ts b/crux/pr-patrol/index.ts
@@ -28,6 +28,7 @@ export type PrIssueType =
   | 'bot-review-nitpick';
 
 interface BotComment {
+  threadId: string;
   path: string;
   line: number | null;
   startLine: number | null;
@@ -662,7 +663,7 @@ const PR_QUERY = `query($owner: String!, $name: String!) {
           }}
         }}}}
         reviewThreads(first: 50) { nodes {
-          isResolved isOutdated path line startLine
+          id isResolved isOutdated path line startLine
           comments(first: 3) { nodes {
             author { login }
             body
@@ -674,6 +675,7 @@ const PR_QUERY = `query($owner: String!, $name: String!) {
 }`;
 
 interface GqlReviewThread {
+  id: string;
   isResolved: boolean;
   isOutdated: boolean;
   path: string;
@@ -732,6 +734,7 @@ function extractBotComments(pr: GqlPrNode): BotComment[] {
     if (!KNOWN_BOT_LOGINS.has(firstComment.author.login)) continue;
 
     comments.push({
+      threadId: thread.id,
       path: thread.path,
       line: thread.line,
       startLine: thread.startLine,
@@ -805,7 +808,7 @@ const SINGLE_PR_QUERY = `query($owner: String!, $name: String!, $number: Int!) {
         }}
       }}}}
       reviewThreads(first: 50) { nodes {
-        isResolved isOutdated path line startLine
+        id isResolved isOutdated path line startLine
         comments(first: 3) { nodes {
           author { login }
           body
@@ -1291,6 +1294,7 @@ function spawnClaude(
   });
 }
 
+
 async function fixPr(pr: ScoredPr, config: PatrolConfig): Promise<void> {
   log(`${cl.bold}→${cl.reset} Fixing PR ${cl.cyan}#${pr.number}${cl.reset} (${pr.title})`);
   log(`  Issues: ${cl.yellow}${pr.issues.join(', ')}${cl.reset}`);
@@ -1383,6 +1387,12 @@ async function fixPr(pr: ScoredPr, config: PatrolConfig): Promise<void> {
           },
         }).catch(() => log('  Warning: could not post summary comment'));
       }
+
+      // Note: we intentionally do NOT auto-resolve bot review threads here.
+      // A zero exit code only means Claude ran without crashing — it does not
+      // guarantee every open thread was addressed. Blanket resolution would
+      // dismiss valid feedback that wasn't actually fixed. Threads are resolved
+      // by CodeRabbit re-review after the fix is pushed, or manually.
     } else if (result.hitMaxTurns) {
       const failCount = recordFailure(pr.number);
       outcome = 'max-turns';
diff --git a/crux/wiki-server/sync-session.ts b/crux/wiki-server/sync-session.ts

Original file line number	Diff line number	Diff line change
`@@ -442,6 +442,13 @@`
`442`	`442`	`"when": 1773993600000,`
`443`	`443`	`"tag": "0062_add_numeric_cost_duration_to_sessions",`
`444`	`444`	`"breakpoints": true`
	`445`	`+ },`
	`446`	`+ {`
	`447`	`+ "idx": 63,`
	`448`	`+ "version": "7",`
	`449`	`+ "when": 1774080000000,`
	`450`	`+ "tag": "0063_add_session_id_to_agent_sessions",`
	`451`	`+ "breakpoints": true`
`445`	`452`	`}`
`446`	`453`	`]`
`447`	`454`	`}`