feat: perf workflow + enhance hooks/skills - #151
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
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 significantly expands the toolset by introducing a robust, multi-phase performance investigation workflow. It also refines the existing code quality analysis capabilities by adding specialized analyzers for hooks and skills, ensuring better adherence to best practices for these components. These additions are supported by comprehensive documentation and testing, ensuring reliability and ease of use across various platforms. 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 /perf plugin for rigorous performance investigations, adding it to the marketplace, CLI installation scripts, and documentation across various files like README.md, CHANGELOG.md, and docs/reference/AGENTS.md. The /perf plugin includes extensive new modules for argument parsing, baseline management, benchmarking, breaking point detection, checkpointing, code path analysis, consolidation, constraint running, experiment execution, investigation state management, and language-specific profilers (Go, Java, Node, Python, Rust), along with dedicated agents and skills. Concurrently, the existing /enhance plugin is expanded to include new hooks and skills analyzers, which are integrated into its orchestrator, MCP tool definitions, and documentation. The review comments highlight critical path traversal vulnerabilities in the getInvestigationLogPath and getBaselinePath functions within the lib/perf/investigation-state.js and lib/perf/baseline-store.js modules, respectively, where user-controlled input is directly used to construct file paths without proper validation, potentially allowing malicious users to write files outside intended directories.
| function getInvestigationLogPath(id, basePath = process.cwd()) { | ||
| if (!id) { | ||
| throw new Error('Investigation id is required'); | ||
| } | ||
| const { logDir } = ensurePerfDirs(basePath); | ||
| return path.join(logDir, `${id}.md`); | ||
| } |
There was a problem hiding this comment.
The id parameter, which can be user-controlled via the --id CLI flag, is used directly in path.join to construct a file path. This creates a path traversal vulnerability. A malicious user could provide an id like ../../../../etc/passwd to write the log file outside the intended directory. The final path should be validated to ensure it's within the logDir. This aligns with the repository rule to always validate input paths to prevent path traversal vulnerabilities.
function getInvestigationLogPath(id, basePath = process.cwd()) {
if (!id) {
throw new Error('Investigation id is required');
}
const { logDir } = ensurePerfDirs(basePath);
const logPath = path.join(logDir, `${id}.md`);
validatePathWithinBase(logPath, logDir);
return logPath;
}References
- Always validate input paths to prevent path traversal vulnerabilities. Ensure all file operations are constrained within the intended project directory.
| function getBaselinePath(version, basePath = process.cwd()) { | ||
| if (!version) { | ||
| throw new Error('Baseline version is required'); | ||
| } | ||
| return path.join(ensureBaselineDir(basePath), `${version}.json`); | ||
| } |
There was a problem hiding this comment.
The version parameter, which can be user-controlled via the --version CLI flag, is used directly in path.join to construct a file path. This creates a path traversal vulnerability, allowing a user to write a baseline file outside the intended directory (e.g., with a version like ../../foo). The final path should be validated to ensure it is within the baselineDir. This aligns with the repository rule to always validate input paths to prevent path traversal vulnerabilities.
References
- Always validate input paths to prevent path traversal vulnerabilities. Ensure all file operations are constrained within the intended project directory.
39dfa77 to
74a12d3
Compare
Code reviewI found one issue that needs to be addressed: Missing lib/perf export from lib/index.jsFile: lib/perf/index.js (lines 24-41) The new Issue: The new-lib-module.md checklist (required by CLAUDE.md Rule #5) requires all new lib modules to be exported from Required change: Add to const perf = require('./perf');And include Summary: Otherwise the code looks good. No bugs found in the new performance workflow modules or enhance hooks/skills analyzers. Good test coverage and documentation. |
74a12d3 to
8dd7f37
Compare
Code ReviewI've reviewed this PR and found 3 CLAUDE.md compliance issues. I'll post inline comments for each with specific fix suggestions. |
Issue 1: Model Selection Violation for Enhancer AgentsFiles:
Problem: CLAUDE.md Rule:
Fix: model: sonnetTo: model: opusReference: See CLAUDE.md Model Selection Guidelines |
Issue 2: Missing lib/perf Export from lib/index.jsFile: Problem: CLAUDE.md Rule:
Fix:
const perf = require('./perf');
module.exports = {
platform,
patterns,
state,
utils,
config,
sources,
xplat,
enhance,
repoMap,
perf, // Add this
// ...
};Reference: See new-lib-module.md checklist |
Issue 3: Missing CHANGELOG.md Entry for Performance WorkflowFile: Problem: CLAUDE.md Rule:
What's Missing:
Fix: - **/perf Command** - New performance investigation workflow with multi-phase testing, profiling (Node.js, Python, Go, Rust, Java), baseline comparison, breaking point detection, and optimization runner
- **lib/perf Module** - Core performance investigation infrastructure including state management, checkpoint/rollback, experiment runner, and code path trackingReference: See CLAUDE.md Critical Rule #3 |
Code ReviewI've completed my review of this PR and found several issues that need to be addressed: 1. Missing CHANGELOG Entry for /perf WorkflowFile: The This PR adds a major new feature (10-phase performance investigation framework with multi-language profiling support), which should be documented alongside the enhance hooks/skills entries. Suggested addition: - **/perf Plugin** - New performance investigation workflow with 10-phase structured approach, multi-language profilers (Node, Python, Go, Rust, Java), baseline establishment, breaking-point discovery, hypothesis generation, code-path analysis, optimization experiments, and decision gates2. Missing lib/perf ExportFile: The new This prevents external code from importing the perf module using Required changes:
3. Command Injection Risk in Profiler CommandsFiles: Security: The profiler commands use string interpolation with user-supplied options that are executed via While current usage in Example attack: Recommendation: Use 4. Unused experimentRun1 VariableFile: Logic issue: The first benchmark run ( This creates an unfair comparison:
If warm-up is intentional, the baseline should also have a warm-up run. If not, remove the first experiment run. The comment on line 42 doesn't explain why there are two runs or why the first is discarded. Summary: 4 issues found
All issues have clear fixes outlined above. |
Code Review - 4 Issues FoundI found 4 issues that need to be addressed: 1. Missing /perf Workflow in CHANGELOG.mdFile: CHANGELOG.md (line 10) The CHANGELOG.md was updated to document the enhance hooks/skills analyzers but completely omits the new /perf workflow, which is a major feature addition. CLAUDE.md Rule Violated: From checklists/new-command.md Step 5: "Update CHANGELOG.md to note the addition" Reference: The PR adds an entire new plugin with:
Action: Please add a section to the Unreleased changelog documenting this major feature. 2. Missing /perf Plugin in ARCHITECTURE.mdFile: docs/ARCHITECTURE.md (lines 57-228) The /perf plugin and command are not documented in ARCHITECTURE.md plugin directory structure or commands lists across all three platforms (Claude Code, OpenCode, Codex). CLAUDE.md Rule Violated: From checklists/new-command.md Step 5: "Update docs/ARCHITECTURE.md to add to commands list if significant" Reference: Missing sections:
The perf plugin was correctly added to bin/cli.js for all three platforms, but ARCHITECTURE.md was not updated accordingly. Action: Please add /perf to all command lists and the compatibility table in ARCHITECTURE.md. 3. Path Traversal Vulnerability in getInvestigationLogPathFile: lib/perf/investigation-state.js (line 132) The id parameter is directly concatenated into the file path without sanitization, allowing path traversal attacks. Issue: The --id argument is user-controlled via command-line input (documented in perf.md, parsed at line 78), passed directly to initializeInvestigation, and then used unsanitized in this function. Exploit scenario: This would write to /tmp/malicious.md instead of the intended .claude/perf/investigations/ directory. Evidence:
Suggested fix: function getInvestigationLogPath(id, basePath = process.cwd()) {
if (!id) {
throw new Error('Investigation id is required');
}
// Sanitize id to prevent path traversal
if (id.includes('..') || id.includes('/') || id.includes('\\')) {
throw new Error('Investigation id contains invalid characters');
}
const { logDir } = ensurePerfDirs(basePath);
return path.join(logDir, `${id}.md`);
}4. Unused Variable - Dead CodeFile: lib/perf/optimization-runner.js (line 43) The variable experimentRun1 is assigned but never used. The code runs the benchmark twice but only uses the second result. Issue: If this is intentional warmup behavior, it should be documented. Otherwise, this wastes significant time (minimum 60 seconds based on DEFAULT_MIN_DURATION). Current code: const experimentRun1 = runBenchmark(command, { duration: DEFAULT_MIN_DURATION, env });
const experimentRun2 = runBenchmark(command, { duration: DEFAULT_MIN_DURATION, env });
const experimentMetrics = parseMetrics(experimentRun2.output); // Only uses experimentRun2Recommended fix: If this is intentional warmup, document it clearly: // Run once to warm up the system (cache, JIT, etc.)
runBenchmark(command, { duration: DEFAULT_MIN_DURATION, env });
// Run again for actual measurement
const experimentRun = runBenchmark(command, { duration: DEFAULT_MIN_DURATION, env });
const experimentMetrics = parseMetrics(experimentRun.output);Or if both runs should be used for statistical validity, implement proper averaging or comparison. |
2 participants
Summary
Testing