fix(boot): run pending data migrations on every server start#268
Merged
Conversation
The migration runner was only invoked from update.sh — plain pull-and-restart (or pm2 restart after a manual git pull) left shipped migrations unapplied, so new prompt stages, settings renames, and seeded data sat in the queue until the user ran 'npm run update' or 'npm run migrations'. Existing installs hit cryptic 'Stage X not found' errors despite the fix shipping in main (see PR #262 / pipeline-volume-cover-concepts). Refactored scripts/run-migrations.js to export a runMigrations() function and kept the CLI shim. server/index.js now awaits the runner before the AI toolkit reads stage-config.json. Idempotent and cheap when the applied list is current; logs and continues on failure rather than refusing to boot.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR ensures shipped data migrations are applied automatically during server boot, preventing upgraded installs from missing newly-added prompt stages/config (e.g., “Stage X not found”) when users restart via npm start, npm run dev, or pm2 restart without running update.sh.
Changes:
- Refactor
scripts/run-migrations.jsto exportrunMigrations({ rootDir } = {})while preserving CLI behavior via an “invoked directly” guard. - Invoke
runMigrations()early inserver/index.jsstartup, before the AI toolkit loadsstage-config.json/providers.json. - Document the behavior change in
.changelog/NEXT.md.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
server/index.js |
Runs migrations during boot before AI toolkit initialization to prevent missing-stage errors on restart. |
scripts/run-migrations.js |
Exposes a programmatic migration runner and adds a CLI-only execution guard. |
.changelog/NEXT.md |
Changelog entry describing auto-application of pending migrations on server start. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
`pathToFileURL()` throws on relative paths, which can happen when the script is launched as `node scripts/run-migrations.js`. Resolve argv[1] to an absolute path first, then realpath both sides to follow npm bin shims / symlinks reliably. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- scripts/run-migrations.js: drop the async realpath-based isInvokedAsScript helper that turned the module into an async module and forced filesystem I/O at evaluation time. Switch to a synchronous pathToFileURL(resolve(argv[1])) compare against import.meta.url, which still handles the relative-argv case the original review flagged. - scripts/run-migrations.js + server/index.js: log err?.stack ?? err on migration failures so boot diagnostics include the full trace and tolerate non-Error throws. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
scripts/run-migrations.js:16
runMigrations()is now a shared module API and is invoked during server boot, but there are no automated tests covering the refactored runner behavior (e.g., applied-list handling, migration import selection, and the “does not auto-run when imported” contract). Consider adding a vitest unit test (can live underserver/and mockfs/promises+import()via a tiny fixture migration) to prevent regressions that could silently skip migrations or break startup.
export async function runMigrations({ rootDir = DEFAULT_ROOT_DIR } = {}) {
const appliedFile = join(rootDir, 'data', 'migrations.applied.json');
// Ensure data/ exists so we can persist applied state (migrationsDir
// ships in the repo and always exists).
await mkdir(dirname(appliedFile), { recursive: true });
A crash mid-write to data/migrations.applied.json can leave the file truncated or malformed, which previously caused every subsequent boot to hard-fail. Detect bad JSON or non-array shapes, rename the corrupt file aside (.corrupt-<ISO timestamp>) for forensics, and rebuild from scratch — migrations are idempotent so re-running them is safe. Also parameterize migrationsDir so runMigrations is testable against a fixture migrations directory, and broaden the vitest include glob to pick up scripts/*.test.js alongside scripts/migrations/*.test.js.
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
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
Stage pipeline-volume-cover-concepts not found(and equivalent migration-shaped errors) on plain pull-and-restart becausescripts/run-migrations.jswas only invoked fromupdate.sh—npm start,npm run dev, andpm2 restartall skipped it.runMigrations({ rootDir } = {})while keeping its CLI behavior, then awaited it at the top ofserver/index.jsbeforecreateAIToolkit()readsstage-config.json.console.errorwith the full stack and let boot continue, matching the existing provider-warmup pattern at line 243.Why
PR #262 shipped migration 017 for the missing stage entry but couldn't help users who didn't run
update.sh— they restarted viapm2 restartornpm startand still saw the route error. Wiring the runner into server boot makes shipped migrations apply automatically on the next launch, regardless of how the process was restarted.Implementation notes
import.meta.url === pathToFileURL(resolve(process.argv[1])).href.resolve()normalizes a possibly-relativeargv[1](the case when launched asnode scripts/run-migrations.js) into the absolute pathpathToFileURLrequires; URL equality normalizes slash/case differences on Windows. Symlink chains aren't normalized — but the script is launched vianode, not a bin shim, so there's no symlink in play in any of the documented entry points (npm run migrations, server boot, manualnode ./scripts/...).server/index.jsdoesn't make it an async module or trigger filesystem I/O at evaluation time.MIGRATIONS_DIRis now a module-level const (was function-scoped) since both the exported function and the CLI shim share it.runMigrations()returns the number of applied migrations for callers that want it; current consumers ignore the return value.Test plan
data/migrations.applied.jsonentries 015–019, runnpm start, verify the server applies them on boot andpipeline-volume-cover-conceptsresolves on the next request.pm2 restart ecosystem.config.cjs→ server logs✅ No pending migrationsand serves traffic.npm run migrationsruns through migrations and exits 0 (CLI guard fires when invoked directly).await import('./scripts/run-migrations.js')yields exports only, no migration logs).