Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds an experimental CLI subcommand Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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 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 |
paulo
force-pushed
the
pa/pull-migrations
branch
from
08f5e37 to
ae85de3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/unit/commands/database/migration-pull.test.ts (1)
219-229: Consider adding tests for additional error scenarios.The test suite covers the happy path and API errors well. Consider adding tests for:
- Filesystem write failures (e.g.,
mockWriteFile.mockRejectedValue(new Error('EACCES')))- Network failures (e.g.,
mockFetch.mockRejectedValue(new Error('Network error')))- Path traversal in
migration.path(if the security fix is implemented)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/commands/database/migration-pull.test.ts` around lines 219 - 229, Add unit tests to migration-pull.test.ts that cover three additional failure scenarios: (1) simulate filesystem write failures by making mockWriteFile.mockRejectedValue(new Error('EACCES')) and assert migrationPull rejects with the expected error, (2) simulate network failures by making mockFetch.mockRejectedValue(new Error('Network error')) and assert migrationPull rejects with that error, and (3) if migration.path sanitization/path-traversal protection was added, add a test that supplies a migration with a malicious path (e.g., '../etc/passwd') and assert migrationPull either rejects or sanitizes the path (use migrationPull and createMockCommand() to invoke and check behavior).src/commands/database/migration-pull.ts (1)
66-70: Consider adding a timeout to the fetch request.The
fetchcall has no timeout configured. If the API is slow or unresponsive, the command will hang indefinitely, degrading user experience.♻️ Suggested improvement using AbortController
+ const controller = new AbortController() + const timeoutId = setTimeout(() => controller.abort(), 30000) + const response = await fetch(url, { headers: { Authorization: `Bearer ${token}`, }, + signal: controller.signal, }) + + clearTimeout(timeoutId)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/database/migration-pull.ts` around lines 66 - 70, The fetch call that assigns to the variable response (fetch(url, { headers: { Authorization: `Bearer ${token}` } })) has no timeout and can hang; wrap the request with an AbortController, set a reasonable timeout (e.g., 5–15s) that calls controller.abort() via setTimeout, pass signal: controller.signal into fetch, and clear the timeout on success; also catch AbortError to produce a user-friendly timeout message and ensure any existing response.ok handling still runs.
🤖 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/database/database.ts`:
- Line 157: Update the help text for the CLI option definition .option('-b,
--branch [branch]', ...) to accurately describe the real behavior: state that
omission defaults to 'production' and that passing --branch without a value uses
the local git branch; e.g. change the description to something like "Pull
migrations for a specific branch (defaults to 'production'; pass --branch with
no value to use local git branch)". Locate the option call in the database
command where .option('-b, --branch [branch]', ...) is defined and replace the
misleading text accordingly.
In `@src/commands/database/migration-pull.ts`:
- Around line 117-123: Ensure paths are validated to prevent directory traversal
and accidental deletes: resolve migrationsDirectory to an absolute canonical
path (resolve/realpath) and verify it is not a dangerous root/home/system dir
before calling rm; for each migration, reject or sanitize migration.path if it's
absolute or contains ".." segments, then build the candidate path, resolve it to
an absolute/real path and verify it starts with the canonical
migrationsDirectory before calling mkdir/join/writeFile; reference the symbols
migrationsDirectory, migration.path, rm, join, dirname, writeFile and
resolveMigrationsDirectory when applying these checks.
---
Nitpick comments:
In `@src/commands/database/migration-pull.ts`:
- Around line 66-70: The fetch call that assigns to the variable response
(fetch(url, { headers: { Authorization: `Bearer ${token}` } })) has no timeout
and can hang; wrap the request with an AbortController, set a reasonable timeout
(e.g., 5–15s) that calls controller.abort() via setTimeout, pass signal:
controller.signal into fetch, and clear the timeout on success; also catch
AbortError to produce a user-friendly timeout message and ensure any existing
response.ok handling still runs.
In `@tests/unit/commands/database/migration-pull.test.ts`:
- Around line 219-229: Add unit tests to migration-pull.test.ts that cover three
additional failure scenarios: (1) simulate filesystem write failures by making
mockWriteFile.mockRejectedValue(new Error('EACCES')) and assert migrationPull
rejects with the expected error, (2) simulate network failures by making
mockFetch.mockRejectedValue(new Error('Network error')) and assert migrationPull
rejects with that error, and (3) if migration.path sanitization/path-traversal
protection was added, add a test that supplies a migration with a malicious path
(e.g., '../etc/passwd') and assert migrationPull either rejects or sanitizes the
path (use migrationPull and createMockCommand() to invoke and check behavior).
🪄 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: 63856767-19b1-496a-ab30-8caf221471b8
📒 Files selected for processing (3)
src/commands/database/database.tssrc/commands/database/migration-pull.tstests/unit/commands/database/migration-pull.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/commands/database/migration-pull.ts (1)
117-133:⚠️ Potential issue | 🟠 MajorValidate all migration paths before deleting local migrations.
The current implementation deletes the local migrations directory (line 119) before validating individual migration paths (lines 122-129). If any migration from the API has an invalid path, the function throws after the local directory is already deleted, causing data loss.
Move path validation to occur before the
rmcall:🛠️ Proposed fix
const canonicalMigrationsDir = resolve(migrationsDirectory) + for (const migration of migrations) { + if (isAbsolute(migration.path) || migration.path.split(/[/\\]/).includes('..')) { + throw new Error(`Migration path "${migration.path}" contains invalid path segments.`) + } + const filePath = resolve(canonicalMigrationsDir, migration.path) + if (!filePath.startsWith(canonicalMigrationsDir + '/') && filePath !== canonicalMigrationsDir) { + throw new Error(`Migration path "${migration.path}" resolves outside the migrations directory.`) + } + } + await rm(canonicalMigrationsDir, { recursive: true, force: true }) for (const migration of migrations) { - if (isAbsolute(migration.path) || migration.path.split(/[/\\]/).includes('..')) { - throw new Error(`Migration path "${migration.path}" contains invalid path segments.`) - } - const filePath = resolve(canonicalMigrationsDir, migration.path) - if (!filePath.startsWith(canonicalMigrationsDir)) { - throw new Error(`Migration path "${migration.path}" resolves outside the migrations directory.`) - } - await mkdir(dirname(filePath), { recursive: true }) await writeFile(filePath, migration.content) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/database/migration-pull.ts` around lines 117 - 133, The code currently calls rm(canonicalMigrationsDir) before validating each migration.path which can delete local data if any API migration path is invalid; move the entire validation block (the checks using isAbsolute(migration.path), path.split(...).includes('..'), and the resolve+startsWith check for filePath vs canonicalMigrationsDir) to run for all migrations before invoking rm(canonicalMigrationsDir), so that you validate every migration in migrations first and only after all pass call rm, then proceed to mkdir(dirname(filePath)) and writeFile(filePath) for each migration; reference the variables/functions canonicalMigrationsDir, migrationsDirectory, migrations, isAbsolute, resolve, dirname, rm, mkdir, and writeFile to locate the relevant code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/commands/database/migration-pull.ts`:
- Around line 117-133: The code currently calls rm(canonicalMigrationsDir)
before validating each migration.path which can delete local data if any API
migration path is invalid; move the entire validation block (the checks using
isAbsolute(migration.path), path.split(...).includes('..'), and the
resolve+startsWith check for filePath vs canonicalMigrationsDir) to run for all
migrations before invoking rm(canonicalMigrationsDir), so that you validate
every migration in migrations first and only after all pass call rm, then
proceed to mkdir(dirname(filePath)) and writeFile(filePath) for each migration;
reference the variables/functions canonicalMigrationsDir, migrationsDirectory,
migrations, isAbsolute, resolve, dirname, rm, mkdir, and writeFile to locate the
relevant code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 659f1748-d20c-4536-8ee2-bf56e0db1986
📒 Files selected for processing (3)
src/commands/database/database.tssrc/commands/database/migration-pull.tstests/unit/commands/database/migration-pull.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/commands/database/database.ts
- tests/unit/commands/database/migration-pull.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/commands/database/migration-pull.test.ts`:
- Line 48: Remove the unused import symbol "join" from the import statement that
currently reads "import { join, resolve } from 'path'"; update the import to
only include "resolve" so the test file no longer imports an unused identifier
and the pipeline error is resolved.
🪄 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: 8eb9281d-e921-45ff-abfd-3de495f03708
📒 Files selected for processing (1)
tests/unit/commands/database/migration-pull.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/commands/database/migration-pull.test.ts (1)
211-218: Consider a more explicit pluralization assertion.The assertion on line 217 checks that "migrations" (plural) doesn't appear in the message. While this works with the current test setup, it could become fragile if the branch name ever contained "migrations". Consider matching the exact expected message pattern instead.
Alternative assertion approach
test('handles single migration with correct pluralization', async () => { mockFetchResponse([sampleMigrations[0]]) await migrationPull({ force: true }, createMockCommand()) - expect(logMessages[0]).toContain('Pulled 1 migration from production') - expect(logMessages[0]).not.toContain('migrations') + expect(logMessages[0]).toMatch(/Pulled 1 migration from/) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/commands/database/migration-pull.test.ts` around lines 211 - 218, The test handles single migration pluralization but uses a negative contains check which is fragile; update the assertion in the test 'handles single migration with correct pluralization' to assert the exact expected message (or use a strict regex) against logMessages[0] so it matches "Pulled 1 migration from production" exactly (e.g., replace the .toContain/.not.toContain checks with a single equality or .toMatch that matches the full string), targeting the migrationPull invocation and logMessages[0].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/unit/commands/database/migration-pull.test.ts`:
- Around line 211-218: The test handles single migration pluralization but uses
a negative contains check which is fragile; update the assertion in the test
'handles single migration with correct pluralization' to assert the exact
expected message (or use a strict regex) against logMessages[0] so it matches
"Pulled 1 migration from production" exactly (e.g., replace the
.toContain/.not.toContain checks with a single equality or .toMatch that matches
the full string), targeting the migrationPull invocation and logMessages[0].
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 58af3de1-d8d5-40aa-bb3b-5364eb8f10e2
📒 Files selected for processing (1)
tests/unit/commands/database/migration-pull.test.ts
kathmbeck
approved these changes
Apr 8, 2026
eduardoboucas
pushed a commit
that referenced
this pull request
Apr 9, 2026
🤖 I have created a release *beep* *boop* --- ## [24.11.0](v24.10.0...v24.11.0) (2026-04-09) ### Features * load connection string from env var ([#8142](#8142)) ([e522b7e](e522b7e)) * pull DB migrations ([#8139](#8139)) ([2a8f7c0](2a8f7c0)) * re-structure db commands ([#8137](#8137)) ([c28ffa3](c28ffa3)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: token-generator-app[bot] <82042599+token-generator-app[bot]@users.noreply.github.com>
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
Add
netlify db migrations pullcommand that fetches migrations from the API and overwrites the local migrations directory