fix(cli): report error when plugin module cannot be resolved#2447
Conversation
Previously, when a plugin module couldn't be found (e.g., package not installed), the error was silently swallowed and the plugin was skipped. Now the CLI reports a clear error message distinguishing between missing modules and other loading failures. Closes #2393 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @ymc9's task —— View job PR Review: Fix CLI plugin module resolution error reportingReview Checklist
🎯 Overall Assessment: Excellent ImplementationThis is a well-crafted fix that properly addresses issue #2393 with good error handling, clear messaging, and comprehensive test coverage. The implementation follows existing patterns and maintains backward compatibility. ✅ Strengths1. Error Handling Excellence ( const errorCode = (err as NodeJS.ErrnoException)?.code;
if (errorCode === 'ERR_MODULE_NOT_FOUND' || errorCode === 'MODULE_NOT_FOUND') {
throw new CliError(`Cannot find plugin module "${provider}". Please make sure the package exists.`);
}
2. Backward Compatibility ( // plugin may not export a generator, return undefined in that case
return mod.default as CliPlugin | undefined;
3. Comprehensive Test Coverage
✅ Code Quality1. Consistent Error Handling Pattern
2. Type Safety
3. Documentation
✅ Performance Considerations
✅ Security Assessment
🔍 Minor Suggestions1. Error Code Coverage ( if (errorCode === 'ERR_MODULE_NOT_FOUND' || errorCode === 'MODULE_NOT_FOUND' || errorCode === 'ENOENT') {
2. Test Robustness expect(() => runCli('generate', workDir)).toThrow('Cannot find plugin module "@zenstackhq/nonexistent-plugin"');📋 Test Coverage: ExceptionalThe test suite covers all three critical scenarios:
🚀 Recommendation: APPROVEThis PR is ready for merge. It:
The implementation correctly distinguishes between legitimate missing modules (which should error) and plugins that intentionally don't export CLI generators (which should continue working). This is exactly the behavior needed to fix issue #2393 without breaking existing workflows. |
📝 WalkthroughWalkthroughReplaced local plugin provider extraction and dynamic loading in the CLI with shared utilities in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
Extract `loadPluginModule` and `getPluginProvider` to shared action-utils so both `generate` and `check` commands validate plugin resolution. The `zen check` command now reports an error when a plugin module cannot be found, matching the behavior of `zen generate`. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @ymc9's task —— View job PR Code ReviewReview Checklist
🎯 Overall Assessment: Excellent ImplementationThis PR effectively addresses issue #2393 by improving plugin module error handling with clear, actionable error messages while maintaining backward compatibility. The implementation is well-structured, follows existing patterns, and includes comprehensive test coverage. ✅ Strengths1. Robust Error Handling (
|
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @ymc9's task —— View job PR Code ReviewReviewing PR #2447: "fix(cli): report error when plugin module cannot be resolved" Review Checklist
🎯 Overall Assessment: Excellent ImplementationThis PR effectively addresses issue #2393 by improving plugin module error handling with clear, actionable error messages while maintaining backward compatibility. The implementation is well-structured, follows existing patterns, and includes comprehensive test coverage. ✅ Strengths1. Robust Error Handling (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/cli/src/actions/action-utils.ts (1)
232-301: Consider adding explicit return type annotation.The function handles multiple return paths correctly but lacks an explicit return type. Adding
Promise<CliPlugin | undefined>would improve code clarity and help catch any future regressions.Suggested change
-export async function loadPluginModule(provider: string, basePath: string) { +export async function loadPluginModule(provider: string, basePath: string): Promise<CliPlugin | undefined> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/actions/action-utils.ts` around lines 232 - 301, The function loadPluginModule lacks an explicit return type; update its signature to include Promise<CliPlugin | undefined> (i.e., export async function loadPluginModule(...): Promise<CliPlugin | undefined>) so all code paths are properly typed; ensure the CliPlugin type is imported/available in this module and that helper functions (importAsEsm, importAsTs) continue to return values compatible with CliPlugin | undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/cli/src/actions/action-utils.ts`:
- Around line 232-301: The function loadPluginModule lacks an explicit return
type; update its signature to include Promise<CliPlugin | undefined> (i.e.,
export async function loadPluginModule(...): Promise<CliPlugin | undefined>) so
all code paths are properly typed; ensure the CliPlugin type is
imported/available in this module and that helper functions (importAsEsm,
importAsTs) continue to return values compatible with CliPlugin | undefined.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d8dffed1-63c2-48d3-b592-92be77db4be2
📒 Files selected for processing (4)
packages/cli/src/actions/action-utils.tspackages/cli/src/actions/check.tspackages/cli/src/actions/generate.tspackages/cli/test/check.test.ts
Validates that generate succeeds when a plugin provider points to a .zmodel file that defines custom attributes (no code module needed). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @ymc9's task —— View job PR Code ReviewReviewing PR #2447: "fix(cli): report error when plugin module cannot be resolved" Review Checklist
🎯 Overall Assessment: Excellent ImplementationThis PR effectively addresses issue #2393 by improving plugin module error handling with clear, actionable error messages while maintaining backward compatibility. The implementation is well-structured, follows existing patterns, and includes comprehensive test coverage. ✅ Strengths1. Robust Error Handling (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/cli/test/generate.test.ts (1)
216-273: Consider adding one.tsno-default plugin test for parity.This block covers the ESM/no-default path well; adding a
.tsno-default case would also cover the TS loading branch and prevent regressions there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/test/generate.test.ts` around lines 216 - 273, Add a new test mirroring "should succeed when plugin module exists but has no CLI generator" that uses a .ts plugin file (no default export) to exercise the TypeScript loader branch; in the new test createProject with a model that references provider = './my-plugin.ts', write a minimal TS module to workDir/zenstack/my-plugin.ts exporting a named symbol (e.g., export const name = 'no-generator'), call runCli('generate', workDir), and assert the default output (fs.existsSync(path.join(workDir, 'zenstack/schema.ts'))) is true, similar to the existing tests so the TS loading path is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/actions/action-utils.ts`:
- Around line 253-257: importAsTs currently uses jiti.import(spec, { default:
true }), which lets jiti fall back to the module object when no default export
exists and allows non-plugin modules to be treated as CliPlugin; update
importAsTs to require and validate a true default export (or set { default:
false } and then check for result.default) and throw a descriptive error if no
default export or if the default export does not match the expected CliPlugin
shape (e.g., required function or properties). Locate importAsTs and jiti.import
in action-utils.ts and add an explicit existence/type check for the default
export (or validate the plugin interface) before casting to CliPlugin and
returning, so invalid modules are rejected with a clear error.
- Around line 300-303: The error handling for dynamic import in action-utils.ts
is too broad: when import(moduleSpec) throws and errorCode is
'ERR_MODULE_NOT_FOUND' or 'MODULE_NOT_FOUND', parse the error message to extract
the missing package name and compare it to the requested module
(provider/moduleSpec) before converting it into a CliError that says the plugin
module was not found; if the missing package is a transitive dependency (name
differs from provider/moduleSpec) then rethrow the original error (or wrap with
the original message) instead of claiming the plugin package is missing. Locate
the import call and the current check using errorCode and provider, implement
message parsing to identify the actual missing module, and only throw the
“Cannot find plugin module \"${provider}\"” CliError when the extracted name
matches the target module.
---
Nitpick comments:
In `@packages/cli/test/generate.test.ts`:
- Around line 216-273: Add a new test mirroring "should succeed when plugin
module exists but has no CLI generator" that uses a .ts plugin file (no default
export) to exercise the TypeScript loader branch; in the new test createProject
with a model that references provider = './my-plugin.ts', write a minimal TS
module to workDir/zenstack/my-plugin.ts exporting a named symbol (e.g., export
const name = 'no-generator'), call runCli('generate', workDir), and assert the
default output (fs.existsSync(path.join(workDir, 'zenstack/schema.ts'))) is
true, similar to the existing tests so the TS loading path is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0d2bd923-e12a-4f78-bfa7-9ff85754035b
📒 Files selected for processing (4)
packages/cli/src/actions/action-utils.tspackages/cli/test/generate.test.tstests/runtimes/bun/schemas/schema.zmodeltests/runtimes/edge-runtime/schemas/schema.zmodel
1 participant
Summary
ERR_MODULE_NOT_FOUND/MODULE_NOT_FOUND) and other loading failuresplugin.zmodelfor custom attributes) continue to work as beforeCloses #2393
Test plan
Cannot find plugin moduleerrorplugin.zmodelfor custom attributes works correctly🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores