Open
Conversation
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
Comment on lines
+99
to
+105
| export function formatMigrationSql(statements: string[]): string { | ||
| return statements | ||
| .map((statement) => statement.trim().replace(/;\s*$/u, '')) | ||
| .filter(Boolean) | ||
| .join(';\n\n') | ||
| .concat(statements.length > 0 ? ';\n' : ''); | ||
| } |
There was a problem hiding this comment.
🟢 Low lib/migrations.ts:99
formatMigrationSql uses statements.length > 0 to decide whether to append a trailing semicolon, but this checks the original array length, not the length after filtering. When statements contains only whitespace strings, they are filtered out yet the condition still passes, causing the function to return '; ' instead of an empty string.
+export function formatMigrationSql(statements: string[]): string {
+ const formatted = statements
+ .map((statement) => statement.trim().replace(/;\s*$/u, ''))
+ .filter(Boolean);
+ return formatted.length > 0
+ ? formatted.join(';\n\n').concat(';\n')
+ : '';
+}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file src/lib/migrations.ts around lines 99-105:
`formatMigrationSql` uses `statements.length > 0` to decide whether to append a trailing semicolon, but this checks the original array length, not the length after filtering. When `statements` contains only whitespace strings, they are filtered out yet the condition still passes, causing the function to return `';
'` instead of an empty string.
Evidence trail:
src/lib/migrations.ts lines 99-104 at REVIEWED_COMMIT - formatMigrationSql function shows `.concat(statements.length > 0 ? ';\n' : '')` checking original array length after the chain applies `.filter(Boolean)` which removes empty strings.
WalkthroughThis PR introduces database migration management to the InsForge CLI ecosystem. It adds a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/lib/migrations.test.ts (1)
77-105: Add a regression test for explicit filename targets with duplicate sequences.Current coverage misses the case where filename-target resolution should succeed even if another file shares the same sequence number.
🧪 Proposed test addition
describe('resolveMigrationTarget', () => { const filenames = ['6_create-users.sql', '7_add-user-index.sql']; @@ it('fails when a sequence number is ambiguous', () => { expect(() => resolveMigrationTarget('6', ['6_create-users.sql', '6_create-accounts.sql']) ).toThrow(/multiple local migration files/i); }); + + it('uses the exact filename target even when sequence number is duplicated', () => { + expect( + resolveMigrationTarget( + '6_create-users.sql', + ['6_create-users.sql', '6_create-accounts.sql'], + ), + ).toEqual({ + filename: '6_create-users.sql', + sequenceNumber: 6, + name: 'create-users', + }); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/migrations.test.ts` around lines 77 - 105, Add a regression test to migrations.test.ts that verifies resolveMigrationTarget accepts an explicit filename even when another file shares the same sequence number: create a case where filenames include two files with the same leading sequence (e.g., '6_create-users.sql' and '6_create-accounts.sql') and call resolveMigrationTarget with the full filename target (e.g., '6_create-accounts.sql'), then assert the returned object has the correct filename, sequenceNumber 6, and name 'create-accounts'; this ensures resolveMigrationTarget still succeeds for explicit filename targets despite duplicate sequence numbers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/db/migrations.ts`:
- Around line 102-109: Call assertValidMigrationName(migration.name) for each
remote migration before using it to build a filepath so malicious names can't
escape migrationsDir; inside the for loop that iterates over migrations (the
loop that also calls buildMigrationFilename), insert a validation step calling
assertValidMigrationName(migration.name) immediately before invoking
buildMigrationFilename(migration.sequenceNumber, migration.name) and let the
function throw or handle errors as appropriate to stop processing invalid names.
In `@src/lib/migrations.ts`:
- Around line 99-105: formatMigrationSql currently decides whether to append the
trailing ';\n' using the original statements.length, which emits a lone
semicolon for inputs that are all-blank; modify the function to compute the
cleaned/filtered list first (e.g., map+trim+remove-empty into a local variable)
and then use that filtered array's length when deciding to append the trailing
';\n' (keep the same join delimiter ';\n\n' and trimming/semicolon-stripping
logic in the mapping step) so blank-only inputs produce an empty string instead
of ";\\n".
- Around line 139-150: The code rejects exact filename targets when there are
multiple same-sequence siblings because after verifying
filenames.includes(target) it re-resolves by sequence using
findLocalMigrationBySequence(parsedTarget.sequenceNumber, filenames); change
this to return the migration for the exact filename instead of re-resolving by
sequence — e.g., use a helper to find by filename or construct/lookup the
migration entry for target (referencing parsedTarget, target, filenames, and
replacing the call to findLocalMigrationBySequence) so an explicit filename
never triggers the "multiple local migration files" error.
---
Nitpick comments:
In `@src/lib/migrations.test.ts`:
- Around line 77-105: Add a regression test to migrations.test.ts that verifies
resolveMigrationTarget accepts an explicit filename even when another file
shares the same sequence number: create a case where filenames include two files
with the same leading sequence (e.g., '6_create-users.sql' and
'6_create-accounts.sql') and call resolveMigrationTarget with the full filename
target (e.g., '6_create-accounts.sql'), then assert the returned object has the
correct filename, sequenceNumber 6, and name 'create-accounts'; this ensures
resolveMigrationTarget still succeeds for explicit filename targets despite
duplicate sequence numbers.
🪄 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
Run ID: 9087ec1f-6fa5-4a2a-9761-9ba6ad7cb1da
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
docs/specs/2026-04-17-db-migrations-command-design.mdpackage.jsonsrc/commands/db/migrations.tssrc/index.tssrc/lib/migrations.test.tssrc/lib/migrations.tssrc/types.ts
Comment on lines
+102
to
+109
| for (const migration of [...migrations].sort( | ||
| (left, right) => left.sequenceNumber - right.sequenceNumber, | ||
| )) { | ||
| const filename = buildMigrationFilename( | ||
| migration.sequenceNumber, | ||
| migration.name, | ||
| ); | ||
| const filePath = join(migrationsDir, filename); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Verify where remote migration names are converted to file paths:"
rg -n -C4 "buildMigrationFilename\\(|writeFileSync\\(|migration\\.name" src/commands/db/migrations.ts
echo
echo "Verify validation usage points:"
rg -n -C2 "assertValidMigrationName\\(" src/commands/db/migrations.tsRepository: InsForge/CLI
Length of output: 2368
🏁 Script executed:
# Get more context around the fetch flow to understand error handling pattern
sed -n '95,125p' src/commands/db/migrations.tsRepository: InsForge/CLI
Length of output: 1038
🏁 Script executed:
# Check how errors are typically handled in this file for consistency
rg -n "throw new CLIError|warn\\(|logger" src/commands/db/migrations.ts | head -20Repository: InsForge/CLI
Length of output: 524
🏁 Script executed:
# Verify the full context of remote migration fetch and processing
sed -n '60,120p' src/commands/db/migrations.tsRepository: InsForge/CLI
Length of output: 1913
🏁 Script executed:
# Check the full signature and behavior of assertValidMigrationName
sed -n '48,52p' src/commands/db/migrations.tsRepository: InsForge/CLI
Length of output: 251
🏁 Script executed:
# Look for all usages of migration.name in context of file operations
rg -n "migration\.name|migrationName" src/commands/db/migrations.tsRepository: InsForge/CLI
Length of output: 324
🏁 Script executed:
# Verify if there are other remote name sources that might need validation
rg -n "fetchRemoteMigrations|from.*api|from.*remote" src/commands/db/migrations.tsRepository: InsForge/CLI
Length of output: 418
Validate remote migration names before using in file paths.
Line 107 passes migration.name from the remote API directly to buildMigrationFilename without validation. A name containing path separators (e.g., ../../../x) could write files outside the intended .insforge/migrations/ directory. Call assertValidMigrationName(migration.name) before line 105 to harden against malicious or corrupted API responses.
🔒 Proposed hardening
for (const migration of [...migrations].sort(
(left, right) => left.sequenceNumber - right.sequenceNumber,
)) {
+ assertValidMigrationName(migration.name);
+
const filename = buildMigrationFilename(
migration.sequenceNumber,
migration.name,
);
const filePath = join(migrationsDir, filename);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/db/migrations.ts` around lines 102 - 109, Call
assertValidMigrationName(migration.name) for each remote migration before using
it to build a filepath so malicious names can't escape migrationsDir; inside the
for loop that iterates over migrations (the loop that also calls
buildMigrationFilename), insert a validation step calling
assertValidMigrationName(migration.name) immediately before invoking
buildMigrationFilename(migration.sequenceNumber, migration.name) and let the
function throw or handle errors as appropriate to stop processing invalid names.
Comment on lines
+99
to
+105
| export function formatMigrationSql(statements: string[]): string { | ||
| return statements | ||
| .map((statement) => statement.trim().replace(/;\s*$/u, '')) | ||
| .filter(Boolean) | ||
| .join(';\n\n') | ||
| .concat(statements.length > 0 ? ';\n' : ''); | ||
| } |
There was a problem hiding this comment.
Avoid emitting a lone semicolon for all-blank statements.
The trailing semicolon condition uses the original array length, not the filtered statements. For input like [' '], output becomes ";\n" unexpectedly.
🛠️ Proposed fix
export function formatMigrationSql(statements: string[]): string {
- return statements
+ const normalizedStatements = statements
.map((statement) => statement.trim().replace(/;\s*$/u, ''))
- .filter(Boolean)
- .join(';\n\n')
- .concat(statements.length > 0 ? ';\n' : '');
+ .filter(Boolean);
+
+ return normalizedStatements
+ .join(';\n\n')
+ .concat(normalizedStatements.length > 0 ? ';\n' : '');
}📝 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.
Suggested change
| export function formatMigrationSql(statements: string[]): string { | |
| return statements | |
| .map((statement) => statement.trim().replace(/;\s*$/u, '')) | |
| .filter(Boolean) | |
| .join(';\n\n') | |
| .concat(statements.length > 0 ? ';\n' : ''); | |
| } | |
| export function formatMigrationSql(statements: string[]): string { | |
| const normalizedStatements = statements | |
| .map((statement) => statement.trim().replace(/;\s*$/u, '')) | |
| .filter(Boolean); | |
| return normalizedStatements | |
| .join(';\n\n') | |
| .concat(normalizedStatements.length > 0 ? ';\n' : ''); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/migrations.ts` around lines 99 - 105, formatMigrationSql currently
decides whether to append the trailing ';\n' using the original
statements.length, which emits a lone semicolon for inputs that are all-blank;
modify the function to compute the cleaned/filtered list first (e.g.,
map+trim+remove-empty into a local variable) and then use that filtered array's
length when deciding to append the trailing ';\n' (keep the same join delimiter
';\n\n' and trimming/semicolon-stripping logic in the mapping step) so
blank-only inputs produce an empty string instead of ";\\n".
Comment on lines
+139
to
+150
| const parsedTarget = parseMigrationFilename(target); | ||
| if (!parsedTarget) { | ||
| throw new CLIError( | ||
| 'Migration file names must match <sequence_number>_<migration-name>.sql.', | ||
| ); | ||
| } | ||
|
|
||
| if (!filenames.includes(target)) { | ||
| throw new CLIError(`Local migration file not found: ${target}`); | ||
| } | ||
|
|
||
| return findLocalMigrationBySequence(parsedTarget.sequenceNumber, filenames); |
There was a problem hiding this comment.
Exact filename targets should not fail due to same-sequence siblings.
After Line 146 confirms the exact filename exists, Line 150 re-resolves by sequence and can throw “multiple local migration files” unnecessarily. This breaks explicit filename targeting.
✅ Proposed fix
export function resolveMigrationTarget(
target: string,
filenames: string[],
): ParsedMigrationFile {
if (/^[1-9][0-9]*$/u.test(target)) {
return findLocalMigrationBySequence(Number(target), filenames);
}
@@
if (!filenames.includes(target)) {
throw new CLIError(`Local migration file not found: ${target}`);
}
- return findLocalMigrationBySequence(parsedTarget.sequenceNumber, filenames);
+ return parsedTarget;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/migrations.ts` around lines 139 - 150, The code rejects exact
filename targets when there are multiple same-sequence siblings because after
verifying filenames.includes(target) it re-resolves by sequence using
findLocalMigrationBySequence(parsedTarget.sequenceNumber, filenames); change
this to return the migration for the exact filename instead of re-resolving by
sequence — e.g., use a helper to find by filename or construct/lookup the
migration entry for target (referencing parsedTarget, target, filenames, and
replacing the call to findLocalMigrationBySequence) so an explicit filename
never triggers the "multiple local migration files" error.
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
1 participant
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.
Add
db migrationsCLI commands for listing, fetching, creating, and applying migrationsdb migrations:list(shows remote migrations),fetch(writes remote migrations as local.sqlfiles),new <name>(creates a sequenced empty migration file), andup <target>(posts a local migration's SQL to the backend).src/lib/migrations.tsfor filename parsing, local sequence validation, directory management, and migration target resolution.<sequence>_<name>.sql;newandupenforce contiguous local sequences relative to remote state.docs/specs/2026-04-17-db-migrations-command-design.mdcovering validation rules, API dependencies, and output conventions.📊 Macroscope summarized 5112bef. 6 files reviewed, 1 issue evaluated, 0 issues filtered, 1 comment posted
🗂️ Filtered Issues
Summary by CodeRabbit
Release Notes
New Features
listto view all remote migrations,fetchto download remote migrations locally,newto create a new migration file, andupto apply a migration to the database.Tests
Chores