-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add plugins input to GitHub Action #637
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,80 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #!/usr/bin/env bun | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { spawn } from "child_process"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Declare console as global for TypeScript | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| declare const console: { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| log: (message: string) => void; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| error: (message: string) => void; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove unnecessary console type declaration This custom console type declaration is inconsistent with the rest of the codebase. Other files like Recommendation: Remove this declaration entirely and use 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.
Comment on lines
+5
to
+9
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Code Quality: This console declaration should be removed. Issues:
Recommendation: Delete lines 5-9 entirely. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Parses a comma-separated list of plugin names and returns an array of trimmed plugin names | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. DOCUMENTATION: Missing JSDoc comment for this exported function. All other exported functions have JSDoc comments.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export function parsePlugins(pluginsInput: string | undefined): string[] { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!pluginsInput || pluginsInput.trim() === "") { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return []; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return pluginsInput | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .split(",") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .map((plugin) => plugin.trim()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .filter((plugin) => plugin.length > 0); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
whyuan-cc marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+22
to
+25
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CRITICAL Security Vulnerability: The An attacker controlling path_to_claude_code_executable: "/usr/bin/curl" # or any malicious scriptRequired Fix: Add validation before line 50: if (!isValidClaudeExecutable(claudeExecutable)) {
throw new Error(
`Invalid Claude executable path: '${claudeExecutable}'. Must be 'claude' or an absolute path to a claude binary in a safe directory.`,
);
}And add this validation function: function isValidClaudeExecutable(executablePath: string): boolean {
if (executablePath === "claude") return true;
if (path.isAbsolute(executablePath)) {
const normalized = path.normalize(executablePath);
if (normalized.includes("..")) return false;
// Reject writable system directories
const dangerousDirs = ["/tmp/", "/var/tmp/", "/dev/"];
if (dangerousDirs.some(dir => normalized.startsWith(dir))) return false;
// Ensure filename contains "claude"
if (!path.basename(normalized).startsWith("claude")) return false;
return true;
}
return false;
} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Installs a single Claude Code plugin | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export async function installPlugin( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add JSDoc documentation This function is missing complete JSDoc documentation including 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 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pluginName: string, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Medium Security Issue: claudeExecutable parameter not validated The Recommendation: Add validation: function isValidExecutablePath(path: string): boolean {
const allowedNames = ['claude', 'claude-code'];
const basename = path.split('/').pop() || '';
if (!path.includes('/')) {
return allowedNames.includes(path);
}
return (path.startsWith('/usr/local/bin/') ||
path.startsWith('/home/')) &&
allowedNames.includes(basename);
}Then validate before spawning: if (!isValidExecutablePath(claudeExecutable)) {
throw new Error(`Invalid executable path: '${claudeExecutable}'`);
}Reference: CWE-78 (OS Command Injection) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| claudeExecutable: string = "claude", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): Promise<void> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return new Promise((resolve, reject) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const process = spawn(claudeExecutable, ["plugin", "install", pluginName], { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
whyuan-cc marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| stdio: "inherit", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| process.on("close", (code: number | null) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (code === 0) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| resolve(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| reject( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Critical: Duplicate error handler with broken structure Lines 78-85 contain a duplicate error event handler with malformed structure. There's an empty handler on line 78, then the actual handler is nested inside it on lines 80-84. This should be: 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}`),
);
});
} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| new Error( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| `Failed to install plugin '${pluginName}' (exit code: ${code})`, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
whyuan-cc marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
+37
to
+45
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| process.on("error", (err: Error) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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}`)
);
}
}); |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| reject( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| new Error(`Failed to install plugin '${pluginName}': ${err.message}`), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Documentation Issue: JSDoc doesn't mention parallel execution The implementation uses
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Installs Claude Code plugins from a comma-separated list | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export async function installPlugins( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+55
to
+60
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Syntax Error: Multiple issues here:
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pluginsInput: string | undefined, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| claudeExecutable: string = "claude", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): Promise<void> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Critical: Broken control flow and duplicate console.log The This should be: if (plugins.length === 0) {
console.log("No plugins to install");
return;
}
console.log(`Installing ${plugins.length} plugin(s) in parallel...`); |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const plugins = parsePlugins(pluginsInput); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+60
to
+65
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CRITICAL: Syntax error - missing closing brace and duplicate console.log There's a missing
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (plugins.length === 0) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+62
to
+66
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Critical: Duplicate console.log and missing return Lines 100 and 102 both log "Installing plugins in parallel". Remove the duplicate:
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Duplicate console.log statement This line is a duplicate of line 101. Remove this redundant logging.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| console.log("No plugins to install"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Duplicate console.log statement Two nearly identical log messages on lines 102 and 104. Remove one or make them meaningfully different.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| console.log(`Installing ${plugins.length} plugin(s)...`); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for (const plugin of plugins) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| console.log(`Installing plugin: ${plugin}`); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| await installPlugin(plugin, claudeExecutable); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| console.log(`✓ Successfully installed: ${plugin}`); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+73
to
+77
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Performance & Error Handling: Sequential installation is inefficient and provides poor error feedback. Issues:
Recommendation: Use 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. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| console.log("All plugins installed successfully"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| #!/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"]); | ||
| }); | ||
| }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Critical: Missing test coverage for core functionality Current tests only cover
High-risk untested scenarios:
Recommend adding tests with mocked 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CRITICAL: Missing Test Coverage The test file only covers
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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Critical: Missing test coverage for core functionality While the Missing tests:
Impact: The core functionality that spawns processes and handles errors is completely untested, which poses significant risk. Recommendation: Add tests with mocked 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing test coverage for critical functions The test file only covers Critical missing tests:
Recommendation: Add comprehensive tests for these functions before merging. This is critical for production reliability since these functions perform privileged operations (spawning processes).
Comment on lines
+1
to
+84
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test Coverage: Critical functions are untested. Current Coverage:
Missing Test Scenarios:
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. |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SECURITY: Missing Validation for Claude Executable Path
The
INPUT_PATH_TO_CLAUDE_CODE_EXECUTABLEenvironment variable is passed directly tospawn()without validation, creating a command injection vulnerability.An attacker who can control this input could execute arbitrary commands:
Recommendation: Add validation before calling
installPlugins():