feat(sessions): add session_id FK to link agent_sessions to session logs (Phase 1 of #1668)#1812
feat(sessions): add session_id FK to link agent_sessions to session logs (Phase 1 of #1668)#1812
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis PR establishes a foreign key relationship between agent_sessions and sessions tables by adding a nullable session_id column with index. It updates database schemas, API validation, data enrichment logic with a two-tier lookup mechanism (sessionId first, then branch fallback), and includes sync logic to populate the FK relationship when sessions are created. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🛑 Protected Paths ModifiedThis PR modifies 1 protected file(s) that control agent behavior, CI pipelines, or validation logic. These changes require human review before merging. Linter config (.squawk.toml)
Action required: Add the
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/wiki-server/src/routes/agent-sessions.ts (1)
141-148:⚠️ Potential issue | 🟠 MajorHandle foreign-key constraint violation for invalid
sessionIdvalues.The
PATCH /api/agent-sessions/:idendpoint accepts asessionIdparameter (line 141) and writes it directly to the database without validating that the session exists. Since the schema enforces a foreign key constraint fromagent_sessions.session_idtosessions.id, an invalidsessionIdwill trigger a database FK violation. Without explicit error handling, this will surface as a generic 500 error instead of a deterministic 4xx response. Either validate the target session before the update or catch and translate the FK violation into a stable 4xx response.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/wiki-server/src/routes/agent-sessions.ts` around lines 141 - 148, The PATCH handler in agent-sessions.ts writes sessionId directly into agent_sessions (see the sessionId check and the db.update call using getDrizzleDb() and agentSessions) without ensuring the referenced session exists, which can cause a DB foreign-key error; fix it by either (A) validating the sessionId before updating: query the sessions table (sessions) for the given sessionId and return a 404/400 if not found, then proceed with the update, or (B) wrap the db.update(...).returning() in a try/catch, detect a foreign-key constraint violation from the database error and translate it into a deterministic 4xx response (400 or 404) instead of letting it bubble as a 500—apply the approach that matches the codebase error-handling conventions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/app/internal/agent-sessions/agent-sessions-content.tsx`:
- Around line 64-66: The current assignment for `log` uses `logsByBranch` as a
fallback even when `s.sessionId` is present, which can attach another session’s
metadata; change the `log` resolution so that when `s.sessionId != null` you
only use `logsById.get(s.sessionId)` (even if undefined) and do not fall back to
`logsByBranch`, and only run the branch-name heuristic
(`logsByBranch.get(s.branch)`) when `s.sessionId` is null; update the `log`
variable assignment in agent-sessions-content (the line that references
`s.sessionId`, `logsById`, and `logsByBranch`) accordingly.
In `@apps/wiki-server/drizzle/0063_add_session_id_to_agent_sessions.sql`:
- Around line 18-20: The migration currently uses `IF NOT EXISTS` which masks
schema drift; update the statements in migration 0063 to remove `IF NOT EXISTS`
so the DDL fails loudly if a stray `session_id` column or `idx_as_session_id`
index already exists. Specifically, change the ALTER TABLE on "agent_sessions"
to add the "session_id" integer REFERENCES "sessions"("id") ON DELETE SET NULL
without `IF NOT EXISTS`, and create "idx_as_session_id" without `IF NOT EXISTS`,
so conflicts surface during migration application and preserve the intended FK
and ON DELETE behavior.
In `@apps/wiki-server/src/schema.ts`:
- Line 765: Change the sessionId column definition to match sessions.id by
replacing integer("session_id") with bigint("session_id", { mode: "number" }) in
the schema (preserve the .references(() => sessions.id, { onDelete: "set null"
}) part); also update the corresponding migration
(0063_add_session_id_to_agent_sessions.sql) to create the session_id column as
BIGINT instead of INTEGER so the FK types align.
In `@crux/wiki-server/sync-session.ts`:
- Around line 132-136: getAgentSessionByBranch() can return a non-unique branch
row and updateAgentSession(...) unconditionally clobbers sessionId; change the
client to skip any agent session rows that already have a sessionId (check
agentSessionResult.data.sessionId) before calling updateAgentSession, only PATCH
when sessionId is null/undefined, and add a TODO comment to implement a
server-side compare-and-set for sessionId to make this atomic later.
- Around line 121-143: Replace the direct createSession(...) call in main() with
a call to syncSessionFile(resolved) so the FK-linking logic in syncSessionFile
is executed; adjust syncSessionFile to return the created session result (or an
object with ok and sessionId) instead of only boolean (so callers can inspect
result.ok and result.sessionId), and update main() to check result.ok and log
the created session id (e.g. "✓ Session synced to wiki-server (id: ...)" ) when
successful; refer to functions syncSessionFile, createSession, and main to
locate the changes.
---
Outside diff comments:
In `@apps/wiki-server/src/routes/agent-sessions.ts`:
- Around line 141-148: The PATCH handler in agent-sessions.ts writes sessionId
directly into agent_sessions (see the sessionId check and the db.update call
using getDrizzleDb() and agentSessions) without ensuring the referenced session
exists, which can cause a DB foreign-key error; fix it by either (A) validating
the sessionId before updating: query the sessions table (sessions) for the given
sessionId and return a 404/400 if not found, then proceed with the update, or
(B) wrap the db.update(...).returning() in a try/catch, detect a foreign-key
constraint violation from the database error and translate it into a
deterministic 4xx response (400 or 404) instead of letting it bubble as a
500—apply the approach that matches the codebase error-handling conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7099bbdf-c374-4515-af5b-c63c2056597c
📒 Files selected for processing (9)
.squawk.tomlapps/web/src/app/internal/agent-sessions/agent-sessions-content.tsxapps/wiki-server/drizzle/0063_add_session_id_to_agent_sessions.sqlapps/wiki-server/drizzle/meta/_journal.jsonapps/wiki-server/src/__tests__/agent-sessions.test.tsapps/wiki-server/src/api-types.tsapps/wiki-server/src/routes/agent-sessions.tsapps/wiki-server/src/schema.tscrux/wiki-server/sync-session.ts
apps/web/src/app/internal/agent-sessions/agent-sessions-content.tsx
Outdated
Show resolved
Hide resolved
apps/wiki-server/drizzle/0063_add_session_id_to_agent_sessions.sql
Outdated
Show resolved
Hide resolved
OAGr
added
claude-working
Phase 1 of consolidating overlapping session tracking systems (#1668). The Agent Sessions dashboard (E912) joins data from `agent_sessions` and `sessions` tables using a fragile branch-name heuristic — same branch name could match across unrelated sessions, and cost/model data had to be fetched separately. - Add `session_id INTEGER REFERENCES sessions(id)` to `agent_sessions` table (nullable, ON DELETE SET NULL) with index `idx_as_session_id` - Migration: `0063_add_session_id_to_agent_sessions.sql` - `UpdateAgentSessionSchema` now accepts `sessionId` field (nullable int) - PATCH `/api/agent-sessions/:id` handles `sessionId` updates - `sessionId` is included in all agent session responses - `crux wiki-server sync-session` now links the agent_session to the newly-created session log via FK after successful session sync - Best-effort: silently skips if agent session not found - Agent Sessions dashboard prefers FK-linked session log (`s.sessionId`) over branch-name heuristic; falls back to branch join for older records - Dashboard enrichment is now based on direct FK join when available - Eliminates ambiguous branch-name matching for newer sessions - Older records continue to work via branch-name fallback - Does not drop any tables (data preserved) - Does not merge active_agents into agent_sessions - Does not backfill historical records (too many unknowns) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Drizzle's migrator runs all statements in a single transaction, making the two-step NOT VALID / VALIDATE pattern (squawk's recommended fix for adding-foreign-key-constraint) impossible. Excluded the rule with the same justification as the existing exclusions for require-concurrent-index-creation and constraint-missing-not-valid. agent_sessions is a small table (~hundreds of rows) so the brief SHARE ROW EXCLUSIVE lock during migration is acceptable. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Resolve merge conflict with pagination changes (fetchAllPaginated) - Fix type mismatch: session_id column integer→bigint to match sessions.id - Fix dashboard fallback: don't fall back to branch heuristic when sessionId is set but not found (prevents attaching wrong session metadata) - Fix CLI: main() now uses syncSessionFile() to include FK linking - Add clobber guard: skip FK update if agent session already has sessionId - Add FK constraint error handling in PATCH endpoint (400 vs 500) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
OAGr
force-pushed
the
claude/session-id-fk-consolidation
branch
from
5ce8f99 to
e800b18
Compare
🛑 CodeRabbit Security GateAction required: This PR has 2 unresolved Critical/Major CodeRabbit finding(s). Either resolve each thread in the GitHub review interface, or add the Unresolved Critical/Major Findings (2)
|
1 participant
Summary
Phase 1 of consolidating the 3 overlapping session tracking systems (issue #1668).
After investigation, there are actually 4 tables tracking agent activity:
sessions— historical logs (written at session end, immutable)agent_sessions— live checklist tracking (mutable during session)active_agents— real-time coordination (heartbeat-based, ephemeral)agent_session_events— timeline for active_agentsThe Agent Sessions dashboard (E912) currently joins
agent_sessionsandsessionsby branch name — a fragile heuristic that can mismatch when a branch is reused, and loses data when branch names differ slightly.Changes
Schema (
apps/wiki-server/src/schema.ts):session_id INTEGER REFERENCES sessions(id)toagent_sessions(nullable, ON DELETE SET NULL)idx_as_session_idMigration (
0063_add_session_id_to_agent_sessions.sql):ALTER TABLE agent_sessions ADD COLUMN IF NOT EXISTS session_id integer REFERENCES sessions(id) ON DELETE SET NULLAPI (
apps/wiki-server/src/api-types.ts,routes/agent-sessions.ts):UpdateAgentSessionSchemaacceptssessionId(nullable int)sessionIdupdatessessionIdCLI (
crux/wiki-server/sync-session.ts):sessionIdFK on the corresponding agent_session (matched by branch)Dashboard (
apps/web/src/app/internal/agent-sessions/agent-sessions-content.tsx):session_idTests (
apps/wiki-server/src/__tests__/agent-sessions.test.ts):sessionIdPATCH operations (set, clear, reject non-integer, reject zero)What this does NOT do
Why not full merge?
Full merge was considered and rejected because:
sessionsuses(date, title)unique key;agent_sessionsuses(branch, status)— incompatiblesessionsrecords predateagent_sessions(no corresponding live tracking)The FK link achieves the main goal (eliminating fragile branch-name join) without the risk.
Test plan
Partial: #1668
Summary by CodeRabbit
New Features
Documentation