✨ Add plugin architecture (VIZ-60)

[Robdel12]

Summary

Implements a plugin architecture for the Vizzly CLI that enables packages like @vizzly-testing/storybook to register custom commands while keeping the core CLI lean.

Resolves https://github.com/vizzly-testing/vizzly/issues/80

What Changed

Core Implementation

• Plugin Loader (src/plugin-loader.js) - Auto-discovers plugins from @vizzly-testing/* packages and loads explicit plugins from config
• CLI Integration (src/cli.js) - Loads and registers plugins before command definitions
• Config Support (src/utils/config-loader.js) - Added plugins field with ESM default export handling

Features

✅ Zero-config auto-discovery - Just npm install @vizzly-testing/plugin-name and commands are available
✅ Explicit configuration - Support for local/custom plugins via vizzly.config.js
✅ Deduplication - Prevents loading the same plugin twice
✅ Shared context - Plugins get access to config, logger, and services
✅ Security - Scope restriction to @vizzly-testing/* and path validation
✅ Graceful errors - Plugin failures don't crash the CLI

Documentation & Examples

• Plugin Development Guide (docs/plugins.md) - Comprehensive documentation with examples
• Example Plugin (examples/custom-plugin/) - Working demo with 4 commands
• README Updates - Plugin ecosystem section added
• Config Example - Plugins field documented in vizzly.config.js

Testing

• 10 unit tests (tests/unit/plugin-loader.spec.js) - Coverage for discovery, loading, validation
• 3 integration tests (tests/integration/plugin-system.spec.js) - End-to-end plugin system tests
• All tests passing ✓

Usage Example

Install a plugin:

npm install @vizzly-testing/storybook

Use immediately (auto-discovered):

vizzly storybook ./storybook-static
vizzly --help  # Shows new commands automatically

Or configure explicitly:

// vizzly.config.js
export default {
  plugins: [
    '@vizzly-testing/storybook',
    './custom-plugin.js'
  ]
};

Benefits

• Independent releases - Plugins iterate without requiring CLI updates
• Smaller core - Optional features moved to plugins
• Extensible - Community can build and share plugins
• Shared infrastructure - Plugins reuse CLI config, logger, and services

Test Plan

• Plugin auto-discovery from @vizzly-testing/*
• Config-based plugin loading
• Plugin commands appear in help
• Plugin commands execute successfully
• Deduplication works correctly
• Validation prevents malformed plugins
• Build compiles without errors
• All tests passing (13/13)
• No linting errors

[claude]

Code Review: Plugin Architecture Implementation

Summary

This PR implements a well-designed plugin system that enables extensibility while maintaining security and backward compatibility. The implementation is solid with good test coverage and comprehensive documentation.

✅ Strengths

Architecture & Design

• Clean separation of concerns: Plugin loader, validation, and registration are well-separated
• Zero-config auto-discovery: Excellent UX - just npm install and plugins work
• Proper dependency injection: Plugins receive config, logger, and services through context
• Graceful error handling: Plugin failures don't crash the CLI (lines 62-72 in src/cli.js)
• Deduplication logic: Prevents the same plugin from loading twice (lines 23, 44-51 in src/plugin-loader.js)

Security

• Path traversal protection: Lines 91-99 in src/plugin-loader.js validate plugin paths
• Scope restriction: Auto-discovery limited to @vizzly-testing/* namespace
• No arbitrary code execution: Plugins must follow defined interface

Testing

• Excellent coverage: 13 tests covering discovery, loading, validation, deduplication, and integration
• Real-world scenarios: Integration tests use execSync to test actual CLI behavior
• Edge cases handled: Invalid paths, malformed plugins, registration errors all tested

Documentation

• Comprehensive plugin guide (docs/plugins.md): 376 lines covering API, best practices, examples
• Working example: examples/custom-plugin/ demonstrates 4 different command patterns
• Updated README: Clear plugin ecosystem section with usage examples

🔍 Issues & Suggestions

Critical

1. Race Condition in CLI Initialization (src/cli.js:36-72)
The manual argv parsing for config/verbose flags creates a race condition. The service container is created before plugins load, but plugins might need different service initialization.

Concern: If a plugin needs to modify service container behavior, it cannot do so because the container is already instantiated.

Suggestion: Consider lazy-loading the service container or providing a way for plugins to hook into container initialization.

High Priority

2. Security: Insufficient Path Validation (src/plugin-loader.js:91-99)
The path traversal check only validates the relative path from package.json, but doesn't validate the final resolved path.

// Current check - only validates relative path
if (pluginRelativePath.startsWith('/') || pluginRelativePath.includes('..')) {
  logger.warn(...);
  continue;
}

Issue: A malicious package could use symlinks or other filesystem tricks after this validation passes.

Suggestion: Add validation of the final resolved path:

let pluginPath = resolve(packageDir, pluginRelativePath);

// Ensure resolved path is still within package directory
if (!pluginPath.startsWith(packageDir)) {
  logger.warn(`Plugin path escapes package directory: ${packageName}`);
  continue;
}

3. Missing Error Context in Plugin Loading (src/plugin-loader.js:128-148)
When plugin loading fails, the error message doesn't indicate which plugin failed during import, making debugging difficult.

Line 144-146:

throw new Error(
  `Failed to load plugin from ${pluginPath}: ${error.message}`
);

Issue: If the error occurs during dynamic import execution (e.g., syntax error in plugin code), the stack trace is lost.

Suggestion: Preserve the original error:

let newError = new Error(`Failed to load plugin from ${pluginPath}: ${error.message}`);
newError.cause = error; // Node 16.9+ supports error.cause
throw newError;

Medium Priority

4. Plugin Version Conflicts Not Handled (src/plugin-loader.js:13-61)
If the same plugin is discovered with different versions (e.g., auto-discovered v1.0.0 vs config-specified v2.0.0), only the first one loads.

Issue: Silent version skipping could lead to unexpected behavior. A developer explicitly configuring a plugin might expect that version to be used.

Suggestion: Add version conflict detection:

if (loadedNames.has(plugin.name)) {
  let existing = plugins.find(p => p.name === plugin.name);
  logger.warn(
    `Plugin ${plugin.name} already loaded (v${existing.version}), ` +
    `skipping v${plugin.version} from config`
  );
  continue;
}

5. Missing Async Validation in Register Function (src/cli.js:64)
Plugin registration is awaited, but there's no timeout mechanism. A plugin with an infinite loop in register() would hang the CLI.

Suggestion: Add timeout protection:

let registerPromise = plugin.register(program, { config, logger, services: container });
let timeoutPromise = new Promise((_, reject) => 
  setTimeout(() => reject(new Error('Plugin registration timeout')), 5000)
);

await Promise.race([registerPromise, timeoutPromise]);

6. Service Container Lifetime Issue (src/cli.js:57)
The service container is created once at CLI startup, but individual commands might need different service configurations.

Example: The upload command might need different API service settings than the tdd command.

Suggestion: Consider creating the service container per-command or making it more dynamic.

Low Priority

7. Glob Pattern Could Be More Efficient (src/plugin-loader.js:74-80)
Using glob with absolute: true is good, but the pattern could exclude non-scoped packages earlier.

Minor optimization:

let packageJsonPaths = await glob(
  'node_modules/@vizzly-testing/*/package.json',
  {
    cwd: process.cwd(),
    absolute: true,
    ignore: ['**/node_modules/**'], // Prevent nested node_modules
  }
);

