fix(insights): add missing lifecycle column compatibility check#43
Merged
gsxdsm merged 5 commits intoRunfusion:mainfrom May 5, 2026
Merged
Conversation
…ifecycle column Databases already at schema v59+ when the lifecycle column was added never re-ran the v59 migration, causing "no column named lifecycle" errors on insight generation. Adds an unconditional compatibility check following the existing ensureRoutinesSchemaCompatibility pattern. Fixes Runfusion#42 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds an idempotent compatibility backfill, ensureInsightRunsSchemaCompatibility(), invoked during Database.init() to add missing lifecycle and cancelledAt columns and ensure idxInsightRunsProjectTriggerStatus exists on project_insight_runs for databases that missed earlier migrations. A test verifies the backfill restores columns and that InsightStore.createRun() works. ChangesSchema Compatibility Backfill
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Greptile SummaryThis PR fixes a "table project_insight_runs has no column named lifecycle" crash by adding
Confidence Score: 5/5Safe to merge — the change is a narrow, idempotent backfill that cannot regress databases which already have the columns, and the new test proves the fix works end-to-end. Both No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant App
participant Database
participant SQLite
App->>Database: init()
Database->>SQLite: run migrations (migrate())
Note over Database,SQLite: version < SCHEMA_VERSION → apply missing migrations
Database->>Database: ensureRoutinesSchemaCompatibility()
Database->>Database: ensureInsightRunsSchemaCompatibility()
activate Database
Database->>SQLite: hasTable("project_insight_runs")?
SQLite-->>Database: true / false
alt table exists
Database->>SQLite: addColumnIfMissing("lifecycle", "TEXT")
Database->>SQLite: addColumnIfMissing("cancelledAt", "TEXT")
Database->>SQLite: CREATE INDEX IF NOT EXISTS idxInsightRunsProjectTriggerStatus
end
deactivate Database
Database-->>App: ready
Reviews (5): Last reviewed commit: "Update packages/core/src/__tests__/insig..." | Re-trigger Greptile |
…ibility Simulates a database where project_insight_runs exists without lifecycle and cancelledAt columns (the state that caused the original bug), then verifies that re-opening the database adds the columns and creating a run succeeds. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Pre-existing lint error on main — the `any` cast in __setCreateFnAgent mirrors the eslint-disable already on the adjacent variable declarations. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/__tests__/insight-store.test.ts (1)
970-1015: ⚡ Quick winAlways close DB handles via
finallyto avoid flaky cleanup on failed assertions.If an assertion fails before the explicit
close()calls, open SQLite handles can leak and make directory cleanup flaky. Wrapping each opened DB intry/finallykeeps this deterministic.Suggested refactor
- const db1 = createDatabase(compatDir); - db1.init(); - expect(db1.getSchemaVersion()).toBe(61); + const db1 = createDatabase(compatDir); + try { + db1.init(); + expect(db1.getSchemaVersion()).toBe(61); - // Step 2: Strip lifecycle and cancelledAt columns by recreating the - // table without them. This simulates a DB that was created before the - // lifecycle columns were added and already past v59 when they landed. - db1.exec(` - CREATE TABLE project_insight_runs_legacy AS - SELECT id, projectId, trigger, status, summary, error, - insightsCreated, insightsUpdated, inputMetadata, outputMetadata, - createdAt, startedAt, completedAt - FROM project_insight_runs - `); - db1.exec("DROP TABLE project_insight_runs"); - db1.exec("ALTER TABLE project_insight_runs_legacy RENAME TO project_insight_runs"); + // Step 2: Strip lifecycle and cancelledAt columns by recreating the + // table without them. This simulates a DB that was created before the + // lifecycle columns were added and already past v59 when they landed. + db1.exec(` + CREATE TABLE project_insight_runs_legacy AS + SELECT id, projectId, trigger, status, summary, error, + insightsCreated, insightsUpdated, inputMetadata, outputMetadata, + createdAt, startedAt, completedAt + FROM project_insight_runs + `); + db1.exec("DROP TABLE project_insight_runs"); + db1.exec("ALTER TABLE project_insight_runs_legacy RENAME TO project_insight_runs"); - // Verify lifecycle column is gone - const colsBefore = db1.prepare("PRAGMA table_info(project_insight_runs)").all() as Array<{ name: string }>; - const colNamesBefore = colsBefore.map((c) => c.name); - expect(colNamesBefore).not.toContain("lifecycle"); - expect(colNamesBefore).not.toContain("cancelledAt"); - db1.close(); + // Verify lifecycle column is gone + const colsBefore = db1.prepare("PRAGMA table_info(project_insight_runs)").all() as Array<{ name: string }>; + const colNamesBefore = colsBefore.map((c) => c.name); + expect(colNamesBefore).not.toContain("lifecycle"); + expect(colNamesBefore).not.toContain("cancelledAt"); + } finally { + db1.close(); + } // Step 3: Re-open — ensureInsightRunsSchemaCompatibility should add the // missing columns unconditionally. const db2 = createDatabase(compatDir); - db2.init(); + try { + db2.init(); - const colsAfter = db2.prepare("PRAGMA table_info(project_insight_runs)").all() as Array<{ name: string }>; - const colNamesAfter = colsAfter.map((c) => c.name); - expect(colNamesAfter).toContain("lifecycle"); - expect(colNamesAfter).toContain("cancelledAt"); + const colsAfter = db2.prepare("PRAGMA table_info(project_insight_runs)").all() as Array<{ name: string }>; + const colNamesAfter = colsAfter.map((c) => c.name); + expect(colNamesAfter).toContain("lifecycle"); + expect(colNamesAfter).toContain("cancelledAt"); - // Step 4: Creating a run must not throw — proves the INSERT path works - // with the restored columns. - const s = new InsightStore(db2); - const run = s.createRun("proj", { trigger: "manual" }); - expect(run.id).toBeTruthy(); - expect(run.lifecycle).toBeDefined(); - - db2.close(); + // Step 4: Creating a run must not throw — proves the INSERT path works + // with the restored columns. + const s = new InsightStore(db2); + const run = s.createRun("proj", { trigger: "manual" }); + expect(run.id).toBeTruthy(); + expect(run.lifecycle).toBeDefined(); + } finally { + db2.close(); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/__tests__/insight-store.test.ts` around lines 970 - 1015, The test opens two DB handles (created via createDatabase and referenced as db1 and db2) but only calls close() directly, which can leak handles if assertions throw; wrap each DB lifecycle in its own try/finally (or at least ensure db1 and db2 are closed in a finally) so that after db1.init() and after db2.init() you always call db1.close() and db2.close() respectively even on failure—adjust the test around the blocks that use db1 and db2 (and the InsightStore usage) to guarantee deterministic cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/core/src/__tests__/insight-store.test.ts`:
- Around line 970-1015: The test opens two DB handles (created via
createDatabase and referenced as db1 and db2) but only calls close() directly,
which can leak handles if assertions throw; wrap each DB lifecycle in its own
try/finally (or at least ensure db1 and db2 are closed in a finally) so that
after db1.init() and after db2.init() you always call db1.close() and
db2.close() respectively even on failure—adjust the test around the blocks that
use db1 and db2 (and the InsightStore usage) to guarantee deterministic cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0e626c19-00bd-4276-bd0b-28c4fa100637
📒 Files selected for processing (1)
packages/core/src/__tests__/insight-store.test.ts
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+1070
to
+1078
| private ensureInsightRunsSchemaCompatibility(): void { | ||
| if (!this.hasTable("project_insight_runs")) { | ||
| return; | ||
| } | ||
|
|
||
| this.addColumnIfMissing("project_insight_runs", "lifecycle", "TEXT"); | ||
| this.addColumnIfMissing("project_insight_runs", "cancelledAt", "TEXT"); | ||
| this.db.exec(`CREATE INDEX IF NOT EXISTS idxInsightRunsProjectTriggerStatus ON project_insight_runs(projectId, trigger, status)`); | ||
| } |
Contributor
There was a problem hiding this comment.
AGENTS.md requires a changeset file for any change that affects @runfusion/fusion — including bug fixes. Because @fusion/core is bundled into the published CLI via noExternal, this fix to db.ts ships to end-users and needs a patch changeset. None of the three commits in this PR include one. A missing changeset means the next pnpm release will silently skip version bumping and changelog entries for this fix.
Context Used: AGENTS.md (source)
Collaborator
|
@copilot resolve the merge conflicts in this pull request |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/__tests__/insight-store.test.ts`:
- Line 974: The test assertion uses the wrong expected schema version; update
the expectation on db1.getSchemaVersion() from 61 to 62 (or better, import and
use the SCHEMA_VERSION constant from db.ts) so it matches the actual
SCHEMA_VERSION used by the database; locate the assertion on
db1.getSchemaVersion() in the test and change the expected value to 62 (or
replace the literal with the SCHEMA_VERSION symbol) to make it consistent with
other tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 283142d9-4076-476c-91d3-60a2abad55d6
📒 Files selected for processing (2)
packages/core/src/__tests__/insight-store.test.tspackages/core/src/db.ts
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/__tests__/insight-store.test.ts (1)
1001-1004: ⚡ Quick winCover the index backfill in this compatibility test.
ensureInsightRunsSchemaCompatibility()also recreatesidxInsightRunsProjectTriggerStatus, but this test only asserts the restored columns and insert path. A regression in the index creation would still pass here.♻️ Suggested assertion
const colsAfter = db2.prepare("PRAGMA table_info(project_insight_runs)").all() as Array<{ name: string }>; const colNamesAfter = colsAfter.map((c) => c.name); expect(colNamesAfter).toContain("lifecycle"); expect(colNamesAfter).toContain("cancelledAt"); + + const indexesAfter = db2.prepare( + "SELECT name FROM sqlite_master WHERE type = 'index' AND tbl_name = 'project_insight_runs'" + ).all() as Array<{ name: string }>; + expect(indexesAfter.map((i) => i.name)).toContain("idxInsightRunsProjectTriggerStatus");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/__tests__/insight-store.test.ts` around lines 1001 - 1004, The test currently checks restored columns but not the recreated index; update the test that calls ensureInsightRunsSchemaCompatibility() to also verify the index idxInsightRunsProjectTriggerStatus exists and covers the expected columns: after the migration query PRAGMA index_list(project_insight_runs) and assert the returned index names include "idxInsightRunsProjectTriggerStatus", then query PRAGMA index_info('idxInsightRunsProjectTriggerStatus') and assert its column sequence contains "project", "trigger", and "status" to fully cover the index backfill path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/core/src/__tests__/insight-store.test.ts`:
- Around line 1001-1004: The test currently checks restored columns but not the
recreated index; update the test that calls
ensureInsightRunsSchemaCompatibility() to also verify the index
idxInsightRunsProjectTriggerStatus exists and covers the expected columns: after
the migration query PRAGMA index_list(project_insight_runs) and assert the
returned index names include "idxInsightRunsProjectTriggerStatus", then query
PRAGMA index_info('idxInsightRunsProjectTriggerStatus') and assert its column
sequence contains "project", "trigger", and "status" to fully cover the index
backfill path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 12c53b62-1c07-4acf-a6f0-0fcc183161c7
📒 Files selected for processing (1)
packages/core/src/__tests__/insight-store.test.ts
gsxdsm
approved these changes
May 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ensureInsightRunsSchemaCompatibility()to fix "table project_insight_runs has no column named lifecycle" error when clicking Generate InsightsensureRoutinesSchemaCompatibility()pattern — idempotentaddColumnIfMissingcalls that run unconditionally on everyinit()Root Cause
The
lifecycleandcancelledAtcolumns were retroactively added to migration v33 and as a safety-net in theversion < 59block. Databases already at v59+ never re-ran that migration, leaving the columns missing.Test plan
Fixes #42
Summary by CodeRabbit
Bug Fixes
Tests