Conversation
This commit adds support for installing Claude Code plugins via a new `plugins` input parameter.
Changes:
- Added `plugins` input to action.yml (comma-separated list)
- Created `install-plugins.ts` with plugin installation logic
- Added comprehensive tests in `install-plugins.test.ts`
- Updated base-action index.ts to call plugin installation
- Plugins are installed after settings setup but before Claude execution
Usage example:
```yaml
- uses: anthropic-ai/claude-code-action@main
with:
plugins: "feature-dev,test-coverage-reviewer"
```
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
|
@claude review the code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| ), | ||
| ).toEqual(["feature-dev", "test-coverage-reviewer", "code-quality"]); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Critical: Missing test coverage for core functionality
Current tests only cover parsePlugins() (11 tests), but provide zero coverage for:
installPlugin()- subprocess spawning, error handling, process failuresinstallPlugins()- sequential execution, error propagation- Integration with
index.ts
High-risk untested scenarios:
- Process exits with non-zero code
- Process spawn fails (ENOENT)
- Null exit codes
- Multiple plugin failures
- Security: malicious plugin names (
--version,../../etc/passwd)
Recommend adding tests with mocked spawn():
import { spawn } from "child_process";
import { mock } from "bun:test";
describe("installPlugin", () => {
test("should reject when process exits with non-zero code", async () => {
// Mock spawn to emit exit code 1
await expect(installPlugin("bad-plugin")).rejects.toThrow(
"Failed to install plugin 'bad-plugin' (exit code: 1)"
);
});
test("should reject malicious plugin names", async () => {
await expect(installPlugin("--version")).rejects.toThrow("Invalid plugin name");
});
});Without these tests, production failures are likely to go undetected until runtime.
Code Review SummaryI've completed a comprehensive review using specialized agents. Here are the key findings: Critical Issues Requiring Attention
Documentation Gaps
Positive Aspects
RecommendationsBefore merging:
Post-merge:
The implementation is solid overall, but addressing the validation and test coverage gaps is critical for production readiness. |
|
Claude finished @whyuan-cc's task —— View job Code Review Complete ✅I've completed a thorough review of the plugin installation feature. The implementation is functional but has several critical issues that need to be addressed before merging. 🔴 Critical Issues1. Security: Missing Input Validation (
|
base-action/src/install-plugins.ts
Outdated
| console.log(`Installing ${plugins.length} plugin(s) in parallel...`); | ||
|
|
||
| const results = await Promise.allSettled( | ||
| plugins.map(async (plugin) => { | ||
| console.log(`Installing plugin: ${plugin}`); | ||
| try { | ||
| await installPlugin(plugin, claudeExecutable); | ||
| console.log(`✓ Successfully installed: ${plugin}`); | ||
| return { plugin, status: 'success' as const }; | ||
| } catch (error) { | ||
| console.error(`✗ Failed to install: ${plugin}`); | ||
| return { plugin, status: 'failed' as const, error }; | ||
| } | ||
| }) | ||
| ); | ||
|
|
||
| const failed = results.filter(r => r.status === 'rejected' || | ||
| (r.status === 'fulfilled' && r.value.status === 'failed')); | ||
|
|
||
| if (failed.length > 0) { |
There was a problem hiding this comment.
CRITICAL: Missing Resource Limits & Timeout Protection
Two significant issues:
-
Unbounded Parallelism: All plugins install simultaneously without concurrency limits. With 20 plugins, this spawns 20 concurrent processes, potentially overwhelming the runner.
-
No Timeouts: A single hanging installation will block indefinitely.
Recommendation: Implement concurrency limiting and timeouts:
import pLimit from 'p-limit';
const PLUGIN_INSTALL_TIMEOUT = 5 * 60 * 1000; // 5 minutes per plugin
const MAX_CONCURRENT_INSTALLS = 3;
const MAX_PLUGINS = 10;
if (plugins.length > MAX_PLUGINS) {
throw new Error(`Too many plugins (${plugins.length}). Maximum allowed: ${MAX_PLUGINS}`);
}
console.log(`Installing ${plugins.length} plugin(s) (max ${MAX_CONCURRENT_INSTALLS} concurrent)...`);
const limit = pLimit(MAX_CONCURRENT_INSTALLS);
const results = await Promise.allSettled(
plugins.map((plugin) =>
limit(() => Promise.race([
installPlugin(plugin, claudeExecutable),
new Promise((_, reject) =>
setTimeout(() => reject(new Error(`Timeout installing ${plugin}`)), PLUGIN_INSTALL_TIMEOUT)
)
]))
)
);
const failed = results.filter((r): r is PromiseRejectedResult => r.status === 'rejected');
if (failed.length > 0) {
const failureDetails = failed.map((f, i) =>
` - ${plugins[i]}: ${f.reason instanceof Error ? f.reason.message : String(f.reason)}`
).join('\n');
throw new Error(`Failed to install ${failed.length} plugin(s):\n${failureDetails}`);
}
| } | ||
|
|
||
| console.log("All plugins installed successfully"); |
There was a problem hiding this comment.
Code Quality: Duplicate Console Logging
Identical message appears twice. Remove line 98.
| } | |
| console.log("All plugins installed successfully"); | |
| console.log("All plugins installed successfully"); | |
| } |
| ), | ||
| ).toEqual(["feature-dev", "test-coverage-reviewer", "code-quality"]); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
CRITICAL: Missing Test Coverage
The test file only covers parsePlugins() (11 tests), but the two most critical functions have zero test coverage:
installPlugin()- spawns child processes, handles exit codes and errorsinstallPlugins()- orchestrates parallel installation, aggregates errors
This creates significant risk of production failures. Essential missing tests:
// Mock child_process.spawn for testing
import { spawn } from "child_process";
import { mock } from "bun:test";
describe("installPlugin", () => {
test("should succeed with exit code 0", async () => {
// Mock spawn to simulate success
await expect(installPlugin("test-plugin")).resolves.toBeUndefined();
});
test("should reject on non-zero exit code", async () => {
// Mock spawn to simulate failure
await expect(installPlugin("bad-plugin")).rejects.toThrow(
"Failed to install plugin 'bad-plugin' (exit code: 1)"
);
});
test("should reject on spawn error", async () => {
// Mock spawn to emit error event
await expect(installPlugin("test-plugin")).rejects.toThrow();
});
test("should timeout after 5 minutes", async () => {
// Test timeout behavior
});
});
describe("installPlugins", () => {
test("should handle partial failures", async () => {
// Mock some plugins to succeed, others to fail
await expect(installPlugins("good,bad")).rejects.toThrow(
"Failed to install 1 plugin(s)"
);
});
test("should install plugins in parallel with concurrency limit", async () => {
// Verify concurrent installation behavior
});
});These tests are critical before merging.
base-action/src/install-plugins.ts
Outdated
| console.log(`Installing ${plugins.length} plugin(s)...`); | ||
|
|
||
| console.log(`Installing ${plugins.length} plugin(s) in parallel...`); |
There was a problem hiding this comment.
Duplicate console log statements
These two lines both log the same information. Remove the first one and keep the more descriptive "in parallel" version.
| console.log(`Installing ${plugins.length} plugin(s)...`); | |
| console.log(`Installing ${plugins.length} plugin(s) in parallel...`); | |
| console.log(`Installing ${plugins.length} plugin(s) in parallel...`); |
| ), | ||
| ).toEqual(["feature-dev", "test-coverage-reviewer", "code-quality"]); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Critical: Missing test coverage for core functionality
While the parsePlugins() function has excellent test coverage (11 tests), there are zero tests for the actual plugin installation logic:
Missing tests:
installPlugin()- process spawning, exit codes, error handlinginstallPlugins()- orchestration, parallel execution, partial failures- Error scenarios and edge cases
Impact: The core functionality that spawns processes and handles errors is completely untested, which poses significant risk.
Recommendation: Add tests with mocked spawn:
describe("installPlugin", () => {
test("should resolve when plugin installs successfully (exit code 0)");
test("should reject with error when installation fails (non-zero exit)");
test("should reject with error on spawn error event");
test("should handle custom claude executable");
});
describe("installPlugins", () => {
test("should return early for empty plugin list");
test("should install multiple plugins in parallel");
test("should throw error if any plugin fails");
test("should include failed plugin names in error message");
});The syntax error on lines 47-49 of install-plugins.ts would likely be caught by these tests.
Code Review SummaryI've completed a comprehensive review of PR #637 using specialized agents for code quality, performance, security, testing, and documentation. Here are the key findings: Critical Issues (Must Fix Before Merge)
Important Issues
Documentation Gaps
Positive Aspects ✅
Security AssessmentThe implementation is fundamentally secure against command injection. The use of Recommendations PriorityHigh Priority:
Medium Priority: I've posted inline comments with specific suggestions on the affected lines. The feature concept is solid, but these issues should be addressed before merging. 🤖 Review generated by Claude Code with specialized review agents |
base-action/src/install-plugins.ts
Outdated
| if (failed.length > 0) { | ||
| const failedPlugins = failed | ||
| .map((r) => (r.status === "fulfilled" ? r.value.plugin : "unknown")) | ||
| .join(", "); | ||
| throw new Error( | ||
| `Failed to install ${failed.length} plugin(s): ${failedPlugins}` |
There was a problem hiding this comment.
Improve error reporting for parallel failures
When multiple plugins fail, the error only shows the count. Users need to know which plugins failed:
| if (failed.length > 0) { | |
| const failedPlugins = failed | |
| .map((r) => (r.status === "fulfilled" ? r.value.plugin : "unknown")) | |
| .join(", "); | |
| throw new Error( | |
| `Failed to install ${failed.length} plugin(s): ${failedPlugins}` | |
| const failed = results | |
| .map((r, i) => ({ result: r, plugin: plugins[i] })) | |
| .filter(({ result }) => | |
| result.status === 'rejected' || | |
| (result.status === 'fulfilled' && result.value.status === 'failed') | |
| ); | |
| if (failed.length > 0) { | |
| const failedPlugins = failed.map(({ plugin }) => plugin).join(', '); | |
| throw new Error( | |
| `Failed to install ${failed.length} plugin(s): ${failedPlugins}` | |
| ); | |
| } |
| stdio: "inherit", | ||
| }); | ||
|
|
||
| process.on("close", (code: number | null) => { | ||
| if (code === 0) { | ||
| resolve(); | ||
| } else { | ||
| reject( | ||
| new Error( | ||
| `Failed to install plugin '${pluginName}' (exit code: ${code ?? 'unknown'})`, | ||
| ), | ||
| ); | ||
| } | ||
| }); | ||
|
|
||
| process.on("error", (err: Error) => { | ||
|
|
||
| process.on("error", (err: Error) => { | ||
| reject( | ||
| new Error(`Failed to install plugin '${pluginName}': ${err.message}`), | ||
| ); | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Missing timeout handling
Plugin installations can hang indefinitely without a timeout. This could cause GitHub Actions to run until the workflow timeout (default 6 hours).
Consider adding timeout protection:
return new Promise((resolve, reject) => {
const childProcess = spawn(claudeExecutable, ["plugin", "install", pluginName], {
stdio: "inherit",
});
const timeoutHandle = setTimeout(() => {
childProcess.kill("SIGTERM");
setTimeout(() => childProcess.kill("SIGKILL"), 5000);
reject(new Error(`Plugin installation timed out: '${pluginName}'`));
}, 300000); // 5 minute timeout
childProcess.on("close", (code: number | null) => {
clearTimeout(timeoutHandle);
// ... rest of handler
});
childProcess.on("error", (err: Error) => {
clearTimeout(timeoutHandle);
// ... rest of handler
});
});
| expect(consoleSpy).toHaveBeenCalledWith("No plugins to install"); | ||
| }); | ||
|
|
||
| test("should install multiple plugins sequentially", async () => { |
There was a problem hiding this comment.
Test name doesn't match implementation
This test is named "sequentially" but the implementation uses Promise.allSettled() for parallel execution.
The test at line 276-300 ("should stop and propagate error on first failure") expects only 2 spawn calls, but with parallel execution all 3 plugins will start simultaneously.
Options:
- Change implementation to sequential with fail-fast (matching test expectations)
- Rename tests and update assertions to match parallel behavior
Current mismatch will cause test failures.
Comprehensive Code Review SummaryI've completed a thorough review of this PR using specialized review agents for code quality, performance, test coverage, documentation accuracy, and security. While the concept and test suite are excellent, the implementation file has critical structural issues that prevent it from functioning correctly. 🚨 Critical Issues (Must Fix Before Merge)The
📊 Review Results by Category
🔍 Detailed FindingsPerformance Issue: Sequential vs Parallel ExecutionThe implementation claims to install plugins "in parallel" (line 106) but the test suite explicitly validates sequential behavior:
Impact: For 5 plugins @ 10s each: Sequential takes 50s vs Parallel takes ~10s (5x performance difference) Recommendation: Implement true parallel execution using Security Assessment: ✅ PASSThe security implementation is strong:
Test Coverage: Excellent QualityThe test suite is well-written with 11 tests covering:
Missing tests:
📝 RecommendationsImmediate Actions Required:
Suggested Implementation (Parallel):See inline comments for complete code examples. The key structure should be: // installPlugins() with parallel execution
const results = await Promise.allSettled(
plugins.map(async (plugin) => {
try {
await installPlugin(plugin, claudeExecutable);
return { plugin, status: 'success' };
} catch (error) {
return { plugin, status: 'failed', error };
}
})
);
// Then collect and report failures✅ Positive Observations
📌 ConclusionCannot merge in current state due to file corruption. However, the concept, tests, and security design are all solid. Once the implementation is rewritten to match the test specifications, this will be a valuable feature addition. Estimated fix time: 1-2 hours for complete rewrite and validation. Please see the inline comments for detailed guidance on each issue. |
base-action/src/install-plugins.ts
Outdated
| } | ||
|
|
||
| // Allow @scope/package-name, package-name, package_name patterns | ||
| const validPattern = /^[@\w][\w\-./]*$/; |
There was a problem hiding this comment.
CRITICAL: Unreachable Security Validation
This early return makes lines 25-32 completely unreachable, including the control character security check on lines 26-28. This creates a security vulnerability where control characters (null bytes, newlines, tabs) are never validated.
| const validPattern = /^[@\w][\w\-./]*$/; | |
| // Allow @scope/package-name, package-name, package_name patterns | |
| const validPattern = /^[@\w][\w\-./]*$/; | |
| // Reject control characters (newlines, tabs, null bytes, etc.) | |
| if (/[\x00-\x1F\x7F]/.test(pluginName)) { | |
| return false; | |
| } | |
| return validPattern.test(pluginName); |
This also fixes the duplicate validPattern declaration.
base-action/src/install-plugins.ts
Outdated
| stdio: "inherit", | ||
| }); | ||
|
|
||
| process.on("close", (code: number | null) => { |
There was a problem hiding this comment.
CRITICAL: Duplicated Promise Implementation
Lines 68-90 duplicate the Promise implementation from lines 61-67, creating unreachable code. This appears to be a merge conflict artifact.
| process.on("close", (code: number | null) => { | |
| process.on("close", (code: number | null) => { | |
| if (code === 0) { | |
| resolve(); | |
| } else { | |
| reject( | |
| new Error( | |
| `Failed to install plugin '${pluginName}' (exit code: ${code ?? 'unknown'})`, | |
| ), | |
| ); | |
| } | |
| }); | |
| process.on("error", (err: Error) => { | |
| reject( | |
| new Error(`Failed to install plugin '${pluginName}': ${err.message}`), | |
| ); | |
| }); | |
| }); | |
| } |
Remove the duplicate Promise wrapper and complete the implementation.
| const plugins = parsePlugins(pluginsInput); | ||
|
|
||
| if (plugins.length === 0) { | ||
| console.log("No plugins to install"); |
There was a problem hiding this comment.
CRITICAL: Early Return Makes All Plugin Installation Code Unreachable
This early return statement causes lines 105-147 (the entire plugin installation logic) to be unreachable. This makes the plugin installation feature completely non-functional.
The lines below also contain multiple issues:
- Lines 107-109: Same console.log repeated 4 times
- Lines 126-143: Duplicate nested error handling
- Missing proper Promise.allSettled result handling
| console.log("No plugins to install"); | |
| if (plugins.length === 0) { | |
| console.log("No plugins to install"); | |
| return; | |
| } | |
| console.log(`Installing ${plugins.length} plugin(s) in parallel...`); | |
| const results = await Promise.allSettled( | |
| plugins.map(async (plugin) => { | |
| try { | |
| await installPlugin(plugin, claudeExecutable); | |
| console.log(`✓ Successfully installed: ${plugin}`); | |
| return { plugin, status: 'success' as const }; | |
| } catch (error) { | |
| console.error(`✗ Failed to install: ${plugin}`); | |
| return { plugin, status: 'failed' as const, error }; | |
| } | |
| }) | |
| ); | |
| const failed = results.filter( | |
| (r) => r.status === "fulfilled" && r.value.status === "failed" | |
| ); | |
| if (failed.length > 0) { | |
| const failureDetails = failed | |
| .map(r => { | |
| const value = (r as PromiseFulfilledResult<any>).value; | |
| return `${value.plugin}: ${value.error?.message || 'unknown error'}`; | |
| }) | |
| .join('; '); | |
| throw new Error( | |
| `Failed to install ${failed.length} plugin(s): ${failureDetails}` | |
| ); | |
| } | |
| console.log("All plugins installed successfully"); | |
| } |
| undefined, // homeDir | ||
| ); | ||
|
|
||
| // Install plugins if specified |
There was a problem hiding this comment.
SECURITY: Missing Validation for Claude Executable Path
The INPUT_PATH_TO_CLAUDE_CODE_EXECUTABLE environment variable is passed directly to spawn() without validation, creating a command injection vulnerability.
An attacker who can control this input could execute arbitrary commands:
path_to_claude_code_executable: "/bin/bash -c 'malicious command' #"Recommendation: Add validation before calling installPlugins():
const claudeExecutable = process.env.INPUT_PATH_TO_CLAUDE_CODE_EXECUTABLE || "claude";
// Validate executable path
if (claudeExecutable !== "claude") {
if (!claudeExecutable.startsWith('/') || /[;&|`$()<>]/.test(claudeExecutable)) {
throw new Error(`Invalid claude executable path: ${claudeExecutable}`);
}
}
await installPlugins(process.env.INPUT_PLUGINS, claudeExecutable);
| expect(consoleSpy).toHaveBeenCalledWith("No plugins to install"); | ||
| }); | ||
|
|
||
| test("should install multiple plugins sequentially", async () => { |
There was a problem hiding this comment.
TEST ISSUE: Test Name Doesn't Match Implementation Behavior
This test is named "should install multiple plugins sequentially" but the implementation uses Promise.allSettled() for parallel installation.
The test expectations are also incorrect:
- Line 249-264: Expects sequential spawn calls, but parallel would initiate all 3 simultaneously
- Line 276-300: Expects fail-fast behavior (2 spawn calls), but parallel continues all installations
Recommendation: Either:
- Update test to expect parallel behavior, OR
- Change implementation to sequential installation (which the unreachable code structure suggests was intended)
Based on the console.log message "Installing X plugin(s) in parallel...", the implementation should be parallel. Update the test accordingly.
| ).toEqual(["feature-dev", "test-coverage-reviewer", "code-quality"]); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
MISSING TEST CASE: Control Character Validation
The implementation attempts to validate control characters (lines 26-28 in install-plugins.ts), but this is never tested. Add test coverage:
test("should reject control characters", () => {
expect(isValidPluginName("plugin\x00name")).toBe(false); // null byte
expect(isValidPluginName("plugin\nname")).toBe(false); // newline
expect(isValidPluginName("plugin\tname")).toBe(false); // tab
expect(isValidPluginName("plugin\rname")).toBe(false); // carriage return
expect(isValidPluginName("plugin\x1fname")).toBe(false); // control char
});This is security-critical as control characters could enable injection attacks.
Code Review SummaryI've completed a comprehensive review of PR #637 using specialized analysis agents. Unfortunately, this PR has critical issues and should not be merged in its current state. 🚨 Critical Issues Found1. Non-Functional Implementation Due to Code CorruptionThe
This appears to be a merge conflict or incomplete refactoring that was not caught before the PR was opened. 2. Security Vulnerability: Command InjectionSeverity: CRITICAL (CWE-78) The Required fix: Add validation before spawning processes. 3. Test Suite MismatchTests expect sequential plugin installation with fail-fast behavior, but the (unreachable) implementation code uses parallel installation with 📋 Additional Issues
✅ What's Good
🔧 Required Actions Before Merge
I've posted detailed inline comments on the specific problematic code sections with suggested fixes. Recommendation: Close this PR, fix the implementation file from a clean baseline, and reopen with corrected code. |
base-action/src/install-plugins.ts
Outdated
| export function isValidPluginName(pluginName: string): boolean { | ||
| // Plugin names should: | ||
| // - Not start with - (could be a flag) | ||
| // - Not contain path traversal (../) | ||
| // - Not contain shell metacharacters that could be dangerous | ||
| // - Match common package naming conventions (alphanumeric, -, _, @, /) | ||
|
|
||
| if (pluginName.startsWith("-")) { | ||
| return false; | ||
| } | ||
|
|
||
| if (pluginName.includes("../") || pluginName.includes("..\\")) { | ||
| return false; | ||
| } | ||
|
|
||
| // Allow @scope/package-name, package-name, package_name patterns | ||
| const validPattern = /^[@\w][\w\-./]*$/; | ||
| return validPattern.test(pluginName); |
There was a problem hiding this comment.
CRITICAL: Unreachable code and duplicate logic
This function has severe code duplication issues:
- Lines 23-25: Early return makes subsequent code unreachable
- Lines 26-33: This entire block never executes (unreachable code)
- Security impact: Control character validation (lines 27-29) is bypassed completely
The control character check [\x00-\x1F\x7F] is critical security validation that's never reached, leaving the system vulnerable to null bytes, newlines, and other control characters.
Recommended fix:
export function isValidPluginName(pluginName: string): boolean {
if (pluginName.startsWith("-")) {
return false;
}
if (pluginName.includes("../") || pluginName.includes("..\\")) {
return false;
}
// Reject control characters (newlines, tabs, null bytes, etc.)
if (/[\x00-\x1F\x7F]/.test(pluginName)) {
return false;
}
// Allow @scope/package-name, package-name, package_name patterns
const validPattern = /^[@\w][\w\-./]*$/;
return validPattern.test(pluginName);
}
base-action/src/install-plugins.ts
Outdated
| return new Promise((resolve, reject) => { | ||
| const process = spawn(claudeExecutable, ["plugin", "install", pluginName], { | ||
| stdio: "inherit", | ||
| }); | ||
|
|
||
| process.on("close", (code: number | null) => { | ||
| if (code === 0) { |
There was a problem hiding this comment.
CRITICAL: Duplicate Promise implementation
Lines 62-68 create a complete Promise, then lines 69-91 duplicate the exact same implementation. The second block (lines 69-91) is unreachable code.
This appears to be an incomplete merge or refactoring. Only the first Promise implementation will ever execute.
base-action/src/install-plugins.ts
Outdated
| console.log(`Installing ${plugins.length} plugin(s) in parallel...`); | ||
|
|
||
| console.log(`Installing ${plugins.length} plugin(s) in parallel...`); | ||
| console.log(`Installing ${plugins.length} plugin(s) in parallel...`); | ||
| console.log(`Installing ${plugins.length} plugin(s) in parallel...`); |
There was a problem hiding this comment.
CRITICAL: Duplicated console.log statements
The same log message is repeated 4 times. This appears to be a copy-paste error or incomplete refactoring.
base-action/src/install-plugins.ts
Outdated
| const failed = results.filter( | ||
| (r) => r.status === "fulfilled" && r.value.status === "failed" | ||
| ); | ||
|
|
||
| if (failed.length > 0) { | ||
| if (failed.length > 0) { | ||
| const failureDetails = results | ||
| .filter(r => r.status === 'fulfilled' && r.value.status === 'failed') | ||
| .map(r => { | ||
| const value = (r as PromiseFulfilledResult<any>).value; | ||
| return `${value.plugin}: ${value.error?.message || 'unknown error'}`; | ||
| }) | ||
| .join('; '); | ||
| throw new Error( | ||
| `Failed to install ${failed.length} plugin(s): ${failureDetails}` | ||
| ); | ||
| } | ||
| .map((r) => (r.status === "fulfilled" ? r.value.plugin : "unknown")) | ||
| .join(", "); | ||
| throw new Error( | ||
| `Failed to install ${failed.length} plugin(s): ${failedPlugins}` | ||
| ); | ||
| console.log("All plugins installed successfully"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
CRITICAL: Conflicting error handling implementations
There are two different error handling patterns here:
- Lines 128-138: Detailed error messages with
failureDetails - Lines 139-143: Simpler error messages with
failedPlugins
The nested if (failed.length > 0) at line 128 inside the outer check at line 126 means lines 139-143 are unreachable. This code needs to be cleaned up to have a single, coherent error handling path.
base-action/src/install-plugins.ts
Outdated
| return new Promise((resolve, reject) => { | ||
| const process = spawn(claudeExecutable, ["plugin", "install", pluginName], { | ||
| stdio: "inherit", | ||
| }); | ||
|
|
||
| process.on("close", (code: number | null) => { | ||
| if (code === 0) { | ||
| return new Promise((resolve, reject) => { | ||
| const process = spawn(claudeExecutable, ["plugin", "install", pluginName], { | ||
| stdio: "inherit", | ||
| }); | ||
|
|
||
| process.on("close", (code: number | null) => { | ||
| if (code === 0) { | ||
| resolve(); | ||
| } else { | ||
| reject( | ||
| new Error( | ||
| `Failed to install plugin '${pluginName}' (exit code: ${code ?? 'unknown'})`, | ||
| ), | ||
| ); | ||
| } | ||
| }); | ||
|
|
||
| process.on("error", (err: Error) => { | ||
| reject( | ||
| new Error(`Failed to install plugin '${pluginName}': ${err.message}`), | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
HIGH: Missing timeout handling
The spawned process has no timeout mechanism. If a plugin installation hangs, it will block the GitHub Action indefinitely (up to the 6-hour job timeout).
Recommendation: Add timeout handling:
export async function installPlugin(
pluginName: string,
claudeExecutable: string = "claude",
): Promise<void> {
if (!isValidPluginName(pluginName)) {
throw new Error(
`Invalid plugin name: '${pluginName}'. Plugin names must not start with '-' or contain path traversal sequences.`,
);
}
return new Promise((resolve, reject) => {
const process = spawn(claudeExecutable, ["plugin", "install", pluginName], {
stdio: "inherit",
});
const timeout = setTimeout(() => {
process.kill('SIGTERM');
reject(new Error(`Plugin installation timed out: '${pluginName}'`));
}, 300000); // 5 minute timeout
const cleanup = () => {
clearTimeout(timeout);
process.removeAllListeners();
};
process.on("close", (code: number | null) => {
cleanup();
if (code === 0) {
resolve();
} else {
reject(new Error(`Failed to install plugin '${pluginName}' (exit code: ${code ?? 'unknown'})`));
}
});
process.on("error", (err: Error) => {
cleanup();
reject(new Error(`Failed to install plugin '${pluginName}': ${err.message}`));
});
});
}
| test("should stop and propagate error on first failure", async () => { | ||
| let callCount = 0; | ||
| const mockSpawn = spyOn(require("child_process"), "spawn"); | ||
|
|
||
| mockSpawn.mockImplementation(() => { | ||
| callCount++; | ||
| const mockProcess = { | ||
| on: mock((event: string, callback: (arg?: any) => void) => { | ||
| if (event === "close") { | ||
| // First plugin succeeds, second fails | ||
| const exitCode = callCount === 1 ? 0 : 1; | ||
| setTimeout(() => callback(exitCode), 0); | ||
| } | ||
| }), | ||
| } as unknown as ChildProcess; | ||
| return mockProcess; | ||
| }); | ||
|
|
||
| await expect(installPlugins("plugin1,plugin2,plugin3")).rejects.toThrow( | ||
| "Failed to install plugin 'plugin2' (exit code: 1)", | ||
| ); | ||
|
|
||
| // Should only call spawn twice (plugin1 succeeds, plugin2 fails, plugin3 never called) | ||
| expect(mockSpawn).toHaveBeenCalledTimes(2); |
There was a problem hiding this comment.
Test-Implementation Mismatch
This test expects sequential installation with early termination ("should stop and propagate error on first failure"), but the implementation in install-plugins.ts appears to use Promise.allSettled() for parallel execution.
With parallel execution:
- All 3 plugins will be attempted
mockSpawnwill be called 3 times, not 2- The error message format will be different
Either:
- Change implementation to sequential with fail-fast behavior, OR
- Update this test to reflect parallel execution behavior
Code Review SummaryI've completed a comprehensive review of PR #637 using specialized agents for code quality, performance, security, test coverage, and documentation. Unfortunately, this PR has critical issues that must be resolved before merging. 🚨 Critical IssuesThe
These issues suggest an incomplete merge conflict resolution or corrupted file state. The code will not execute as intended.
|
whyuan-cc
force-pushed
the
add-plugins-input
branch
from
9d2a603 to
f5f27d4
Compare
Adds plugin installation support to the standalone base-action. Changes: - Added `plugins` input definition (comma-separated list) - Added INPUT_PLUGINS environment variable to pass to src/index.ts 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
| declare const console: { | ||
| log: (message: string) => void; | ||
| error: (message: string) => void; | ||
| }; |
There was a problem hiding this comment.
Remove unnecessary console type declaration
This custom console type declaration is inconsistent with the rest of the codebase. Other files like setup-claude-code-settings.ts and run-claude.ts use console without custom declarations, and the project uses @actions/core for structured logging.
Recommendation: Remove this declaration entirely and use @actions/core for consistency:
import * as core from "@actions/core";
// Replace console.log with core.info
// Replace console.error with core.errorThis provides better GitHub Actions integration and log formatting.
|
|
||
| console.log(`Installing ${plugins.length} plugin(s)...`); | ||
|
|
||
| for (const plugin of plugins) { |
There was a problem hiding this comment.
Consider parallel plugin installation
Plugins are currently installed sequentially. For users specifying multiple plugins, this could add 10-30+ seconds to action execution time.
Recommendation: Install plugins in parallel using Promise.all():
console.log(`Installing ${plugins.length} plugin(s) in parallel...`);
await Promise.all(
plugins.map(async (plugin) => {
console.log(`Installing plugin: ${plugin}`);
await installPlugin(plugin, claudeExecutable);
console.log(`✓ Successfully installed: ${plugin}`);
})
);Impact: 60-70% reduction in installation time for multiple plugins.
| } | ||
| }); | ||
|
|
||
| process.on("error", (err: Error) => { |
There was a problem hiding this comment.
Improve error messages for spawn failures
When spawn fails, the error usually indicates the executable wasn't found or couldn't be started. The current error message doesn't distinguish between different failure types.
Recommendation: Enhance error messages to guide users:
process.on("error", (err: Error) => {
if ((err as NodeJS.ErrnoException).code === 'ENOENT') {
reject(
new Error(
`Failed to install plugin '${pluginName}': Claude executable '${claudeExecutable}' not found. ` +
`Ensure Claude Code is installed and the path is correct.`
)
);
} else {
reject(
new Error(`Failed to spawn plugin install process for '${pluginName}': ${err.message}`)
);
}
});
| /** | ||
| * Installs a single Claude Code plugin | ||
| */ | ||
| export async function installPlugin( |
There was a problem hiding this comment.
Add JSDoc documentation
This function is missing complete JSDoc documentation including @param, @returns, @throws, and @example tags.
Recommendation:
/**
* Installs a single Claude Code plugin by executing the Claude CLI plugin install command.
*
* @param pluginName - The name of the plugin to install (e.g., "feature-dev", "@scope/plugin-name")
* @param claudeExecutable - Path to the Claude Code executable (defaults to "claude")
* @returns A Promise that resolves when installation succeeds
* @throws {Error} When plugin installation fails or the Claude executable is not found
*
* @example
* ```typescript
* await installPlugin("feature-dev");
* await installPlugin("@myorg/custom-plugin", "/usr/local/bin/claude");
* ```
*/Similar documentation should be added to installPlugins() as well.
| ), | ||
| ).toEqual(["feature-dev", "test-coverage-reviewer", "code-quality"]); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing test coverage for critical functions
The test file only covers parsePlugins(), leaving installPlugin() and installPlugins() completely untested (0% coverage for 2 out of 3 exported functions).
Critical missing tests:
-
installPlugin()tests (process spawning, error handling):- Success with exit code 0
- Non-zero exit codes
- Spawn errors (ENOENT, EACCES)
- Null exit code handling
-
installPlugins()tests (orchestration):- No plugins provided
- Single and multiple plugin installation
- Error propagation
- Console output verification
-
Security tests:
- Plugin names with shell metacharacters
Recommendation: Add comprehensive tests for these functions before merging. This is critical for production reliability since these functions perform privileged operations (spawning processes).
Code Review SummaryI've completed a comprehensive review of PR #637 using specialized review agents. Overall, the implementation is well-structured and secure, but there are some noteworthy issues to address before merging. 🟢 Strengths
🟡 Key Issues to Address1. Test Coverage Gap (Priority: High)Only Required tests:
See inline comment on 2. Performance Opportunity (Priority: Medium)Sequential plugin installation could add 10-30+ seconds for multiple plugins. Parallel installation using See inline comment on line 73 of 3. Code Consistency (Priority: Medium)Custom console type declaration is inconsistent with the rest of the codebase. Should use See inline comment on line 9 of 4. Documentation Gaps (Priority: Low-Medium)
See inline comments for specific recommendations. 📊 Review Agent Results
✅ RecommendationRequest changes to add test coverage for 🤖 Generated with Claude Code |
| for (const plugin of plugins) { | ||
| console.log(`Installing plugin: ${plugin}`); | ||
| await installPlugin(plugin, claudeExecutable); | ||
| console.log(`✓ Successfully installed: ${plugin}`); | ||
| } |
There was a problem hiding this comment.
Performance & Error Handling: Sequential installation is inefficient and provides poor error feedback.
Issues:
- Installing plugins one-by-one adds significant CI/CD time (e.g., 5 plugins × 10s = 50s total vs 10s in parallel)
- If plugin 3 fails, users won't know plugins 1-2 succeeded
- Plugins 4-5 never get attempted
Recommendation: Use Promise.allSettled() for parallel installation with better error reporting:
const results = await Promise.allSettled(
plugins.map(async (plugin) => {
console.log(`Installing plugin: ${plugin}`);
await installPlugin(plugin, claudeExecutable);
console.log(`✓ Successfully installed: ${plugin}`);
})
);
const failures = results
.map((result, idx) => ({ result, plugin: plugins[idx] }))
.filter(({ result }) => result.status === "rejected");
if (failures.length > 0) {
failures.forEach(({ plugin, result }) => {
console.error(`✗ Failed: ${plugin} - ${result.reason}`);
});
throw new Error(
`Failed to install ${failures.length}/${plugins.length} plugin(s): ${failures.map(f => f.plugin).join(", ")}`
);
}This provides 60-80% faster installation and clear failure reporting.
| // Declare console as global for TypeScript | ||
| declare const console: { | ||
| log: (message: string) => void; | ||
| error: (message: string) => void; | ||
| }; |
There was a problem hiding this comment.
Code Quality: This console declaration should be removed.
Issues:
consoleis a global object in Node.js/Bun - this declaration is unnecessary- The type definition is incomplete (missing
warn,info,debug, etc.) - This pattern doesn't exist anywhere else in the codebase
- TypeScript already knows about console from built-in type definitions
Recommendation: Delete lines 5-9 entirely.
| #!/usr/bin/env bun | ||
|
|
||
| import { describe, test, expect } from "bun:test"; | ||
| import { parsePlugins } from "../src/install-plugins"; | ||
|
|
||
| describe("parsePlugins", () => { | ||
| test("should return empty array for undefined input", () => { | ||
| expect(parsePlugins(undefined)).toEqual([]); | ||
| }); | ||
|
|
||
| test("should return empty array for empty string", () => { | ||
| expect(parsePlugins("")).toEqual([]); | ||
| }); | ||
|
|
||
| test("should return empty array for whitespace-only string", () => { | ||
| expect(parsePlugins(" \n\t ")).toEqual([]); | ||
| }); | ||
|
|
||
| test("should parse single plugin", () => { | ||
| expect(parsePlugins("feature-dev")).toEqual(["feature-dev"]); | ||
| }); | ||
|
|
||
| test("should parse multiple plugins", () => { | ||
| expect(parsePlugins("feature-dev,test-coverage-reviewer")).toEqual([ | ||
| "feature-dev", | ||
| "test-coverage-reviewer", | ||
| ]); | ||
| }); | ||
|
|
||
| test("should trim whitespace around plugin names", () => { | ||
| expect(parsePlugins(" feature-dev , test-coverage-reviewer ")).toEqual([ | ||
| "feature-dev", | ||
| "test-coverage-reviewer", | ||
| ]); | ||
| }); | ||
|
|
||
| test("should handle spaces between commas", () => { | ||
| expect( | ||
| parsePlugins( | ||
| "feature-dev, test-coverage-reviewer, code-quality-reviewer", | ||
| ), | ||
| ).toEqual([ | ||
| "feature-dev", | ||
| "test-coverage-reviewer", | ||
| "code-quality-reviewer", | ||
| ]); | ||
| }); | ||
|
|
||
| test("should filter out empty values from consecutive commas", () => { | ||
| expect(parsePlugins("feature-dev,,test-coverage-reviewer")).toEqual([ | ||
| "feature-dev", | ||
| "test-coverage-reviewer", | ||
| ]); | ||
| }); | ||
|
|
||
| test("should handle trailing comma", () => { | ||
| expect(parsePlugins("feature-dev,test-coverage-reviewer,")).toEqual([ | ||
| "feature-dev", | ||
| "test-coverage-reviewer", | ||
| ]); | ||
| }); | ||
|
|
||
| test("should handle leading comma", () => { | ||
| expect(parsePlugins(",feature-dev,test-coverage-reviewer")).toEqual([ | ||
| "feature-dev", | ||
| "test-coverage-reviewer", | ||
| ]); | ||
| }); | ||
|
|
||
| test("should handle plugins with special characters", () => { | ||
| expect(parsePlugins("@scope/plugin-name,plugin-name-2")).toEqual([ | ||
| "@scope/plugin-name", | ||
| "plugin-name-2", | ||
| ]); | ||
| }); | ||
|
|
||
| test("should handle complex whitespace patterns", () => { | ||
| expect( | ||
| parsePlugins( | ||
| "\n feature-dev \n,\t test-coverage-reviewer\t, code-quality \n", | ||
| ), | ||
| ).toEqual(["feature-dev", "test-coverage-reviewer", "code-quality"]); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Test Coverage: Critical functions are untested.
Current Coverage:
- ✅
parsePlugins: 11 comprehensive tests (excellent!) - ❌
installPlugin: 0 tests - ❌
installPlugins: 0 tests
Missing Test Scenarios:
- Plugin installation success/failure paths
- Non-zero exit codes from claude CLI
- Process spawn errors (executable not found)
- Error propagation in sequential installation
- Empty plugin list handling in
installPlugins
Recommendation: Add integration tests for the core functionality:
describe("installPlugin", () => {
test("should reject when plugin installation fails", async () => {
await expect(installPlugin("non-existent-plugin-xyz"))
.rejects.toThrow("Failed to install plugin");
});
});
describe("installPlugins", () => {
test("should handle empty plugin list", async () => {
await expect(installPlugins("")).resolves.toBeUndefined();
});
test("should report failures clearly", async () => {
await expect(installPlugins("invalid-plugin"))
.rejects.toThrow("Failed to install");
});
});These tests would catch regressions in the process spawning logic and error handling.
| process.on("close", (code: number | null) => { | ||
| if (code === 0) { | ||
| resolve(); | ||
| } else { | ||
| reject( | ||
| new Error( | ||
| `Failed to install plugin '${pluginName}' (exit code: ${code})`, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Code Quality: Exit code handling could be more explicit about null cases.
While this works in practice, making the null handling explicit improves clarity:
process.on("close", (code: number | null) => {
if (code === 0) {
resolve();
} else {
reject(
new Error(
`Failed to install plugin '${pluginName}' (exit code: ${code ?? "unknown"})`,
),
);
}
});This makes it clear that code can be null and provides a better error message in that case.
Comprehensive Code ReviewI've completed a multi-faceted review of PR #637 using specialized review agents. Here's a summary of the findings: 🎯 Overall AssessmentThe plugin installation feature is well-implemented with clean code structure and excellent input parsing tests. However, there are several areas that need attention before merging.
|
1 participant
Summary
This PR adds support for installing Claude Code plugins via a new
pluginsinput parameter to the GitHub Action.Changes
pluginsinput to action.yml: Accepts a comma-separated list of plugin namesinstall-plugins.ts: Contains the plugin installation logic with proper error handlinginstall-plugins.test.tswith 11 test cases covering various edge casesUsage Example
Or with spaces for readability:
Test Plan
🤖 Generated with Claude Code