fix(persistence): move fork-only schema onto its own migration ledger - #13
Conversation
yordis
commented
Jul 31, 2026
yordis
commented
- The shared migrator ledger only remembers the highest migration id that ran, so carrying fork migrations in the shared chain forced renumbering every incoming upstream migration at each sync, a tax that grows forever now that the subagent-threads divergence will not be upstreamed.
- The documented fallback of reserving ids >= 1000 for fork migrations was a latent bug: the first one to run would make every later upstream migration arrive below the ledger's high-water mark and be silently skipped, drifting the schema.
- A second migration chain with its own ledger table makes fork and upstream ids structurally unable to collide, and lets the shared chain and every upstream migration file stay byte-identical to upstream so future syncs stop conflicting there.
- Existing installs already recorded the fork's ProjectionThreadParent as shared id 33, so a one-time realignment returns their ledger to upstream numbering before the shared migrator runs; fresh and already-realigned installs no-op.
The shared migrator ledger only remembers the highest id that ran, so carrying fork migrations in the shared chain either displaces upstream ids at every sync or, under the reserved high-id convention, would make installs silently skip future upstream migrations. A separate ledger removes both failure modes and keeps the shared chain byte-identical to upstream. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
PR SummaryHigh Risk Overview SQLite startup now runs realign → shared migrations → fork migrations. The old Reviewed by Cursor Bugbot for commit d338f88. Bugbot is set up for automated code reviews on this repo. Configure here. |
github-actions
Bot
added
vouch:trusted
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe persistence layer separates fork migrations from upstream migrations. It restores shared migration IDs 33–35, adds transactional repair for legacy ledgers, runs the fork chain through a separate ledger, and adds fresh-install and legacy-install coverage. ChangesMigration chain separation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
…lout Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/server/src/persistence/ForkMigrations.ts (1)
105-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
ForkMigrationsLiveexport.
ForkMigrationsLiveonly appears inapps/server/src/persistence/ForkMigrations.ts; the SQLite setup imports and callsrunForkMigrations()directly. Remove the dead exported layer if no future entry point needs it.🤖 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 `@apps/server/src/persistence/ForkMigrations.ts` around lines 105 - 108, Remove the unused ForkMigrationsLive export and its Layer.effectDiscard wrapper from ForkMigrations.ts. Keep runForkMigrations unchanged, since SQLite setup invokes it directly.
🤖 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 @.agents/skills/trogonstack-sync-upstream/SKILL.md:
- Around line 42-45: Correct the fail-fast test command in the migration-sync
guidance to follow the CLI convention: use the built-in Vite+ test form with the
file path, removing the extra “run” token. Keep the referenced test file and
surrounding migration instructions unchanged.
---
Nitpick comments:
In `@apps/server/src/persistence/ForkMigrations.ts`:
- Around line 105-108: Remove the unused ForkMigrationsLive export and its
Layer.effectDiscard wrapper from ForkMigrations.ts. Keep runForkMigrations
unchanged, since SQLite setup invokes it directly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6da6d6fe-5fb1-425f-91ae-b6c35f4d28e1
📒 Files selected for processing (11)
.agents/skills/trogonstack-sync-upstream/SKILL.mdapps/server/src/persistence/ForkMigrations.test.tsapps/server/src/persistence/ForkMigrations.tsapps/server/src/persistence/Layers/Sqlite.tsapps/server/src/persistence/Migrations.test.tsapps/server/src/persistence/Migrations.tsapps/server/src/persistence/Migrations/033_ProjectionThreadsSettled.tsapps/server/src/persistence/Migrations/034_ProjectionThreadsSnoozed.tsapps/server/src/persistence/Migrations/035_ProjectionThreadTitleRegeneration.test.tsapps/server/src/persistence/Migrations/035_ProjectionThreadTitleRegeneration.tsapps/server/src/persistence/Migrations/fork/001_ProjectionThreadParent.ts
💤 Files with no reviewable changes (1)
- apps/server/src/persistence/Migrations.test.ts
| - Upstream migrations sync as-is, keeping their own ids. `Migrations.ts` and every file under `apps/server/src/persistence/Migrations/` (excluding `fork/`) must stay byte-identical to upstream. If either conflicts during a sync, take upstream's side wholesale rather than hand-merging. | ||
| - Fork-only schema never lands in `Migrations.ts`. It lives only in `apps/server/src/persistence/ForkMigrations.ts`, numbered independently under `apps/server/src/persistence/Migrations/fork/`, and tracked in its own `trogonstack_fork_migrations` ledger table instead of the shared `effect_sql_migrations` table. | ||
| - Because the two chains never share a ledger, id collisions between fork and upstream migrations are structurally impossible - there is nothing to renumber or offset during a sync. | ||
| - After resolving `Migrations.ts`, run the fail-fast guard: `vp test run apps/server/src/persistence/ForkMigrations.test.ts`. It rejects any fork migration name that leaks into the shared `migrationEntries` chain. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Verify the vp test run command syntax against the CLI convention.
Line 45 uses vp test run apps/server/src/persistence/ForkMigrations.test.ts. The coding guideline states two valid forms: vp test for the built-in Vite+ test command, and vp run test only for the test package script. vp test run <path> matches neither form and looks like a merge of the two.
Confirm the intended command and correct it. If the intent was to run vp test against a specific file, drop run.
📝 Proposed fix
-- After resolving `Migrations.ts`, run the fail-fast guard: `vp test run apps/server/src/persistence/ForkMigrations.test.ts`. It rejects any fork migration name that leaks into the shared `migrationEntries` chain.
+- After resolving `Migrations.ts`, run the fail-fast guard: `vp test apps/server/src/persistence/ForkMigrations.test.ts`. It rejects any fork migration name that leaks into the shared `migrationEntries` chain.As per coding guidelines, "Use vp test for the built-in Vite+ test command; use vp run test only when specifically requiring the test package script."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - Upstream migrations sync as-is, keeping their own ids. `Migrations.ts` and every file under `apps/server/src/persistence/Migrations/` (excluding `fork/`) must stay byte-identical to upstream. If either conflicts during a sync, take upstream's side wholesale rather than hand-merging. | |
| - Fork-only schema never lands in `Migrations.ts`. It lives only in `apps/server/src/persistence/ForkMigrations.ts`, numbered independently under `apps/server/src/persistence/Migrations/fork/`, and tracked in its own `trogonstack_fork_migrations` ledger table instead of the shared `effect_sql_migrations` table. | |
| - Because the two chains never share a ledger, id collisions between fork and upstream migrations are structurally impossible - there is nothing to renumber or offset during a sync. | |
| - After resolving `Migrations.ts`, run the fail-fast guard: `vp test run apps/server/src/persistence/ForkMigrations.test.ts`. It rejects any fork migration name that leaks into the shared `migrationEntries` chain. | |
| - Upstream migrations sync as-is, keeping their own ids. `Migrations.ts` and every file under `apps/server/src/persistence/Migrations/` (excluding `fork/`) must stay byte-identical to upstream. If either conflicts during a sync, take upstream's side wholesale rather than hand-merging. | |
| - Fork-only schema never lands in `Migrations.ts`. It lives only in `apps/server/src/persistence/ForkMigrations.ts`, numbered independently under `apps/server/src/persistence/Migrations/fork/`, and tracked in its own `trogonstack_fork_migrations` ledger table instead of the shared `effect_sql_migrations` table. | |
| - Because the two chains never share a ledger, id collisions between fork and upstream migrations are structurally impossible - there is nothing to renumber or offset during a sync. | |
| - After resolving `Migrations.ts`, run the fail-fast guard: `vp test apps/server/src/persistence/ForkMigrations.test.ts`. It rejects any fork migration name that leaks into the shared `migrationEntries` chain. |
🤖 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 @.agents/skills/trogonstack-sync-upstream/SKILL.md around lines 42 - 45,
Correct the fail-fast test command in the migration-sync guidance to follow the
CLI convention: use the built-in Vite+ test form with the file path, removing
the extra “run” token. Keep the referenced test file and surrounding migration
instructions unchanged.
Source: Coding guidelines
1 participant
