Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a nullable FK column Changes
Sequence Diagram(s)sequenceDiagram
participant Web as Web UI
participant API as WikiServer API
participant DB as Database
participant Sync as Sync Service
Web->>API: PATCH /agent-sessions { id, sessionId? ... }
API->>DB: UPDATE agent_sessions SET session_id = $id
DB-->>API: success / foreign-key error
API-->>Web: 200 OK / 400 { error: "invalid_reference" }
Sync->>API: createSession (file)
API->>DB: INSERT INTO sessions(...)
DB-->>API: session.id
API->>DB: SELECT agent_sessions WHERE branch = $branch
DB-->>API: agent_session row (if found)
API->>DB: UPDATE agent_sessions SET session_id = session.id
DB-->>API: success (errors swallowed)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.
|
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
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>
5ce8f99 to
e800b18
Compare
✅ CodeRabbit Security GateNo unresolved Critical or Major CodeRabbit findings. Check passes.
|
After PR Patrol successfully fixes bot-review issues and pushes, it now resolves the corresponding review threads via GraphQL. This prevents the CodeRabbit Security Gate from failing on stale unresolved threads. Changes: - Add thread ID to GraphQL queries and BotComment interface - New resolveBotReviewThreads() function called after successful fix - Best-effort: failures logged but don't block the fix outcome Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/wiki-server/src/__tests__/agent-sessions.test.ts`:
- Around line 93-95: The mock dispatcher currently assigns any numeric
session_id in the "case \"session_id\"" branch (store[idx].session_id =
params[pIdx]) which allows invalid foreign keys; update this branch to enforce
the FK by either validating the assigned id exists in the test sessions fixture
(e.g., check against the in-memory sessions array used by the tests) and
throw/return the same error path the real schema would (invalid_reference/400)
when missing, or alternatively seed a minimal sessions fixture with the expected
session id(s) and assert existence before assignment; reference the "case
\"session_id\"" switch and the test's sessions fixture/in-memory sessions array
to locate where to add the check.
In `@apps/wiki-server/src/routes/agent-sessions.ts`:
- Around line 145-156: The catch block around the db.update(...).returning()
currently inspects the error message string to detect a foreign key violation;
update it to check the PostgreSQL SQLSTATE code instead by testing the caught
error object (e) for a code property equal to "23503" (e.g., use a type guard
like (e as { code?: string }).code === "23503") and keep the same response for
that case; ensure you still derive msg for other logging/handling but stop using
msg.includes("foreign key") or similar string checks.
In `@crux/pr-patrol/index.ts`:
- Around line 1208-1227: The current resolveBotReviewThreads function
unconditionally attempts to resolve every thread in the provided botComments
list, which can close valid unresolved feedback; change the logic so
resolveBotReviewThreads only processes threads that were explicitly marked as
fixed by the agent or verified by a post-fix checker (e.g., accept and iterate a
filtered list like fixedBotComments or require a flag on each BotComment such as
comment.fixed/ comment.verified), update callers that currently pass
pr.botComments to instead pass only the confirmed-fixed subset, and ensure the
function continues to use comment.threadId when calling githubGraphQL so only
those confirmed threads are resolved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c4579350-adab-400f-bf05-fca75baa7fdd
📒 Files selected for processing (10)
.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/pr-patrol/index.tscrux/wiki-server/sync-session.ts
✅ Files skipped from review due to trivial changes (1)
- .squawk.toml
🚧 Files skipped from review as they are similar to previous changes (5)
- crux/wiki-server/sync-session.ts
- apps/wiki-server/src/schema.ts
- apps/wiki-server/drizzle/meta/_journal.json
- apps/wiki-server/src/api-types.ts
- apps/wiki-server/drizzle/0063_add_session_id_to_agent_sessions.sql
| try { | ||
| result = await db | ||
| .update(agentSessions) | ||
| .set(updates) | ||
| .where(eq(agentSessions.id, id)) | ||
| .returning(); | ||
| } catch (e: unknown) { | ||
| // Translate FK violation into a 400 response | ||
| const msg = e instanceof Error ? e.message : String(e); | ||
| if (msg.includes("foreign key") || msg.includes("violates foreign key")) { | ||
| return c.json({ error: "invalid_reference", message: "sessionId references a non-existent session" }, 400); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== apps/wiki-server/src/db.ts ==\n'
sed -n '1,220p' apps/wiki-server/src/db.ts
printf '\n== DB-related dependencies ==\n'
fd 'package\.json$' . -x sh -c '
jq -r "
((.dependencies // {}) + (.devDependencies // {}))
| to_entries[]
| select(.key | test(\"drizzle|pg|postgres|neon\"))
| \"\(.key)@\(.value)\"
" "$1"
' sh {}
printf '\n== Existing database error handling ==\n'
rg -n '23503|PostgresError|DatabaseError|foreign key' apps/wiki-server/src cruxRepository: quantified-uncertainty/longterm-wiki
Length of output: 12038
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Package.json dependencies (wiki-server) ==\n'
cat apps/wiki-server/package.json | jq '.dependencies + .devDependencies | with_entries(select(.key | test("drizzle|pg|postgres|neon")))' 2>/dev/null || echo "Failed to parse, trying alternative"
printf '\n== Direct grep for dependencies ==\n'
grep -E '"(drizzle|pg|postgres|neon)' apps/wiki-server/package.json || echo "No matches"
printf '\n== Search for error handling patterns ==\n'
rg -n 'code|SQLSTATE|23503' apps/wiki-server/src --type ts -A 2 -B 2 | head -100
printf '\n== Search for foreign key error handling ==\n'
rg -n 'foreign key|fk_violation|FK' apps/wiki-server/src --type ts -A 3 -B 1 | head -100
printf '\n== Check postgres-js error types if available ==\n'
rg -n 'postgres.*Error|PostgresError|throw.*Error' apps/wiki-server/src --type ts | head -50Repository: quantified-uncertainty/longterm-wiki
Length of output: 16787
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Test for FK violation in agent-sessions ==\n'
grep -A 20 'FK\|foreign key\|violates' apps/wiki-server/src/__tests__/agent-sessions.test.ts | head -80
printf '\n== Full statements.ts error handling pattern ==\n'
sed -n '1390,1410p' apps/wiki-server/src/routes/statements.ts
printf '\n== Search for postgres-js usage with error code ==\n'
rg -n "\.code\s*===" apps/wiki-server/src --type ts -B 2 -A 2
printf '\n== Check for SQLSTATE or error code constants ==\n'
rg -n "23503|SQLSTATE" apps/wiki-server --type ts -B 1 -A 1Repository: quantified-uncertainty/longterm-wiki
Length of output: 1767
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Search for postgres error codes used anywhere ==\n'
rg -n "code.*23" apps/wiki-server --type ts
printf '\n== Check node_modules postgres-js for error types ==\n'
find node_modules/postgres -name "*.d.ts" 2>/dev/null | head -5 | xargs head -100
printf '\n== Look for FK violation test patterns ==\n'
rg -n "invalid_reference|sessionId.*42" apps/wiki-server/src/__tests__ --type ts -B 5 -A 10 | head -150
printf '\n== Look at the entire agent-sessions PATCH test suite ==\n'
sed -n '490,520p' apps/wiki-server/src/__tests__/agent-sessions.test.tsRepository: quantified-uncertainty/longterm-wiki
Length of output: 477
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Migration defining the FK ==\n'
cat apps/wiki-server/drizzle/0063_add_session_id_to_agent_sessions.sql
printf '\n== Integration test for agent-sessions ==\n'
rg -n "sessionId" apps/wiki-server/src/__tests__/integration.test.ts -B 3 -A 8 | head -100
printf '\n== Check if there's a test that tests FK error specifically ==\n'
rg -n "invalid_reference\|400.*sessionId\|FK.*error" apps/wiki-server/src/__tests__ --type tsRepository: quantified-uncertainty/longterm-wiki
Length of output: 1299
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Check agent-sessions route for the full error handling ==\n'
sed -n '140,165p' apps/wiki-server/src/routes/agent-sessions.ts
printf '\n== Look for any test that expects 400 invalid_reference ==\n'
rg -n "invalid_reference" apps/wiki-server/src --type ts -B 3 -A 3
printf '\n== Check postgres-js README or docs reference ==\n'
find . -name "README*" -o -name "CHANGELOG*" 2>/dev/null | xargs grep -l "postgres" 2>/dev/null | head -3
printf '\n== Search for error.code usage patterns ==\n'
rg -n "error\.code|err\.code" apps/wiki-server/src --type ts -B 2 -A 2 | head -60Repository: quantified-uncertainty/longterm-wiki
Length of output: 1801
🌐 Web query:
postgres-js error handling SQLSTATE code property
💡 Result:
In postgres-js (the postgres / Postgres.js library), PostgreSQL server errors are thrown as a PostgresError where the SQLSTATE is available on:
error.code→ the 5-character PostgreSQL SQLSTATE (e.g.23505for unique violation). [1]
Example:
import postgres from 'postgres'
const sql = postgres()
try {
await sql`insert into users(email) values (${email})`
} catch (err) {
if (err.code === '23505') {
// UNIQUE_VIOLATION
}
// other useful fields (when present): err.detail, err.hint, err.constraint_name, etc.
}Notes:
- Postgres.js also uses
codefor some client/connection errors, but those may be non-SQLSTATE strings likeUNDEFINED_VALUE,CONNECT_TIMEOUT, etc. [1] - SQLSTATE codes are standardized 5-character codes; PostgreSQL documents them and recommends checking the code instead of parsing text messages. [2]
Sources:
[1] Postgres.js error handling docs (PostgresError fields incl. code as SQLSTATE) (tessl.io)
[2] PostgreSQL error codes (SQLSTATE) documentation (postgresql.org)
Citations:
- 1: https://tessl.io/registry/tessl/npm-postgres/3.4.0/files/docs/errors.md
- 2: https://www.postgresql.org/docs/8.4/errcodes-appendix.htm?utm_source=openai
🌐 Web query:
PostgreSQL SQLSTATE 23503 foreign key violation
💡 Result:
SQLSTATE 23503 in PostgreSQL means foreign_key_violation: a statement would break a FOREIGN KEY constraint. [1]
It typically happens in two cases:
- INSERT/UPDATE on the child (referencing) table: you set a foreign-key column to a value that doesn’t exist in the parent (referenced) table (often shown as
DETAIL: Key (...)=(...) is not present in table ...). [2] - DELETE/UPDATE on the parent table: you try to delete/update a parent row that’s still referenced by rows in a child table (behavior depends on
ON DELETE/ON UPDATEaction likeRESTRICT/NO ACTION/CASCADE/SET NULL/SET DEFAULT). [2]
How to fix
- Ensure the parent row exists first (insert parent, then child), or change the FK value to an existing parent key. [2]
- If you’re deleting/updating a parent row, choose the right referential action:
ON DELETE CASCADE(delete children automatically),ON DELETE SET NULL/SET DEFAULT,- or keep
RESTRICT/NO ACTIONand delete/update children explicitly first. [2]
- If the order of operations is the issue within a transaction, consider a DEFERRABLE FK and defer checking until commit (e.g.,
SET CONSTRAINTS ... DEFERRED). [2]
Quick diagnosis tip: the error message includes the constraint name (e.g., ... violates foreign key constraint "child_parent_id_fkey"). Use that to find the exact FK definition in your schema.
Sources: PostgreSQL error code 23503 = foreign_key_violation [1]; PostgreSQL docs on foreign keys and referential actions (and deferrable checking) [2].
[1] PostgreSQL Documentation: “PostgreSQL Error Codes” (SQLSTATE 23503)
[2] PostgreSQL Documentation: “Constraints” (Foreign keys, ON DELETE/ON UPDATE, deferrable checking)
Use error.code === "23503" instead of parsing error.message for FK violations.
The postgres library exposes PostgreSQL error codes on error.code (the 5-character SQLSTATE). For foreign key violations, this is "23503". The current string-matching approach is fragile and may fail if the error message shape changes; this codebase already uses the .code property pattern in statements.ts (lines 1397–1404).
Replace the message check:
if (msg.includes("foreign key") || msg.includes("violates foreign key"))with:
if ((e as { code?: string }).code === "23503")🤖 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 145 - 156, The
catch block around the db.update(...).returning() currently inspects the error
message string to detect a foreign key violation; update it to check the
PostgreSQL SQLSTATE code instead by testing the caught error object (e) for a
code property equal to "23503" (e.g., use a type guard like (e as { code?:
string }).code === "23503") and keep the same response for that case; ensure you
still derive msg for other logging/handling but stop using msg.includes("foreign
key") or similar string checks.
|
🤖 PR Patrol: Abandoning automatic fix after 2 failed attempts. Issues detected: ci-failure, missing-issue-ref, bot-review-major This PR likely needs human intervention to resolve. |
|
This PR shares 1 file(s) with PR #1835:
Coordinate to avoid merge conflicts. Posted by PR Patrol — informational only. |
…ause check, remove blanket bot-thread resolution
Three fixes based on CodeRabbit review:
1. **Test mock FK enforcement** (agent-sessions.test.ts):
- Add `VALID_SESSION_IDS` set (seeds ID 42 as a valid FK target)
- `session_id` update branch now throws FK-like error for unknown IDs
- New test: `returns 400 invalid_reference for non-existent sessionId`
2. **Route FK error detection** (routes/agent-sessions.ts):
- Drizzle wraps DB errors in `DrizzleQueryError` whose message is
`"Failed query: ..."` — the original FK message lives in `e.cause`
- The previous check `msg.includes("foreign key")` never matched in
production; now also checks `e.cause.message`
3. **pr-patrol thread resolution** (crux/pr-patrol/index.ts):
- Remove `resolveBotReviewThreads()` call after successful fix run
- A zero exit code does not prove every open thread was addressed;
blanket resolution dismisses valid feedback that wasn't fixed
- Threads are now resolved by CodeRabbit re-review or manually
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
This PR shares 1 file(s) with PR #1835:
Coordinate to avoid merge conflicts. Posted by PR Patrol — informational only. |
|
This PR shares 1 file(s) with PR #1837:
Coordinate to avoid merge conflicts. Posted by PR Patrol — informational only. |
|
This PR shares 1 file(s) with PR #1838:
Coordinate to avoid merge conflicts. Posted by PR Patrol — informational only. |
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
Bug Fixes
Tests
Documentation