fix(code-reviews): review .sql migration files for production safety#982
Merged
alex-alecu merged 4 commits intomainfrom Mar 11, 2026
Merged
fix(code-reviews): review .sql migration files for production safety#982alex-alecu merged 4 commits intomainfrom
alex-alecu merged 4 commits intomainfrom
Conversation
The reviewer prompt told the agent to skip all migrations. This meant dangerous DDL like CREATE INDEX without CONCURRENTLY went unflagged. Narrow the skip rule to migration snapshots/journals only and add explicit checks for table-locking DDL, NOT NULL without DEFAULT, column drops, unbatched backfills, and missing partial indexes.
Contributor
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Reviewed by gpt-5.4-20260305 · 322,950 tokens |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
…for .sql migrations The skip list said 'migrations' which contradicted the new DB migration review checklist. Narrow it to 'migration snapshots & journals' so .sql files are consistently reviewed.
RSO
approved these changes
Mar 11, 2026
Contributor
RSO
left a comment
RSO
left a comment
There was a problem hiding this comment.
Not entirely sure how I feel about hard-coding stuff like this in the prompt. That can get hairy real quick if we want to also worry about MySQL/Redis/whatever.
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
meta/_journal.json,meta/*_snapshot.json,migrations.js) is still skipped, but hand-written.sqlmigration files are reviewedCONCURRENTLY,NOT NULLwithoutDEFAULT, column drops, unbatched backfills, missing partial indexesContext: PR #979 added a
CREATE INDEXon a high-write table. A human caught the missingCONCURRENTLYbut the code reviewer skipped the file entirely because the prompt said to skip all migrations.Verification
pnpm typecheckpassespnpm test generate-prompt— all 14 tests passVisual Changes
N/A
Reviewer Notes
The key change in the
whatToReviewfield is the "Skip these" bullet:Generated files (lock files, migrations)→Generated files (lock files, migration snapshots & journals). This removes the contradiction with the new "Database migrations (.sql files — DO review these)" section that was flagged by both sentry[bot] and kilo-code-bot[bot].