feat(migrate): genie host-migrations framework + 2 initial migrations - #1619
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a versioned host-migration framework for the Genie CLI to manage host-state drift, including an orchestrator, a JSON-based tracking store, and a post-install hook for automatic updates. While the framework is well-structured, several critical issues must be addressed for production readiness. The migration discovery logic will fail in the distributed package because the steps directory is not included in the build, and the pathing for package.json is inconsistent. Furthermore, Migration 002 is incompatible with macOS and will crash when run under Node.js due to the use of Bun.sleepSync. Other concerns include the use of process.exit() which bypasses CLI cleanup routines and the lack of file locking in the migration store to prevent race conditions.
| export function discoverMigrations(): DiscoveredMigration[] { | ||
| const stepsDir = join(__dirname, 'steps'); | ||
| if (!existsSync(stepsDir)) return []; | ||
| const files = readdirSync(stepsDir); | ||
| const matched: DiscoveredMigration[] = []; | ||
| for (const file of files) { | ||
| const m = file.match(FILE_PATTERN); | ||
| if (!m) continue; | ||
| matched.push({ id: m[1], filePath: join(stepsDir, file) }); | ||
| } | ||
| matched.sort((a, b) => a.id.localeCompare(b.id)); | ||
| return matched; | ||
| } |
There was a problem hiding this comment.
The migration discovery logic will fail in production. The src/migrations/steps/ directory is not included in the package.json files array, nor is it copied to the dist/ directory during the build process. Since discoverMigrations relies on readdirSync at runtime to find .ts or .js files, it will find no migrations in the published npm package. You should add the steps directory to the package files and ensure they are accessible relative to the bundled executable, or use a build-time macro to register them.
| } | ||
|
|
||
| const result = await migrate({ quiet: options.quiet, dryRun: options.dryRun }); | ||
| process.exit(result.ok ? 0 : 1); |
There was a problem hiding this comment.
Calling process.exit() here is problematic. The main CLI entry point in src/genie.ts wraps command execution in try...finally blocks to ensure that database connections are closed and observability receivers are stopped. process.exit() terminates the process immediately, bypassing these essential cleanup steps. Consider throwing an error or returning a status to let the top-level handler manage the exit code while allowing cleanup to proceed.
| process.exit(result.ok ? 0 : 1); | |
| if (!result.ok) throw new Error('genie host-migrations failed'); |
| const out = execSync('ss -tlnp', { stdio: ['ignore', 'pipe', 'pipe'] }).toString(); | ||
| const out_lines = out.split('\n'); | ||
| const found: ListeningPg[] = []; | ||
| const seen = new Set<number>(); | ||
| for (const line of out_lines) { | ||
| // Match "127.0.0.1:<port>" with users:(("postgres",pid=<n>,...)) | ||
| const portMatch = line.match(/127\.0\.0\.1:(\d+)\s/); | ||
| const procMatch = line.match(/users:\(\("postgres",pid=(\d+)/); | ||
| if (portMatch && procMatch) { | ||
| const pid = parseInt(procMatch[1], 10); | ||
| const port = parseInt(portMatch[1], 10); | ||
| if (!seen.has(pid)) { | ||
| seen.add(pid); | ||
| found.push({ pid, port }); | ||
| } | ||
| } | ||
| } | ||
| return found; | ||
| } catch { | ||
| return []; | ||
| } | ||
| } | ||
|
|
||
| function canonicalReachable(): boolean { | ||
| try { | ||
| execSync(`pg_isready -h 127.0.0.1 -p ${CANONICAL_PORT}`, { stdio: 'pipe' }); | ||
| return true; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| function findLegacyEmbedded(): ListeningPg | undefined { | ||
| if (process.env.GENIE_KEEP_LEGACY_PG === '1') return undefined; | ||
| if (!canonicalReachable()) return undefined; | ||
| return listListeningPgserve().find((p) => p.port !== CANONICAL_PORT); | ||
| } | ||
|
|
||
| export async function check(_ctx: MigrationContext): Promise<boolean> { | ||
| return findLegacyEmbedded() !== undefined; | ||
| } | ||
|
|
||
| export async function apply(ctx: MigrationContext): Promise<void> { | ||
| const target = findLegacyEmbedded(); | ||
| if (!target) { | ||
| ctx.log('no legacy embedded found at apply time (race resolved)'); | ||
| return; | ||
| } | ||
| ctx.log(`stopping legacy embedded pgserve PID ${target.pid} (port ${target.port})`); | ||
| // Try pg_ctl stop via discovered data dir from the process | ||
| try { | ||
| // Process cmdline to find -D <dataDir> | ||
| const cmdline = execSync(`cat /proc/${target.pid}/cmdline | tr '\\0' ' '`, { stdio: ['ignore', 'pipe', 'pipe'] }).toString(); |
There was a problem hiding this comment.
This migration is not compatible with macOS. It relies on Linux-specific tools and filesystems: ss -tlnp (line 29) and /proc/${pid}/cmdline (line 81). Since Genie is intended to support both Linux and macOS, this migration will fail to detect or stop legacy processes on macOS. Consider using more portable alternatives like lsof or ps -p <pid> -o command=. Additionally, the tr command on line 81 uses \0 which may be interpreted as a literal backslash and zero rather than the null character depending on the shell environment.
| function getGenieVersion(): string { | ||
| try { | ||
| // genie cli installed structure: <root>/dist/genie.js + <root>/package.json | ||
| const pkgPath = join(__dirname, '..', '..', 'package.json'); | ||
| const pkg = JSON.parse(readFileSync(pkgPath, 'utf8')); | ||
| return pkg.version || 'unknown'; | ||
| } catch { | ||
| return 'unknown'; | ||
| } | ||
| } |
There was a problem hiding this comment.
The path logic for finding package.json is inconsistent between development and production. While ../../package.json works from src/migrations/ in the source tree, it will point to the wrong location when the code is bundled into dist/genie.js. In the distributed package, package.json is typically located at ../package.json relative to the dist/ directory.
| export function recordApplied(id: string, version: string, detail?: string): void { | ||
| const store = loadStore(); | ||
| // Strip any prior FAILED record for this id; record APPLIED authoritatively. | ||
| store.applied = store.applied.filter((r) => r.id !== id); | ||
| store.applied.push({ | ||
| id, | ||
| status: 'APPLIED', | ||
| appliedAt: new Date().toISOString(), | ||
| appliedFrom: version, | ||
| detail, | ||
| }); | ||
| saveStore(store); | ||
| } | ||
|
|
||
| export function recordFailed(id: string, version: string, reason: string): void { | ||
| const store = loadStore(); | ||
| store.applied = store.applied.filter((r) => r.id !== id); | ||
| store.applied.push({ | ||
| id, | ||
| status: 'FAILED', | ||
| appliedAt: new Date().toISOString(), | ||
| appliedFrom: version, | ||
| detail: reason, | ||
| }); | ||
| saveStore(store); | ||
| } |
There was a problem hiding this comment.
The migration store is susceptible to race conditions. recordApplied and recordFailed perform a read-modify-write operation on migrations.json without any file locking. If multiple instances of genie migrate run concurrently (e.g., a postinstall hook triggered during a manual update), one process could overwrite the updates made by another. Consider using a simple advisory lock file to synchronize access to the store.
| while (Date.now() < deadline) { | ||
| if (findLegacyEmbedded() === undefined) return; | ||
| // small sleep | ||
| Bun.sleepSync(200); |
There was a problem hiding this comment.
Using Bun.sleepSync will cause a crash if the migration is run using Node.js. While the CLI entry point uses a Bun shebang, the scripts/postinstall-migrations.js hook uses #!/usr/bin/env node and spawns the migration command using process.execPath. If a user installs the package using npm, process.execPath will be Node.js, leading to a ReferenceError when this line is reached. Use a more portable sleep implementation or check for the Bun global before calling it.
8d2854d to
23af388
Compare
Adds versioned, applied-once host-state migrations that detect and fix drift between current code expectations and persisted host state (pm2 env blocks, embedded pgserve fantasmas, config drifts). Same pattern as DB migrations but for HOST state. Auto-runs on `bun add -g @automagik/genie@latest` via postinstall hook so users get fixes transparently. Closes the upgrade-silent-breakage class where a code fix lands but pm2 process configs persist with old behavior — live example: commit 5567e20 (`fix(install): bake DATABASE_URL`) requires manual `genie install` re-run; users had no way to know. Implementation: - src/migrations/index.ts — orchestrator (discover, filter pending, apply) - src/migrations/runner.ts — per-step check → apply → validate contract - src/migrations/store.ts — atomic ~/.genie/migrations.json read/write - src/migrations/discover.ts — scan steps/, sort alphabetical = apply order - src/migrations/steps/001-pm2-env-databaseurl-bake.ts — re-bake DATABASE_URL into pm2 genie-serve env when canonical pgserve registered - src/migrations/steps/002-kill-embedded-pgserve-legacy.ts — stop legacy embedded pgserve on non-canonical ports when canonical 8432 is healthy - src/genie-commands/migrate.ts — CLI verb (--dry-run / --status / --quiet) - src/genie.ts — wire `genie migrate` subcommand - scripts/postinstall-migrations.js — soft-fail postinstall hook - package.json — chain postinstall (preserves existing tmux + hook-binary) - src/migrations/README.md — operator + contributor guide - src/migrations/__tests__/orchestrator.test.ts — 6 framework tests - test/migrations/postinstall.test.ts — 2 postinstall behavior tests - CHANGELOG.md — v4.x.x contract sentence Constraints honored: - File-based tracking (not PG): migrations may need to RUN before genie-serve / canonical pgserve are healthy - Atomic store write (tmp + rename) prevents partial JSON on crash - Soft-fail postinstall: bun install never breaks - GENIE_SKIP_MIGRATIONS=1 escape hatch - GENIE_KEEP_LEGACY_PG=1 escape hatch for migration 002 - Failed migrations recorded + retried (loud, never silently skipped) - All steps idempotent (safe to re-run) - Worktree isolated (working clone untouched) Validation: - bun test src/migrations/__tests__/ test/migrations/ → 8/8 pass Wish: .genie/wishes/genie-host-migrations/WISH.md (lint clean) Sibling: pgserve/autopg-upgrade-command (same self-heal philosophy, pgserve subsystem) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
23af388 to
b7ebecc
Compare
PR #1619 added the host-migrations framework but `package.json` `files` array did not include `scripts/postinstall-migrations.js` nor `src/migrations/`. Result on published 4.260503.5: $ tar -tzf @automagik-genie-4.260503.5.tgz | grep migration package/src/db/migrations/... # only DB migrations shipped # scripts/postinstall-migrations.js MISSING # src/migrations/{runner,discover,store,steps/*}.ts MISSING Symptom on `bun add -g @automagik/genie@next && genie update --next`: - postinstall chain has `node scripts/postinstall-migrations.js` but the file is absent (silent failure under bun's default trust policy). - `genie migrate --status` returns "No migrations discovered." because `discoverMigrations()` scans a non-existent directory. Wish acceptance bullet from `.genie/wishes/genie-host-migrations`: "Auto-runs on `bun add -g @automagik/genie@latest` via postinstall hook so users get fixes transparently." This made the bullet impossible. Two-line fix to make it actually work in the next release. Verified via `npm pack --dry-run`: npm notice 2.2kB scripts/postinstall-migrations.js npm notice 1.8kB src/migrations/discover.ts npm notice 3.8kB src/migrations/index.ts npm notice 2.4kB src/migrations/runner.ts npm notice 3.4kB src/migrations/steps/001-pm2-env-databaseurl-bake.ts npm notice 3.8kB src/migrations/steps/002-kill-embedded-pgserve-legacy.ts npm notice 2.6kB src/migrations/store.ts npm notice 2.8kB src/migrations/README.md Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 participant
Summary
Adds versioned, applied-once host-state migrations that detect and fix drift between current code expectations and persisted host state (pm2 env blocks, embedded pgserve fantasmas, config drifts). Auto-runs on
bun add -g @automagik/genie@latestvia postinstall hook so users get fixes transparently.Wish:
.genie/wishes/genie-host-migrations/WISH.md(lint clean, 3 EGs)Why
Closes the upgrade-silent-breakage class: a code fix lands but pm2 process configs persist with old behavior. Live example: commit
5567e202(fix(install): bake DATABASE_URL) requires manualgenie installre-run on existing hosts — users have no way to know. Result: genie-serve silently spawns its own embedded pgserve instead of connecting to canonical, breakinggenie sendand other genie ops.What ships
Framework (4 modules):
src/migrations/index.ts— orchestratorsrc/migrations/runner.ts— check → apply → validate per-step contractsrc/migrations/store.ts— atomic~/.genie/migrations.json(file-based, not PG)src/migrations/discover.ts— scansteps/, alphabetical = apply orderCLI verb
genie migratewith--dry-run,--status,--quietflags.Postinstall
scripts/postinstall-migrations.js— soft-fail hook chained after existing tmux+hook-binary postinstalls.GENIE_SKIP_MIGRATIONS=1escape hatch.Initial 2 migrations (close live breaks):
001-pm2-env-databaseurl-bake— re-bakes DATABASE_URL into pm2 genie-serve env when canonical pgserve registered (fixes5567e202upgrade gap)002-kill-embedded-pgserve-legacy— stops legacy embedded pgserve listening on non-canonical ports when canonical 8432 is healthyConstraints honored
bun installnever breaksTest plan
bun test src/migrations/__tests__/ test/migrations/→ 8/8 passgenie update --nexttriggers postinstall → both migrations apply →genie sendworks without manual interventionSibling
Companion to
namastexlabs/pgserve#66(autopg upgrade command) — same self-heal philosophy, scoped to pgserve subsystem.🤖 Generated with Claude Code