[#438] Add DB CI Migration to remove Dependency on Dev Lead#439
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughReplaces manual Changes
Sequence DiagramsequenceDiagram
actor Dev as Developer
participant PR as Pull Request
participant CI_Check as migration_check
participant CI_Fresh as migration_apply_fresh
participant Build as build
participant CI_Smoke as migration_upgrade_smoke
participant Prod as migrate_prod
Dev->>PR: Commit schema changes + run `pnpm db:generate` and commit generated files
PR->>CI_Check: Start migration validation
CI_Check->>CI_Check: Run `pnpm db:generate` and fail if diffs exist
CI_Check-->>CI_Fresh: pass
CI_Fresh->>CI_Fresh: Apply migrations to fresh DB twice (idempotency)
CI_Fresh-->>Build: pass
Build->>CI_Smoke: artifact available
CI_Smoke->>CI_Smoke: materialize base schema, restore sanitized prod-like data, run `db:migrate` twice
CI_Smoke-->>PR: pass
alt Merged to main & ENABLE_PROD_DB_MIGRATIONS == 'true'
PR->>Prod: trigger prod migration
Prod->>Prod: run `pnpm db:migrate` against PROD_DATABASE_URL
Prod-->>Dev: production schema updated
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 6 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/db/scripts/get_prod_db.ts (1)
78-100:⚠️ Potential issue | 🟠 MajorAvoid shell-interpolating DB connection parameters in
psqlcommands.Lines 79 and 99 construct shell commands using template literals with database connection fields (
${host},${port},${user},${database},${objectName}). This breaks with special characters and creates an injection surface—e.g., a hostname with backticks could execute arbitrary code.Use
execFile()with argv to pass arguments safely, avoiding shell interpretation entirely:Suggested approach
-import { exec } from "child_process"; +import { execFile, spawn } from "child_process"; ... -const execAsync = promisify(exec); +const execFileAsync = promisify(execFile); ... // For the truncate operation (line 78): - await execAsync( - `psql -v ON_ERROR_STOP=1 -h ${host} -p ${port} -U ${user} -d ${database} << 'EOF' - ...SQL... - EOF`, + await execFileAsync( + "psql", + ["-v", "ON_ERROR_STOP=1", "-h", host, "-p", port, "-U", user, "-d", database, "-c", + "SET session_replication_role = replica; DO $$ ... END $$; SET session_replication_role = DEFAULT;"], { env: envN }, ); // For the restore operation (line 98): - await execAsync( - `psql -v ON_ERROR_STOP=1 -h ${host} -p ${port} -U ${user} ${database} < ${objectName}`, + const restore = spawn( + "psql", + ["-v", "ON_ERROR_STOP=1", "-h", host, "-p", port, "-U", user, "-d", database], + { env: envN, stdio: ["pipe", "inherit", "inherit"] }, ); + fs.createReadStream(objectName).pipe(restore.stdin);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/scripts/get_prod_db.ts` around lines 78 - 100, The code currently shell-interpolates DB params into psql command strings inside execAsync (see execAsync calls around the TRUNCATE heredoc and the file import using ${objectName}), which risks injection and breaking on special chars; replace these with a non-shell exec approach (e.g., execFile or child_process.spawn) passing psql as the executable and args as an array (["-v","ON_ERROR_STOP=1","-h", host, "-p", String(port), "-U", user, "-d", database, "-c", "<SQL statement>"] for the TRUNCATE block instead of a heredoc, and ["-v","ON_ERROR_STOP=1","-h", host, "-p", String(port), "-U", user, database, "-f", objectName] or stream the file to stdin for the import), ensure you pass env: envN unchanged, and remove direct template interpolation of host/port/user/database/objectName in shell commands.
🧹 Nitpick comments (2)
docs/GETTING-STARTED.md (1)
84-88: Adddb:migrateafterdb:generatein the schema-change flow.At Line 84, contributors are told to generate and commit migrations, but applying them locally is implied rather than explicit. Adding
pnpm db:migratehere makes the test loop clearer and catches migration SQL issues earlier.🛠️ Suggested update
When you change files in `packages/db/src/schemas`, generate a new migration and commit it: ```bash pnpm db:generate +pnpm db:migrate</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/GETTING-STARTED.mdaround lines 84 - 88, Update the schema-change
instructions to run the migration after generating it: after thepnpm db:generatestep add an explicitpnpm db:migratestep so contributors both
create and apply the migration locally (ensure the docs showpnpm db:generate
followed bypnpm db:migrateand instruct committing the generated migration).</details> </blockquote></details> <details> <summary>docs/ARCHITECTURE.md (1)</summary><blockquote> `81-85`: **Clarify package paths now that migrations are part of the source of truth.** Line 81 adds migrations, but Line 82 still reads as if `@forge/db` is only under `packages/db/src/schemas/`. Consider listing both schema and migration locations to avoid confusion. <details> <summary>📝 Suggested doc tweak</summary> ```diff -- Located in `packages/db/src/schemas/` +- Schemas live in `packages/db/src/schemas/`, and committed migrations live in `packages/db/drizzle/`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ARCHITECTURE.md` around lines 81 - 85, Update the ARCHITECTURE.md wording to list both schema source locations so readers know migrations are now part of the source of truth: mention that `@forge/db` includes committed SQL migrations under packages/db/drizzle/ (or similar migrations directory) as well as schema definitions under packages/db/src/schemas/, and clarify the developer workflow (pnpm db:migrate then pnpm db:generate) so the generated files in packages/db/drizzle/ are reviewed source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/db/scripts/get_prod_db.ts`:
- Around line 78-100: The code currently shell-interpolates DB params into psql
command strings inside execAsync (see execAsync calls around the TRUNCATE
heredoc and the file import using ${objectName}), which risks injection and
breaking on special chars; replace these with a non-shell exec approach (e.g.,
execFile or child_process.spawn) passing psql as the executable and args as an
array (["-v","ON_ERROR_STOP=1","-h", host, "-p", String(port), "-U", user, "-d",
database, "-c", "<SQL statement>"] for the TRUNCATE block instead of a heredoc,
and ["-v","ON_ERROR_STOP=1","-h", host, "-p", String(port), "-U", user,
database, "-f", objectName] or stream the file to stdin for the import), ensure
you pass env: envN unchanged, and remove direct template interpolation of
host/port/user/database/objectName in shell commands.
---
Nitpick comments:
In `@docs/ARCHITECTURE.md`:
- Around line 81-85: Update the ARCHITECTURE.md wording to list both schema
source locations so readers know migrations are now part of the source of truth:
mention that `@forge/db` includes committed SQL migrations under
packages/db/drizzle/ (or similar migrations directory) as well as schema
definitions under packages/db/src/schemas/, and clarify the developer workflow
(pnpm db:migrate then pnpm db:generate) so the generated files in
packages/db/drizzle/ are reviewed source of truth.
In `@docs/GETTING-STARTED.md`:
- Around line 84-88: Update the schema-change instructions to run the migration
after generating it: after the `pnpm db:generate` step add an explicit `pnpm
db:migrate` step so contributors both create and apply the migration locally
(ensure the docs show `pnpm db:generate` followed by `pnpm db:migrate` and
instruct committing the generated migration).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: c0055da0-cba0-4603-ad3d-265d8722459d
📒 Files selected for processing (13)
.github/PULL_REQUEST_TEMPLATE.github/workflows/ci.ymlCONTRIBUTING.mddocs/ARCHITECTURE.mddocs/GETTING-STARTED.mdpackage.jsonpackages/db/README.mdpackages/db/drizzle.config.tspackages/db/drizzle/0000_petite_sunfire.sqlpackages/db/drizzle/meta/0000_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/package.jsonpackages/db/scripts/get_prod_db.ts
DGoel1602
reviewed
Apr 12, 2026
Contributor
DGoel1602
left a comment
DGoel1602
left a comment
There was a problem hiding this comment.
Mostly lgtm I'm glad we have a real pipeline for this now
| @@ -0,0 +1,13 @@ | |||
| { | |||
Contributor
There was a problem hiding this comment.
This file probably shouldn't be commited
Contributor
Author
There was a problem hiding this comment.
this is necessary for the migration logic
| //Imma be real ts was GPT pls lmk if its cooked | ||
| await execAsync( | ||
| `psql -h localhost -p ${port} -U ${user} -d local << 'EOF' | ||
| `psql -v ON_ERROR_STOP=1 -h ${host} -p ${port} -U ${user} -d ${database} << 'EOF' |
Contributor
There was a problem hiding this comment.
I was using localhost so that admins didnt accidently push it backups to prod, is there a reason to change this? I get changing local to database but why change host
Contributor
Author
There was a problem hiding this comment.
good catch, this was from when i was thinking of using this job in the CI in ephemeral stuff but changed my mind on that
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.
Why
PRs would be stuck in review hell until I was able to be in front of an editor to properly apply db:push commands before approving a merge. This PR fixes that dependency on me through a series of CI steps.
What
Closes: #438
Test Plan
Going to need to merge this in with the migration set to false right now, so the job will pass even thought it is skipped. Then I will need to do some manual migrations to update the drizzle state, verify, and we should be good to have this run on every future PR :)
Checklist
db:pushbefore mergingSummary by CodeRabbit
New Features
Documentation