fix(cli): improve compatibility for executing prisma commands #401
Conversation
WalkthroughThe PR adds an execPrisma() utility for invoking the Prisma CLI under ESM/CJS, updates schema path resolution to handle directory-configured schemas, replaces execPackage with execPrisma in DB/migrate flows, and converts several migration helpers from async to synchronous functions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
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 |
|
Claude Code is working… I'll analyze this and get back to you. |
There was a problem hiding this comment.
Pull Request Overview
This PR improves compatibility for executing Prisma commands by introducing a new execPrisma utility function that directly resolves and executes the Prisma CLI binary path instead of using npx/bunx. The changes also enhance schema directory handling and re-enable previously skipped tests.
Key changes:
- Added
execPrismafunction to handle Prisma command execution with ESM/CJS compatibility - Refactored migrate and db commands to use
execPrismainstead ofexecPackage - Enhanced
getSchemaFileto support directory-based schema configuration
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/cli/src/utils/exec-utils.ts | Added new execPrisma function with ESM/CJS module resolution for direct Prisma CLI execution |
| packages/cli/src/actions/migrate.ts | Updated to use execPrisma and changed async functions to synchronous |
| packages/cli/src/actions/db.ts | Updated to use execPrisma instead of execPackage |
| packages/cli/src/actions/action-utils.ts | Enhanced getSchemaFile to handle directory paths by appending 'schema.zmodel' |
| packages/cli/test/migrate.test.ts | Re-enabled test suite by removing .skip |
| packages/cli/test/generate.test.ts | Added test case for package.json schema directory configuration |
Comments suppressed due to low confidence (2)
packages/cli/src/actions/migrate.ts:58
- The functions
runDev,runReset,runDeploy,runStatus, andrunResolveare no longer async functions (they don't return promises), but they are still being awaited. Remove theawaitkeywords from these function calls since the functions now execute synchronously.
case 'dev':
await runDev(prismaSchemaFile, options as DevOptions);
break;
case 'reset':
await runReset(prismaSchemaFile, options as ResetOptions);
break;
case 'deploy':
await runDeploy(prismaSchemaFile, options as DeployOptions);
break;
case 'status':
await runStatus(prismaSchemaFile, options as StatusOptions);
break;
case 'resolve':
await runResolve(prismaSchemaFile, options as ResolveOptions);
break;
packages/cli/src/actions/migrate.ts:137
- The
handleSubProcessErrorfunction is duplicated in this file. The same function is already exported fromaction-utils.ts(which is imported bydb.ts). Consider importing and reusing the shared implementation fromaction-utils.tsto avoid code duplication.
function handleSubProcessError(err: unknown) {
if (err instanceof Error && 'status' in err && typeof err.status === 'number') {
process.exit(err.status);
} else {
process.exit(1);
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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 (1)
packages/cli/src/actions/migrate.ts (1)
67-80: Remove redundantawaitkeywords or keep functionsasyncfor consistency.The helper functions (
runDev,runReset,runDeploy,runStatus,runResolve) are now synchronous, but the mainrunfunction (lines 41, 45, 49, 53, 57) still awaits them. While not incorrect, awaiting synchronous functions is unnecessary.Consider either:
- Removing
awaitkeywords when calling these functions- Keeping the functions as
asyncfor consistency, even if they don't await internallyApply this diff to remove redundant awaits:
export async function run(command: string, options: CommonOptions) { const schemaFile = getSchemaFile(options.schema); const prismaSchemaDir = options.migrations ? path.dirname(options.migrations) : undefined; const prismaSchemaFile = await generateTempPrismaSchema(schemaFile, prismaSchemaDir); try { switch (command) { case 'dev': - await runDev(prismaSchemaFile, options as DevOptions); + runDev(prismaSchemaFile, options as DevOptions); break; case 'reset': - await runReset(prismaSchemaFile, options as ResetOptions); + runReset(prismaSchemaFile, options as ResetOptions); break; case 'deploy': - await runDeploy(prismaSchemaFile, options as DeployOptions); + runDeploy(prismaSchemaFile, options as DeployOptions); break; case 'status': - await runStatus(prismaSchemaFile, options as StatusOptions); + runStatus(prismaSchemaFile, options as StatusOptions); break; case 'resolve': - await runResolve(prismaSchemaFile, options as ResolveOptions); + runResolve(prismaSchemaFile, options as ResolveOptions); break; } } finally {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/cli/src/actions/action-utils.ts(1 hunks)packages/cli/src/actions/db.ts(2 hunks)packages/cli/src/actions/migrate.ts(2 hunks)packages/cli/src/utils/exec-utils.ts(2 hunks)packages/cli/test/generate.test.ts(1 hunks)packages/cli/test/migrate.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
packages/cli/src/actions/action-utils.ts (1)
packages/cli/src/cli-error.ts (1)
CliError(4-4)
packages/cli/src/actions/db.ts (1)
packages/cli/src/utils/exec-utils.ts (1)
execPrisma(32-42)
packages/cli/test/generate.test.ts (1)
packages/cli/test/utils.ts (2)
createProject(12-18)runCli(20-23)
packages/cli/src/actions/migrate.ts (3)
packages/cli/src/utils/exec-utils.ts (1)
execPrisma(32-42)packages/cli/src/actions/action-utils.ts (1)
handleSubProcessError(58-64)packages/cli/src/cli-error.ts (1)
CliError(4-4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (javascript-typescript)
- GitHub Check: build-test (20.x, sqlite)
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: claude-review
🔇 Additional comments (5)
packages/cli/src/actions/action-utils.ts (1)
22-30: LGTM! Directory-based schema configuration is well-implemented.The logic correctly handles the case where
pkgJsonConfig.schemapoints to a directory by looking forschema.zmodelinside it, with proper existence checks and clear error messages.packages/cli/test/migrate.test.ts (1)
12-12: Good to see tests re-enabled!Re-enabling the migrate commands test suite ensures the new
execPrisma-based implementation is properly validated.packages/cli/src/actions/db.ts (1)
2-2: LGTM! Transition toexecPrismais correct.The changes properly replace
execPackagewith the new synchronousexecPrismautility. The command format is updated to remove theprismaprefix, and the function correctly maintainsasyncforgenerateTempPrismaSchema.Also applies to: 30-30, 38-38
packages/cli/test/generate.test.ts (1)
48-61: Excellent test coverage for directory-based schema configuration!This test validates the new behavior where
zenstack.schemacan point to a directory containingschema.zmodel, ensuring the feature works as intended.packages/cli/src/actions/migrate.ts (1)
67-129: LGTM! Migration toexecPrismais well-executed.The conversion from async to synchronous functions aligns with the synchronous nature of
execPrisma, and the command format updates (removingprismaprefix) are correct. Error handling viahandleSubProcessErroris preserved.Note: The command injection concern for user-provided options (e.g.,
options.name,options.applied,options.rolledBack) is addressed in theexec-utils.tsreview.
|
Claude finished @ymc9's task —— View job I'll review this PR for code quality, security, performance, and test coverage. Review Checklist
Code Review Feedback🔴 Critical Security Issues1. Command Injection Vulnerability in
|
Summary by CodeRabbit
Bug Fixes
Chores
Tests