8. Package.json Parsing Error Handling (src/plugin-loader.js:84)
JSON parsing errors are caught but only logged at debug level. This might hide issues.

Suggestion: Log at warn level for invalid package.json files in @vizzly-testing/* scope:

} catch (error) {
  logger.warn(
    `Failed to parse package.json at ${pkgPath}: ${error.message}`
  );
}

9. Test Cleanup Could Be More Robust (tests/unit/plugin-loader.spec.js:29-33)
Using force: true in rmSync is good, but errors are silently ignored.

Suggestion: Log cleanup failures in CI:

} catch (error) {
  if (process.env.CI) {
    console.warn(`Failed to cleanup ${testDir}: ${error.message}`);
  }
}

10. Example Plugin Missing Error Handling (examples/custom-plugin/plugin.js:75-95)
The list-screenshots command could fail if glob is not installed.

Suggestion: Add try-catch for the import:

try {
  let { glob } = await import('glob');
} catch (error) {
  logger.error('glob package not found. Run: npm install glob');
  process.exit(1);
}

🎯 Best Practices

Positive Patterns

• ✅ ESM dynamic imports with proper error handling
• ✅ Comprehensive JSDoc comments
• ✅ Validation-first approach
• ✅ Consistent logging levels
• ✅ Mock-based unit tests that don't require filesystem

Suggested Improvements

• Consider adding plugin lifecycle hooks (onLoad, onUnload)
• Add telemetry for plugin usage metrics (if privacy-compliant)
• Consider supporting plugin dependencies (plugin A requires plugin B)
• Add vizzly plugin list command to show loaded plugins

📊 Test Coverage Assessment

Well-Covered

• ✅ Auto-discovery from node_modules
• ✅ Config-based loading
• ✅ Path validation
• ✅ Deduplication
• ✅ Malformed plugin handling

Missing Coverage

• ❌ Plugin version conflicts
• ❌ Plugin registration timeout scenarios
• ❌ Concurrent plugin loading
• ❌ Plugin unloading/cleanup
• ❌ ESM vs CommonJS plugin formats

🚀 Performance Considerations

Current: All plugins load synchronously during CLI startup. For 10+ plugins, this could add noticeable latency.

Future optimization: Consider lazy-loading plugins when their commands are invoked:

program
  .command('storybook')
  .action(async () => {
    let plugin = await loadPlugin('@vizzly-testing/storybook');
    await plugin.execute();
  });

📝 Documentation Quality

The documentation is excellent! The 376-line plugin guide covers:

• ✅ Clear plugin structure
• ✅ Multiple examples
• ✅ Best practices
• ✅ Troubleshooting
• ✅ Security considerations

Minor suggestion: Add a "Plugin Development Checklist" section for quick reference.

🎉 Overall Assessment

This is a high-quality implementation that demonstrates strong software engineering practices:

• Clean architecture with proper separation of concerns
• Security-conscious design with path validation
• Comprehensive testing (13 tests, multiple scenarios)
• Excellent documentation
• Graceful error handling
• Good extensibility story

The issues noted above are mostly preventive suggestions rather than critical bugs. The implementation is production-ready with the high-priority security fix applied.

Recommendation: ✅ Approve with Minor Changes

Required before merge:

1. Fix path validation security issue (♻️ Run lint before tests #2 above)

Recommended for follow-up PR:
2. Add plugin version conflict detection (#4)
3. Add plugin registration timeout (#5)
4. Improve error context preservation (#3)

Great work on this feature! The plugin system will significantly improve Vizzly's extensibility. 🎊

---

Generated with Claude Code - Review based on CLAUDE.md repository guidelines