diff --git a/.squawk.toml b/.squawk.toml index 803672f9a..08b4689a7 100644 --- 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 index e3e151cae..dbd78d98a 100644 --- 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,10 +49,14 @@ async function loadFromApi(): Promise> { 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(); const logsByBranch = new Map(); if (logsResult.ok) { for (const log of logsResult.data.items) { + logsById.set(log.id, log); if (log.branch) { logsByBranch.set(log.branch, log); } @@ -60,7 +64,10 @@ async function loadFromApi(): Promise> { } 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> { 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 new file mode 100644 index 000000000..0f766088c --- /dev/null +++ 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 index df1257b94..2d5aec8b0 100644 --- 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 index 9881646b4..66bd228be 100644 --- 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([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 index 32c0e7dea..0ed8a2bc8 100644 --- 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; diff --git a/apps/wiki-server/src/routes/agent-sessions.ts b/apps/wiki-server/src/routes/agent-sessions.ts index ccea3937b..f354c0836 100644 --- 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 = { 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 index 703947308..d77dc0263 100644 --- 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 index 9d81d4fa0..d2df68ec8 100644 --- 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; @@ -614,7 +615,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 @@ -626,6 +627,7 @@ const PR_QUERY = `query($owner: String!, $name: String!) { }`; interface GqlReviewThread { + id: string; isResolved: boolean; isOutdated: boolean; path: string; @@ -684,6 +686,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, @@ -757,7 +760,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 @@ -1202,6 +1205,7 @@ function spawnClaude( }); } + async function fixPr(pr: ScoredPr, config: PatrolConfig): Promise { log(`${cl.bold}→${cl.reset} Fixing PR ${cl.cyan}#${pr.number}${cl.reset} (${pr.title})`); log(` Issues: ${cl.yellow}${pr.issues.join(', ')}${cl.reset}`); @@ -1254,6 +1258,12 @@ async function fixPr(pr: ScoredPr, config: PatrolConfig): Promise { }, }).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 = recordMaxTurnsFailure(pr.number); outcome = 'max-turns'; diff --git a/crux/wiki-server/sync-session.ts b/crux/wiki-server/sync-session.ts index 3d731da74..53851d2a5 100644 --- a/crux/wiki-server/sync-session.ts +++ b/crux/wiki-server/sync-session.ts @@ -20,6 +20,7 @@ import { fileURLToPath } from 'url'; import { parse as parseYaml } from 'yaml'; import { parseCliArgs } from '../lib/cli.ts'; import { createSession, type SessionApiEntry } from '../lib/wiki-server/sessions.ts'; +import { getAgentSessionByBranch, updateAgentSession } from '../lib/wiki-server/agent-sessions.ts'; const PAGE_ID_RE = /^[a-z0-9][a-z0-9-]*$/; @@ -112,13 +113,34 @@ export function parseSessionYaml(filePath: string): SessionApiEntry | null { /** * Sync a single session YAML file to the wiki-server. * Returns true if the POST succeeded, false otherwise. + * + * After successfully creating the session record, also updates the corresponding + * agent_session (matched by branch) to set session_id — establishing the FK link + * between the live tracking record and the historical session log. */ export async function syncSessionFile(filePath: string): Promise { const entry = parseSessionYaml(filePath); if (!entry) return false; const result = await createSession(entry); - return result.ok; + if (!result.ok) return false; + + // Best-effort: link the agent_session to the newly-created session log via FK. + // This replaces the fragile branch-name join used by the Agent Sessions dashboard. + if (entry.branch) { + try { + const agentSessionResult = await getAgentSessionByBranch(entry.branch); + if (agentSessionResult.ok && agentSessionResult.data.sessionId == null) { + await updateAgentSession(agentSessionResult.data.id, { + sessionId: result.data.id, + }); + } + } catch { + // Best-effort — local YAML is authoritative; FK linking is a nice-to-have + } + } + + return true; } // --------------------------------------------------------------------------- @@ -153,9 +175,9 @@ async function main() { console.log(` Branch: ${entry.branch || '(none)'}`); console.log(` Pages: ${entry.pages?.length || 0}`); - const result = await createSession(entry); - if (result.ok) { - console.log(`\u2713 Session synced to wiki-server (id: ${result.data.id})`); + const ok = await syncSessionFile(resolved); + if (ok) { + console.log(`\u2713 Session synced to wiki-server`); } else { console.log('Warning: could not sync session to wiki-server (server unavailable or error)'); // Not a hard failure — YAML is authoritative