feat: add /enhance:plugin - plugin structure and tool use analyzer - #130
Conversation
Implements #119 - creates a new enhance plugin that analyzes: - Plugin.json structure and required fields - MCP tool definitions against best practices - Security patterns in agent/command files Features: - HIGH/MEDIUM/LOW certainty levels following slop-patterns model - Auto-fix capability for HIGH certainty issues - Markdown report generation - Comprehensive test suite (21 tests) New files: - plugins/enhance/ - plugin command and agent - lib/enhance/ - pattern matching, reporter, fixer modules - tests/enhance/ - unit tests
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
Summary of ChangesHello @avifenesh, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a significant enhancement by introducing a dedicated plugin analysis tool. The Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new 'Plugin Structure Analyzer' feature, accessible via the /enhance:plugin command, designed to validate plugin structures, MCP tool definitions, and detect security patterns within Claude Code plugins. The changes involve adding new modules (lib/enhance/) for a fixer, reporter, and various pattern definitions (plugin, tool, and security patterns), along with a core analyzer. The analyzer checks for issues like missing schema properties, version mismatches, tool overexposure, and security vulnerabilities such as unrestricted Bash access or hardcoded secrets, categorizing them by HIGH, MEDIUM, or LOW certainty. It also includes an auto-fix capability for HIGH certainty issues. Review comments highlight critical issues with the auto-fix functionality for commands, specifically regarding schemaPath construction needing to use array indices instead of command names, and suggest improving the robustness of package.json path resolution. Additionally, a comment points out that the autoFixFn for missing_required_fields in plugin-patterns.js is unused and misleading, as a more sophisticated fixer is already implemented elsewhere, and another comment advises against using the experimental recursive: true option in fs.readdirSync for broader Node.js version compatibility.
| for (const cmd of pluginJson.commands) { | ||
| const cmdIssues = analyzeCommand(cmd, pluginJsonFile); | ||
| results.toolIssues.push(...cmdIssues); |
There was a problem hiding this comment.
There's a critical issue here that breaks the auto-fix functionality for commands. You're iterating over pluginJson.commands (an array) but constructing the schemaPath for fixes using cmd.name instead of the array index. The applyAtPath fixer function expects a numeric index for array access and will fail when it receives a string like a command name.
To fix this, you need to:
- Pass the command's array index to
analyzeCommand. - Update
analyzeCommandto accept the index. - Use the index to construct the
schemaPath.
After applying the suggestion below, you'll also need to update the analyzeCommand signature to function analyzeCommand(cmd, filePath, index) and change how schemaPath is constructed on lines 195 and 211 to schemaPath: commands[${index}].parameters``.
for (const [index, cmd] of pluginJson.commands.entries()) {
const cmdIssues = analyzeCommand(cmd, pluginJsonFile, index);
results.toolIssues.push(...cmdIssues);
}There was a problem hiding this comment.
Fixed in 226ee2f - now using array index in schemaPath instead of command name.
| function cleanupBackups(directory) { | ||
| let count = 0; | ||
|
|
||
| const files = fs.readdirSync(directory, { recursive: true }); |
There was a problem hiding this comment.
The recursive: true option for fs.readdirSync is an experimental feature added in Node.js v18.17.0. Your package.json supports Node.js versions >=18.0.0, so this code may fail on versions between 18.0.0 and 18.16.x. For better portability, you should use a more widely supported method for recursive directory traversal, such as a manual recursive function or a library like glob.
There was a problem hiding this comment.
Fixed in 226ee2f - now checking if path is a file before unlinking.
| } | ||
|
|
||
| // Check package.json for version comparison | ||
| const packageJsonPath = path.join(pluginPath, '..', '..', 'package.json'); |
There was a problem hiding this comment.
Hardcoding the relative path to package.json with path.join(pluginPath, '..', '..', 'package.json') is brittle. It assumes that all plugins are exactly two levels deep from the project root, which might not hold true for all project structures or future refactors. For more robustness, consider searching upwards from the pluginPath to find the package.json file. Libraries like find-up are designed for this purpose and would make the implementation more resilient.
| return { | ||
| issue: 'No required fields declared', | ||
| fix: 'Add required array with all mandatory fields', | ||
| autoFixFn: (s) => ({ ...s, required: Object.keys(s.properties) }) |
There was a problem hiding this comment.
The autoFixFn defined here for missing_required_fields is a simplistic version that assumes all properties are required. The plugin-analyzer actually uses the more sophisticated fixer.fixRequiredFields function for the auto-fix. This makes the autoFixFn here unused and misleading. It would be best to remove it to avoid confusion and maintain a single source of truth for the fix logic.
There was a problem hiding this comment.
Fixed in 93c0e98 - removed misleading autoFixFn; plugin-analyzer uses fixer.fixRequiredFields which is more sophisticated.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9133afe8f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| file: filePath, | ||
| filePath: filePath, | ||
| schemaPath: `commands[${cmd.name}].parameters`, | ||
| certainty: addPropsPattern.certainty, |
There was a problem hiding this comment.
The schemaPath here uses commands[${cmd.name}], but pluginJson.commands is iterated as an array and applyAtPath in fixer.js only resolves array segments with numeric indices ((\w+)\[(\d+)\]). When a command name is used instead of an index, the fix runs against the wrong location (or no location), so --fix will silently skip the intended update for additionalProperties/required. This means HIGH-certainty auto-fixes for command schemas never apply on real plugin.json command arrays.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR adds a new /enhance:plugin command that analyzes plugin structures, MCP tool definitions, and security patterns. The implementation provides HIGH/MEDIUM/LOW certainty levels for issues and includes auto-fix capabilities for HIGH certainty issues, following the slop-patterns detection model.
Changes:
- New
plugins/enhance/plugin with command definition and agent documentation - New
lib/enhance/module containing pattern detection (plugin-patterns, tool-patterns, security-patterns), reporter, and fixer - Comprehensive test suite with 21 tests covering all pattern modules
- Integration into main library via
lib/index.js - CHANGELOG.md updated to document new features
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/enhance/plugin-analyzer.test.js | Comprehensive test suite covering pattern detection, reporting, and fixing functionality |
| plugins/enhance/commands/enhance.md | Command documentation defining arguments, workflow, detection categories, and usage examples |
| plugins/enhance/agents/plugin-enhancer.md | Agent documentation describing analysis categories, security patterns, and auto-fix implementation |
| plugins/enhance/.claude-plugin/plugin.json | Plugin manifest with metadata, version 2.7.1 |
| lib/index.js | Integration of enhance module into main library exports |
| lib/enhance/tool-patterns.js | Pattern definitions for MCP tool issues (naming, enums, nesting, strict mode, etc.) |
| lib/enhance/security-patterns.js | Security vulnerability patterns (Bash restrictions, command injection, path traversal, secrets) |
| lib/enhance/plugin-patterns.js | Plugin structure validation patterns (additionalProperties, required fields, versioning) |
| lib/enhance/reporter.js | Markdown report generation with filtering and summary capabilities |
| lib/enhance/plugin-analyzer.js | Main orchestrator for plugin analysis, coordinating pattern checks across files |
| lib/enhance/fixer.js | Auto-fix implementation for HIGH certainty issues with backup support |
| lib/enhance/index.js | Module entry point aggregating all enhance functionality |
| CHANGELOG.md | Documentation of new feature under [Unreleased] section |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ### HIGH Certainty (auto-fixable) | ||
|
|
||
| | Pattern | Description | Auto-Fix | | ||
| |---------|-------------|----------| | ||
| | Missing additionalProperties | Schema allows extra fields | Add `"additionalProperties": false` | | ||
| | Missing required fields | Parameters not marked required | Add to `required` array | | ||
| | Version mismatch | plugin.json vs package.json | Sync versions | | ||
| | Missing tool description | Tool has no description | Flag for manual fix | |
There was a problem hiding this comment.
The table on line 37 indicates "Missing tool description" has "Flag for manual fix" in the Auto-Fix column, but it's listed under "HIGH Certainty (auto-fixable)" section. This is inconsistent - if it's not auto-fixable, it should either be in a different section or the section title should be updated to reflect that not all HIGH certainty issues are auto-fixable.
| // Apply fixes if requested | ||
| if (applyFixes) { | ||
| const fixed = await pluginAnalyzer.applyFixes(results); | ||
| console.log(`Applied ${fixed.length} fixes`); |
There was a problem hiding this comment.
The implementation example shows accessing fixed.length on line 96, but based on the fixer.js implementation, the applyFixes function returns an object with structure { applied: [], skipped: [], errors: [] }, not an array. This should be fixed.applied.length instead.
| console.log(`Applied ${fixed.length} fixes`); | |
| console.log(`Applied ${fixed.applied.length} fixes`); |
| if (file.endsWith('.backup')) { | ||
| fs.unlinkSync(path.join(directory, file)); |
There was a problem hiding this comment.
The cleanupBackups function uses the 'recursive' option with fs.readdirSync, but this returns a flat list of relative paths. The code then tries to check if each entry ends with '.backup' and unlink it, but doesn't properly handle the relative path structure. When recursive is true, the returned entries are paths relative to the directory (e.g., 'subdir/file.backup'), so you need to join them with the directory before unlinking. However, line 289 correctly uses path.join. The actual issue is that 'files' will contain both files and directories when recursive is true, and trying to unlink a directory will fail. You should check if the entry is a file before attempting to unlink it.
| if (file.endsWith('.backup')) { | |
| fs.unlinkSync(path.join(directory, file)); | |
| const fullPath = path.join(directory, file); | |
| const stat = fs.statSync(fullPath); | |
| if (stat.isFile() && file.endsWith('.backup')) { | |
| fs.unlinkSync(fullPath); |
| } | ||
|
|
||
| // Check package.json for version comparison | ||
| const packageJsonPath = path.join(pluginPath, '..', '..', 'package.json'); |
There was a problem hiding this comment.
The package.json path construction uses '../..' to go up two levels from the plugin directory. This assumes a specific directory structure (pluginsDir/pluginName/...) but may be fragile. If the plugin directory is at 'plugins/enhance', going up two levels would be at the root, but this assumes 'plugins' is directly under the root. Consider making the root directory or package.json path configurable or using a more reliable method to find the package.json file.
| pluginJson = JSON.parse(fs.readFileSync(pluginJsonPath, 'utf8')); | ||
| pluginJsonFile = pluginJsonPath; | ||
| results.filesScanned++; | ||
| } else if (fs.existsSync(altPluginJsonPath)) { | ||
| pluginJson = JSON.parse(fs.readFileSync(altPluginJsonPath, 'utf8')); | ||
| pluginJsonFile = altPluginJsonPath; | ||
| results.filesScanned++; | ||
| } | ||
|
|
||
| // Check package.json for version comparison | ||
| const packageJsonPath = path.join(pluginPath, '..', '..', 'package.json'); | ||
| let packageJson = null; | ||
| if (fs.existsSync(packageJsonPath)) { | ||
| packageJson = JSON.parse(fs.readFileSync(packageJsonPath, 'utf8')); |
There was a problem hiding this comment.
JSON.parse operations on lines 42, 46, and 55 lack error handling. If any of these JSON files are malformed, the function will throw an uncaught exception and crash instead of reporting the issue gracefully. Consider wrapping these operations in try-catch blocks to handle parse errors and report them as analysis issues.
| pluginJson = JSON.parse(fs.readFileSync(pluginJsonPath, 'utf8')); | |
| pluginJsonFile = pluginJsonPath; | |
| results.filesScanned++; | |
| } else if (fs.existsSync(altPluginJsonPath)) { | |
| pluginJson = JSON.parse(fs.readFileSync(altPluginJsonPath, 'utf8')); | |
| pluginJsonFile = altPluginJsonPath; | |
| results.filesScanned++; | |
| } | |
| // Check package.json for version comparison | |
| const packageJsonPath = path.join(pluginPath, '..', '..', 'package.json'); | |
| let packageJson = null; | |
| if (fs.existsSync(packageJsonPath)) { | |
| packageJson = JSON.parse(fs.readFileSync(packageJsonPath, 'utf8')); | |
| try { | |
| const pluginJsonContent = fs.readFileSync(pluginJsonPath, 'utf8'); | |
| pluginJson = JSON.parse(pluginJsonContent); | |
| pluginJsonFile = pluginJsonPath; | |
| results.filesScanned++; | |
| } catch (err) { | |
| results.structureIssues.push({ | |
| type: 'parse_error', | |
| message: 'Failed to parse plugin.json', | |
| file: pluginJsonPath, | |
| detail: err && err.message ? err.message : String(err), | |
| certainty: 'HIGH', | |
| patternId: 'malformed_plugin_json' | |
| }); | |
| } | |
| } else if (fs.existsSync(altPluginJsonPath)) { | |
| try { | |
| const pluginJsonContent = fs.readFileSync(altPluginJsonPath, 'utf8'); | |
| pluginJson = JSON.parse(pluginJsonContent); | |
| pluginJsonFile = altPluginJsonPath; | |
| results.filesScanned++; | |
| } catch (err) { | |
| results.structureIssues.push({ | |
| type: 'parse_error', | |
| message: 'Failed to parse plugin.json', | |
| file: altPluginJsonPath, | |
| detail: err && err.message ? err.message : String(err), | |
| certainty: 'HIGH', | |
| patternId: 'malformed_plugin_json' | |
| }); | |
| } | |
| } | |
| // Check package.json for version comparison | |
| const packageJsonPath = path.join(pluginPath, '..', '..', 'package.json'); | |
| let packageJson = null; | |
| if (fs.existsSync(packageJsonPath)) { | |
| try { | |
| const packageJsonContent = fs.readFileSync(packageJsonPath, 'utf8'); | |
| packageJson = JSON.parse(packageJsonContent); | |
| } catch (err) { | |
| results.structureIssues.push({ | |
| type: 'parse_error', | |
| message: 'Failed to parse package.json for version comparison', | |
| file: packageJsonPath, | |
| detail: err && err.message ? err.message : String(err), | |
| certainty: 'MEDIUM', | |
| patternId: 'malformed_package_json' | |
| }); | |
| } |
| if (!schema || !schema.properties) return null; | ||
|
|
||
| const propCount = Object.keys(schema.properties).length; | ||
| const requiredCount = schema.required?.length || 0; |
There was a problem hiding this comment.
Unused variable requiredCount.
| const requiredCount = schema.required?.length || 0; |
| * Plugin Analyzer Tests | ||
| */ | ||
|
|
||
| const { describe, it, beforeEach, afterEach } = require('node:test'); |
There was a problem hiding this comment.
Unused variable beforeEach.
| const { describe, it, beforeEach, afterEach } = require('node:test'); | |
| const { describe, it } = require('node:test'); |
| * Plugin Analyzer Tests | ||
| */ | ||
|
|
||
| const { describe, it, beforeEach, afterEach } = require('node:test'); |
There was a problem hiding this comment.
Unused variable afterEach.
| const { describe, it, beforeEach, afterEach } = require('node:test'); | |
| const { describe, it } = require('node:test'); |
|
|
||
| const { describe, it, beforeEach, afterEach } = require('node:test'); | ||
| const assert = require('node:assert'); | ||
| const fs = require('fs'); |
There was a problem hiding this comment.
Unused variable fs.
| const fs = require('fs'); |
| const { describe, it, beforeEach, afterEach } = require('node:test'); | ||
| const assert = require('node:assert'); | ||
| const fs = require('fs'); | ||
| const path = require('path'); |
There was a problem hiding this comment.
Unused variable path.
| const path = require('path'); |
The test file was using Node.js native test runner (node:test) but Jest picks up .test.js files. Convert to Jest assertions so the tests run properly in CI.
Code Review - CLAUDE.md Compliance IssuesI've reviewed this PR and found 4 CLAUDE.md compliance violations related to Critical Rule #5 (Read checklists BEFORE multi-file changes). The PR adds both a new lib module and a new command but didn't follow the required checklist steps. I'll post inline comments for each issue. |
Issue 1: Missing lib/ sync to pluginsFile: The new Required steps not completed:
Missing from PR:
To fix: Run Reference: lib/enhance/index.js#L9-L14 |
Issue 2: Missing CLI installer update for cross-platform supportFile: The new Required step not completed:
Impact:
To fix: Add these entries to Around line 225 in ['enhance.md', 'enhance', 'enhance.md'],Around line 336 in ['enhance', 'enhance', 'enhance.md', 'Analyze plugin structures, MCP tools, and security patterns'],Reference: bin/cli.js#L217-L224 |
Issue 3: Missing marketplace.json updateFile: The new Required step not completed:
Current state: The marketplace.json lists 5 plugins (next-task, ship, deslop-around, project-review, reality-check) but does NOT include the new To fix: Add the enhance plugin entry to {
"name": "enhance",
"source": "./plugins/enhance",
"description": "Plugin structure and tool use analyzer: validates plugin.json, MCP tool definitions, security patterns with 3 certainty levels (HIGH/MEDIUM/LOW) and auto-fix capability",
"version": "2.7.1",
"category": "development"
}Reference: .claude-plugin/marketplace.json#L11-L46 |
Issue 4: Missing README.md updateFile: The new Required step not completed:
Current state: The README.md lists existing commands like To fix: Add a new section to the Available Commands area in README.md (after line 75): ### `/enhance:plugin` - Plugin Structure and Tool Use Analyzer
Analyze and validate plugin structures, MCP tool definitions, and security patterns.
```bash
/enhance:plugin # Analyze current plugin
/enhance:plugin --fix # Auto-fix HIGH certainty issuesFeatures:
|
- Use array index instead of command name in schemaPath for auto-fix to work - Add try-catch for JSON.parse to handle malformed plugin.json gracefully - Check if path is a file before unlinking in cleanupBackups - Use fake test secret to avoid GitGuardian false positive
- Fix documentation: HIGH certainty section title (not all are auto-fixable) - Fix example: use fixed.applied.length instead of fixed.length - Remove misleading inline autoFixFn from missing_required_fields pattern (plugin-analyzer provides the proper fixer.fixRequiredFields)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| // Check package.json for version comparison | ||
| const packageJsonPath = path.join(pluginPath, '..', '..', 'package.json'); |
There was a problem hiding this comment.
The package.json path calculation assumes a specific directory structure where the plugin is located two levels below the package.json (e.g., plugins/enhance). This may not work correctly if the analyzer is run from a different working directory or if plugins are stored in a non-standard location. Consider making the package.json path configurable or using a more robust path resolution strategy.
Address PR #130 feedback: Replace hardcoded relative path assumption with findNearestPackageJson() that walks up the directory tree. This fixes the issue where plugin analyzer would fail if run from non-standard directory structure.
#120) (#131) * feat(enhance): add agent prompt analyzer library Create core library modules for analyzing agent prompt files: - agent-patterns.js: 14 patterns across 6 categories (structure, tool, xml, cot, example, anti-pattern) - agent-analyzer.js: orchestrator with frontmatter parsing - fixer.js: markdown-specific auto-fixes (frontmatter, unrestricted bash, missing role) - index.js: export agent analyzer functions - reporter.js: agent report generation functions Part of implementation plan step 1-4 * feat(enhance): add agent-enhancer agent and update enhance command Create agent-enhancer.md with: - Full agent specification using opus model - 14 pattern checks across 6 categories - Auto-fix details for HIGH certainty issues - Example usage and workflow Update enhance.md command to add /enhance:agent: - Arguments and workflow documentation - Detection categories table - Implementation code snippet - Example usage Part of implementation plan step 5-6 * test(enhance): add comprehensive agent analyzer tests Create test suite with: - Pattern tests for all 14 patterns - Frontmatter parsing tests - Agent analysis integration tests - Fixer function tests for markdown - Helper function tests Part of implementation plan step 7 * fix(enhance): address code review issues for agent analyzer - Clean up redundant agentPatterns.agentPatterns access pattern in agent-analyzer.js - Fix auto-fix filtering to use patternId matching instead of missing autoFixFn - Replace fs.readdirSync recursive option with compatible withFileTypes approach - Add null safety checks for frontmatter name/description trim() calls - Fix test cases for CoT pattern edge cases (word count and reasoning keyword) * fix(enhance): use robust package.json path resolution Address PR #130 feedback: Replace hardcoded relative path assumption with findNearestPackageJson() that walks up the directory tree. This fixes the issue where plugin analyzer would fail if run from non-standard directory structure. * docs: update documentation for /enhance:agent command - Added CHANGELOG entry for new agent prompt optimizer - Added model selection guidelines to CLAUDE.md - Documents 14 detection patterns across 6 categories - Explains opus model choice for quality multiplier effect * fix(enhance): preserve plugin auto-fix by honoring autoFixFn Address PR #131 Codex feedback: The fixable issues filter was hardcoding markdown pattern IDs only, breaking plugin.json auto-fixes that rely on autoFixFn (version mismatch, schema fixes). Now includes issues with autoFixFn OR known markdown pattern IDs. * fix: address Copilot review comments - Remove unused 'body' variable in agent-analyzer.js - Remove unused 'body' variable in test file - Remove unused 'frontmatterEnd' variable in fixer.js - Fix test count in CHANGELOG (21 → 45) - Fix auto-fix counts (4 → 3 patterns) - Set missing_name and missing_description autoFix to false (no implementation, requires context-dependent values) - Remove duplicate entry from 2.7.0 section in CHANGELOG - Fix category breakdown table in agent-enhancer.md - Add agent convenience exports to index.js
Summary
/enhance:plugincommand for analyzing plugin structures (feat: /enhance:plugin - Plugin structure and tool use analyzer #119)Changes
plugins/enhance/plugin with command and agentlib/enhance/module with pattern matching, reporter, fixerTest Plan
node --test tests/enhance/plugin-analyzer.test.js- 21 tests pass/enhance:plugincommand on existing pluginsRelated Issues
Closes #119