mirror of
https://github.com/catlog22/Claude-Code-Workflow.git
synced 2026-02-13 02:41:50 +08:00
Add multi-dimensional code review rules for architecture, correctness, performance, readability, security, and testing
- Introduced architecture rules to detect circular dependencies, god classes, layer violations, and mixed concerns. - Added correctness rules focusing on null checks, empty catch blocks, unreachable code, and type coercion. - Implemented performance rules addressing nested loops, synchronous I/O, memory leaks, and unnecessary re-renders in React. - Created readability rules to improve function length, variable naming, deep nesting, magic numbers, and commented code. - Established security rules to identify XSS risks, hardcoded secrets, SQL injection vulnerabilities, and insecure random generation. - Developed testing rules to enhance test quality, coverage, and maintainability, including missing assertions and error path testing. - Documented the structure and schema for rule files in the index.md for better understanding and usage.
This commit is contained in:
catlog22
@@ -0,0 +1,63 @@
|
||||
{
|
||||
"dimension": "architecture",
|
||||
"prefix": "ARCH",
|
||||
"description": "Rules for detecting architecture issues including coupling, layering, and design patterns",
|
||||
"rules": [
|
||||
{
|
||||
"id": "circular-dependency",
|
||||
"category": "dependency",
|
||||
"severity": "high",
|
||||
"pattern": "import\\s+.*from\\s+['\"]\\.\\..*['\"]",
|
||||
"patternType": "regex",
|
||||
"contextPattern": "export.*import.*from.*same-module",
|
||||
"description": "Potential circular dependency detected. Circular imports cause initialization issues and tight coupling",
|
||||
"recommendation": "Extract shared code to a separate module, use dependency injection, or restructure the dependency graph",
|
||||
"fixExample": "// Before - A imports B, B imports A\n// moduleA.ts\nimport { funcB } from './moduleB';\nexport const funcA = () => funcB();\n\n// moduleB.ts\nimport { funcA } from './moduleA'; // circular!\n\n// After - extract shared logic\n// shared.ts\nexport const sharedLogic = () => { ... };\n\n// moduleA.ts\nimport { sharedLogic } from './shared';"
|
||||
},
|
||||
{
|
||||
"id": "god-class",
|
||||
"category": "single-responsibility",
|
||||
"severity": "high",
|
||||
"pattern": "class\\s+\\w+\\s*\\{",
|
||||
"patternType": "regex",
|
||||
"methodThreshold": 15,
|
||||
"lineThreshold": 300,
|
||||
"description": "Class with too many methods or lines violates single responsibility principle",
|
||||
"recommendation": "Split into smaller, focused classes. Each class should have one reason to change",
|
||||
"fixExample": "// Before - UserManager handles everything\nclass UserManager {\n createUser() { ... }\n updateUser() { ... }\n sendEmail() { ... }\n generateReport() { ... }\n validatePassword() { ... }\n}\n\n// After - separated concerns\nclass UserRepository { create, update, delete }\nclass EmailService { sendEmail }\nclass ReportGenerator { generate }\nclass PasswordValidator { validate }"
|
||||
},
|
||||
{
|
||||
"id": "layer-violation",
|
||||
"category": "layering",
|
||||
"severity": "high",
|
||||
"pattern": "import.*(?:repository|database|sql|prisma|mongoose).*from",
|
||||
"patternType": "regex",
|
||||
"contextPath": ["controller", "handler", "route", "component"],
|
||||
"description": "Direct database access from presentation layer violates layered architecture",
|
||||
"recommendation": "Access data through service/use-case layer. Keep controllers thin and delegate to services",
|
||||
"fixExample": "// Before - controller accesses DB directly\nimport { prisma } from './database';\nconst getUsers = async () => prisma.user.findMany();\n\n// After - use service layer\nimport { userService } from './services';\nconst getUsers = async () => userService.getAll();"
|
||||
},
|
||||
{
|
||||
"id": "missing-interface",
|
||||
"category": "abstraction",
|
||||
"severity": "medium",
|
||||
"pattern": "new\\s+\\w+Service\\(|new\\s+\\w+Repository\\(",
|
||||
"patternType": "regex",
|
||||
"negativePatterns": ["interface", "implements", "inject"],
|
||||
"description": "Direct instantiation of services/repositories creates tight coupling",
|
||||
"recommendation": "Define interfaces and use dependency injection for loose coupling and testability",
|
||||
"fixExample": "// Before - tight coupling\nclass OrderService {\n private repo = new OrderRepository();\n}\n\n// After - loose coupling\ninterface IOrderRepository {\n findById(id: string): Promise<Order>;\n}\n\nclass OrderService {\n constructor(private repo: IOrderRepository) {}\n}"
|
||||
},
|
||||
{
|
||||
"id": "mixed-concerns",
|
||||
"category": "separation-of-concerns",
|
||||
"severity": "medium",
|
||||
"pattern": "fetch\\s*\\(|axios\\.|http\\.",
|
||||
"patternType": "regex",
|
||||
"contextPath": ["component", "view", "page"],
|
||||
"description": "Network calls in UI components mix data fetching with presentation",
|
||||
"recommendation": "Extract data fetching to hooks, services, or state management layer",
|
||||
"fixExample": "// Before - fetch in component\nfunction UserList() {\n const [users, setUsers] = useState([]);\n useEffect(() => {\n fetch('/api/users').then(r => r.json()).then(setUsers);\n }, []);\n}\n\n// After - custom hook\nfunction useUsers() {\n return useQuery('users', () => userService.getAll());\n}\n\nfunction UserList() {\n const { data: users } = useUsers();\n}"
|
||||
}
|
||||
]
|
||||
}
|
||||
@@ -0,0 +1,60 @@
|
||||
{
|
||||
"dimension": "correctness",
|
||||
"prefix": "CORR",
|
||||
"description": "Rules for detecting logical errors, null handling, and error handling issues",
|
||||
"rules": [
|
||||
{
|
||||
"id": "null-check",
|
||||
"category": "null-check",
|
||||
"severity": "high",
|
||||
"pattern": "\\w+\\.\\w+(?!\\.?\\?)",
|
||||
"patternType": "regex",
|
||||
"negativePatterns": ["\\?\\.", "if\\s*\\(", "!==?\\s*null", "!==?\\s*undefined", "&&\\s*\\w+\\."],
|
||||
"description": "Property access without null/undefined check may cause runtime errors",
|
||||
"recommendation": "Add null/undefined check before accessing properties using optional chaining or conditional checks",
|
||||
"fixExample": "// Before\nobj.property.value\n\n// After\nobj?.property?.value\n// or\nif (obj && obj.property) { obj.property.value }"
|
||||
},
|
||||
{
|
||||
"id": "empty-catch",
|
||||
"category": "empty-catch",
|
||||
"severity": "high",
|
||||
"pattern": "catch\\s*\\([^)]*\\)\\s*\\{\\s*\\}",
|
||||
"patternType": "regex",
|
||||
"description": "Empty catch block silently swallows errors, hiding bugs and making debugging difficult",
|
||||
"recommendation": "Log the error, rethrow it, or handle it appropriately. Never silently ignore exceptions",
|
||||
"fixExample": "// Before\ncatch (e) { }\n\n// After\ncatch (e) {\n console.error('Operation failed:', e);\n throw e; // or handle appropriately\n}"
|
||||
},
|
||||
{
|
||||
"id": "unreachable-code",
|
||||
"category": "unreachable-code",
|
||||
"severity": "medium",
|
||||
"pattern": "return\\s+[^;]+;\\s*\\n\\s*[^}\\s]",
|
||||
"patternType": "regex",
|
||||
"description": "Code after return statement is unreachable and will never execute",
|
||||
"recommendation": "Remove unreachable code or restructure the logic to ensure all code paths are accessible",
|
||||
"fixExample": "// Before\nfunction example() {\n return value;\n doSomething(); // unreachable\n}\n\n// After\nfunction example() {\n doSomething();\n return value;\n}"
|
||||
},
|
||||
{
|
||||
"id": "array-index-unchecked",
|
||||
"category": "boundary-check",
|
||||
"severity": "high",
|
||||
"pattern": "\\[\\d+\\]|\\[\\w+\\](?!\\s*[!=<>])",
|
||||
"patternType": "regex",
|
||||
"negativePatterns": ["\\.length", "Array\\.isArray", "\\?.\\["],
|
||||
"description": "Array index access without boundary check may cause undefined access or out-of-bounds errors",
|
||||
"recommendation": "Check array length or use optional chaining before accessing array elements",
|
||||
"fixExample": "// Before\nconst item = arr[index];\n\n// After\nconst item = arr?.[index];\n// or\nconst item = index < arr.length ? arr[index] : defaultValue;"
|
||||
},
|
||||
{
|
||||
"id": "comparison-type-coercion",
|
||||
"category": "type-safety",
|
||||
"severity": "medium",
|
||||
"pattern": "[^!=]==[^=]|[^!]==[^=]",
|
||||
"patternType": "regex",
|
||||
"negativePatterns": ["===", "!=="],
|
||||
"description": "Using == instead of === can lead to unexpected type coercion",
|
||||
"recommendation": "Use strict equality (===) to avoid implicit type conversion",
|
||||
"fixExample": "// Before\nif (value == null)\nif (a == b)\n\n// After\nif (value === null || value === undefined)\nif (a === b)"
|
||||
}
|
||||
]
|
||||
}
|
||||
140
.claude/skills/review-code/specs/rules/index.md
Normal file
140
.claude/skills/review-code/specs/rules/index.md
Normal file
@@ -0,0 +1,140 @@
|
||||
# Code Review Rules Index
|
||||
|
||||
This directory contains externalized review rules for the multi-dimensional code review skill.
|
||||
|
||||
## Directory Structure
|
||||
|
||||
```
|
||||
rules/
|
||||
├── index.md # This file
|
||||
├── correctness-rules.json # CORR - Logic and error handling
|
||||
├── security-rules.json # SEC - Security vulnerabilities
|
||||
├── performance-rules.json # PERF - Performance issues
|
||||
├── readability-rules.json # READ - Code clarity
|
||||
├── testing-rules.json # TEST - Test quality
|
||||
└── architecture-rules.json # ARCH - Design patterns
|
||||
```
|
||||
|
||||
## Rule File Schema
|
||||
|
||||
Each rule file follows this JSON schema:
|
||||
|
||||
```json
|
||||
{
|
||||
"dimension": "string", // Dimension identifier
|
||||
"prefix": "string", // Finding ID prefix (4 chars)
|
||||
"description": "string", // Dimension description
|
||||
"rules": [
|
||||
{
|
||||
"id": "string", // Unique rule identifier
|
||||
"category": "string", // Rule category within dimension
|
||||
"severity": "critical|high|medium|low",
|
||||
"pattern": "string", // Detection pattern
|
||||
"patternType": "regex|includes|ast",
|
||||
"negativePatterns": [], // Patterns that exclude matches
|
||||
"caseInsensitive": false, // For regex patterns
|
||||
"contextPattern": "", // Additional context requirement
|
||||
"contextPath": [], // Path patterns for context
|
||||
"lineThreshold": 0, // For size-based rules
|
||||
"methodThreshold": 0, // For complexity rules
|
||||
"description": "string", // Issue description
|
||||
"recommendation": "string", // How to fix
|
||||
"fixExample": "string" // Code example
|
||||
}
|
||||
]
|
||||
}
|
||||
```
|
||||
|
||||
## Dimension Summary
|
||||
|
||||
| Dimension | Prefix | Rules | Focus Areas |
|
||||
|-----------|--------|-------|-------------|
|
||||
| Correctness | CORR | 5 | Null checks, error handling, type safety |
|
||||
| Security | SEC | 5 | XSS, injection, secrets, crypto |
|
||||
| Performance | PERF | 5 | Complexity, I/O, memory leaks |
|
||||
| Readability | READ | 5 | Naming, length, nesting, magic values |
|
||||
| Testing | TEST | 5 | Assertions, coverage, mock quality |
|
||||
| Architecture | ARCH | 5 | Dependencies, layering, coupling |
|
||||
|
||||
## Severity Levels
|
||||
|
||||
| Severity | Description | Action |
|
||||
|----------|-------------|--------|
|
||||
| **critical** | Security vulnerability or data loss risk | Must fix before release |
|
||||
| **high** | Bug or significant quality issue | Fix in current sprint |
|
||||
| **medium** | Code smell or maintainability concern | Plan to address |
|
||||
| **low** | Style or minor improvement | Address when convenient |
|
||||
|
||||
## Pattern Types
|
||||
|
||||
### regex
|
||||
Standard regular expression pattern. Supports flags via `caseInsensitive`.
|
||||
|
||||
```json
|
||||
{
|
||||
"pattern": "catch\\s*\\([^)]*\\)\\s*\\{\\s*\\}",
|
||||
"patternType": "regex"
|
||||
}
|
||||
```
|
||||
|
||||
### includes
|
||||
Simple substring match. Faster than regex for literal strings.
|
||||
|
||||
```json
|
||||
{
|
||||
"pattern": "innerHTML",
|
||||
"patternType": "includes"
|
||||
}
|
||||
```
|
||||
|
||||
### ast (Future)
|
||||
AST-based detection for complex structural patterns.
|
||||
|
||||
```json
|
||||
{
|
||||
"pattern": "function[params>5]",
|
||||
"patternType": "ast"
|
||||
}
|
||||
```
|
||||
|
||||
## Usage in Code
|
||||
|
||||
```javascript
|
||||
// Load rules
|
||||
const rules = JSON.parse(fs.readFileSync('correctness-rules.json'));
|
||||
|
||||
// Apply rules
|
||||
for (const rule of rules.rules) {
|
||||
const matches = detectByPattern(content, rule.pattern, rule.patternType);
|
||||
for (const match of matches) {
|
||||
// Check negative patterns
|
||||
if (rule.negativePatterns?.some(np => match.context.includes(np))) {
|
||||
continue;
|
||||
}
|
||||
findings.push({
|
||||
id: `${rules.prefix}-${counter++}`,
|
||||
severity: rule.severity,
|
||||
category: rule.category,
|
||||
description: rule.description,
|
||||
recommendation: rule.recommendation,
|
||||
fixExample: rule.fixExample
|
||||
});
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
## Adding New Rules
|
||||
|
||||
1. Identify the appropriate dimension
|
||||
2. Create rule with unique `id` within dimension
|
||||
3. Choose appropriate `patternType`
|
||||
4. Provide clear `description` and `recommendation`
|
||||
5. Include practical `fixExample`
|
||||
6. Test against sample code
|
||||
|
||||
## Rule Maintenance
|
||||
|
||||
- Review rules quarterly for relevance
|
||||
- Update patterns as language/framework evolves
|
||||
- Track false positive rates
|
||||
- Collect feedback from users
|
||||
@@ -0,0 +1,59 @@
|
||||
{
|
||||
"dimension": "performance",
|
||||
"prefix": "PERF",
|
||||
"description": "Rules for detecting performance issues including inefficient algorithms, memory leaks, and resource waste",
|
||||
"rules": [
|
||||
{
|
||||
"id": "nested-loops",
|
||||
"category": "algorithm-complexity",
|
||||
"severity": "medium",
|
||||
"pattern": "for\\s*\\([^)]+\\)\\s*\\{[^}]*for\\s*\\([^)]+\\)|forEach\\s*\\([^)]+\\)\\s*\\{[^}]*forEach",
|
||||
"patternType": "regex",
|
||||
"description": "Nested loops may indicate O(n^2) or worse complexity. Consider if this can be optimized",
|
||||
"recommendation": "Use Map/Set for O(1) lookups, break early when possible, or restructure the algorithm",
|
||||
"fixExample": "// Before - O(n^2)\nfor (const a of listA) {\n for (const b of listB) {\n if (a.id === b.id) { ... }\n }\n}\n\n// After - O(n)\nconst bMap = new Map(listB.map(b => [b.id, b]));\nfor (const a of listA) {\n const b = bMap.get(a.id);\n if (b) { ... }\n}"
|
||||
},
|
||||
{
|
||||
"id": "array-in-loop",
|
||||
"category": "inefficient-operation",
|
||||
"severity": "high",
|
||||
"pattern": "\\.includes\\s*\\(|indexOf\\s*\\(|find\\s*\\(",
|
||||
"patternType": "includes",
|
||||
"contextPattern": "for|while|forEach|map|filter|reduce",
|
||||
"description": "Array search methods inside loops cause O(n*m) complexity. Consider using Set or Map",
|
||||
"recommendation": "Convert array to Set before the loop for O(1) lookups",
|
||||
"fixExample": "// Before - O(n*m)\nfor (const item of items) {\n if (existingIds.includes(item.id)) { ... }\n}\n\n// After - O(n)\nconst idSet = new Set(existingIds);\nfor (const item of items) {\n if (idSet.has(item.id)) { ... }\n}"
|
||||
},
|
||||
{
|
||||
"id": "synchronous-io",
|
||||
"category": "io-efficiency",
|
||||
"severity": "high",
|
||||
"pattern": "readFileSync|writeFileSync|execSync|spawnSync",
|
||||
"patternType": "includes",
|
||||
"description": "Synchronous I/O blocks the event loop and degrades application responsiveness",
|
||||
"recommendation": "Use async versions (readFile, writeFile) or Promise-based APIs",
|
||||
"fixExample": "// Before\nconst data = fs.readFileSync(path);\n\n// After\nconst data = await fs.promises.readFile(path);\n// or\nfs.readFile(path, (err, data) => { ... });"
|
||||
},
|
||||
{
|
||||
"id": "memory-leak-closure",
|
||||
"category": "memory-leak",
|
||||
"severity": "high",
|
||||
"pattern": "addEventListener\\s*\\(|setInterval\\s*\\(|setTimeout\\s*\\(",
|
||||
"patternType": "regex",
|
||||
"negativePatterns": ["removeEventListener", "clearInterval", "clearTimeout"],
|
||||
"description": "Event listeners and timers without cleanup can cause memory leaks",
|
||||
"recommendation": "Always remove event listeners and clear timers in cleanup functions (componentWillUnmount, useEffect cleanup)",
|
||||
"fixExample": "// Before\nuseEffect(() => {\n window.addEventListener('resize', handler);\n}, []);\n\n// After\nuseEffect(() => {\n window.addEventListener('resize', handler);\n return () => window.removeEventListener('resize', handler);\n}, []);"
|
||||
},
|
||||
{
|
||||
"id": "unnecessary-rerender",
|
||||
"category": "react-performance",
|
||||
"severity": "medium",
|
||||
"pattern": "useState\\s*\\(\\s*\\{|useState\\s*\\(\\s*\\[",
|
||||
"patternType": "regex",
|
||||
"description": "Creating new object/array references in useState can cause unnecessary re-renders",
|
||||
"recommendation": "Use useMemo for computed values, useCallback for functions, or consider state management libraries",
|
||||
"fixExample": "// Before - new object every render\nconst [config] = useState({ theme: 'dark' });\n\n// After - stable reference\nconst defaultConfig = useMemo(() => ({ theme: 'dark' }), []);\nconst [config] = useState(defaultConfig);"
|
||||
}
|
||||
]
|
||||
}
|
||||
@@ -0,0 +1,60 @@
|
||||
{
|
||||
"dimension": "readability",
|
||||
"prefix": "READ",
|
||||
"description": "Rules for detecting code readability issues including naming, complexity, and documentation",
|
||||
"rules": [
|
||||
{
|
||||
"id": "long-function",
|
||||
"category": "function-length",
|
||||
"severity": "medium",
|
||||
"pattern": "function\\s+\\w+\\s*\\([^)]*\\)\\s*\\{|=>\\s*\\{",
|
||||
"patternType": "regex",
|
||||
"lineThreshold": 50,
|
||||
"description": "Functions longer than 50 lines are difficult to understand and maintain",
|
||||
"recommendation": "Break down into smaller, focused functions. Each function should do one thing well",
|
||||
"fixExample": "// Before - 100 line function\nfunction processData(data) {\n // validation\n // transformation\n // calculation\n // formatting\n // output\n}\n\n// After - composed functions\nfunction processData(data) {\n const validated = validateData(data);\n const transformed = transformData(validated);\n return formatOutput(calculateResults(transformed));\n}"
|
||||
},
|
||||
{
|
||||
"id": "single-letter-variable",
|
||||
"category": "naming",
|
||||
"severity": "low",
|
||||
"pattern": "(?:const|let|var)\\s+[a-z]\\s*=",
|
||||
"patternType": "regex",
|
||||
"negativePatterns": ["for\\s*\\(", "\\[\\w,\\s*\\w\\]", "catch\\s*\\(e\\)"],
|
||||
"description": "Single letter variable names reduce code readability except in specific contexts (loop counters, catch)",
|
||||
"recommendation": "Use descriptive names that convey the variable's purpose",
|
||||
"fixExample": "// Before\nconst d = getData();\nconst r = d.map(x => x.value);\n\n// After\nconst userData = getData();\nconst userValues = userData.map(user => user.value);"
|
||||
},
|
||||
{
|
||||
"id": "deep-nesting",
|
||||
"category": "complexity",
|
||||
"severity": "high",
|
||||
"pattern": "\\{[^}]*\\{[^}]*\\{[^}]*\\{",
|
||||
"patternType": "regex",
|
||||
"description": "Deeply nested code (4+ levels) is hard to follow and maintain",
|
||||
"recommendation": "Use early returns, extract functions, or flatten conditionals",
|
||||
"fixExample": "// Before\nif (user) {\n if (user.permissions) {\n if (user.permissions.canEdit) {\n if (document.isEditable) {\n // do work\n }\n }\n }\n}\n\n// After\nif (!user?.permissions?.canEdit) return;\nif (!document.isEditable) return;\n// do work"
|
||||
},
|
||||
{
|
||||
"id": "magic-number",
|
||||
"category": "magic-value",
|
||||
"severity": "low",
|
||||
"pattern": "[^\\d]\\d{2,}[^\\d]|setTimeout\\s*\\([^,]+,\\s*\\d{4,}\\)",
|
||||
"patternType": "regex",
|
||||
"negativePatterns": ["const", "let", "enum", "0x", "100", "1000"],
|
||||
"description": "Magic numbers without explanation make code hard to understand",
|
||||
"recommendation": "Extract magic numbers into named constants with descriptive names",
|
||||
"fixExample": "// Before\nif (status === 403) { ... }\nsetTimeout(callback, 86400000);\n\n// After\nconst HTTP_FORBIDDEN = 403;\nconst ONE_DAY_MS = 24 * 60 * 60 * 1000;\nif (status === HTTP_FORBIDDEN) { ... }\nsetTimeout(callback, ONE_DAY_MS);"
|
||||
},
|
||||
{
|
||||
"id": "commented-code",
|
||||
"category": "dead-code",
|
||||
"severity": "low",
|
||||
"pattern": "//\\s*(const|let|var|function|if|for|while|return)\\s+",
|
||||
"patternType": "regex",
|
||||
"description": "Commented-out code adds noise and should be removed. Use version control for history",
|
||||
"recommendation": "Remove commented code. If needed for reference, add a comment explaining why with a link to relevant commit/issue",
|
||||
"fixExample": "// Before\n// function oldImplementation() { ... }\n// const legacyConfig = {...};\n\n// After\n// See PR #123 for previous implementation\n// removed 2024-01-01"
|
||||
}
|
||||
]
|
||||
}
|
||||
58
.claude/skills/review-code/specs/rules/security-rules.json
Normal file
58
.claude/skills/review-code/specs/rules/security-rules.json
Normal file
@@ -0,0 +1,58 @@
|
||||
{
|
||||
"dimension": "security",
|
||||
"prefix": "SEC",
|
||||
"description": "Rules for detecting security vulnerabilities including XSS, injection, and credential exposure",
|
||||
"rules": [
|
||||
{
|
||||
"id": "xss-innerHTML",
|
||||
"category": "xss-risk",
|
||||
"severity": "critical",
|
||||
"pattern": "innerHTML\\s*=|dangerouslySetInnerHTML",
|
||||
"patternType": "includes",
|
||||
"description": "Direct HTML injection via innerHTML or dangerouslySetInnerHTML can lead to XSS vulnerabilities",
|
||||
"recommendation": "Use textContent for plain text, or sanitize HTML input using a library like DOMPurify before injection",
|
||||
"fixExample": "// Before\nelement.innerHTML = userInput;\n<div dangerouslySetInnerHTML={{__html: data}} />\n\n// After\nelement.textContent = userInput;\n// or\nimport DOMPurify from 'dompurify';\nelement.innerHTML = DOMPurify.sanitize(userInput);"
|
||||
},
|
||||
{
|
||||
"id": "hardcoded-secret",
|
||||
"category": "hardcoded-secret",
|
||||
"severity": "critical",
|
||||
"pattern": "(?:password|secret|api[_-]?key|token|credential)\\s*[=:]\\s*['\"][^'\"]{8,}['\"]",
|
||||
"patternType": "regex",
|
||||
"caseInsensitive": true,
|
||||
"description": "Hardcoded credentials detected in source code. This is a security risk if code is exposed",
|
||||
"recommendation": "Use environment variables, secret management services, or configuration files excluded from version control",
|
||||
"fixExample": "// Before\nconst apiKey = 'sk-1234567890abcdef';\n\n// After\nconst apiKey = process.env.API_KEY;\n// or\nconst apiKey = await getSecretFromVault('api-key');"
|
||||
},
|
||||
{
|
||||
"id": "sql-injection",
|
||||
"category": "injection",
|
||||
"severity": "critical",
|
||||
"pattern": "query\\s*\\(\\s*[`'\"].*\\$\\{|execute\\s*\\(\\s*[`'\"].*\\+",
|
||||
"patternType": "regex",
|
||||
"description": "String concatenation or template literals in SQL queries can lead to SQL injection",
|
||||
"recommendation": "Use parameterized queries or prepared statements with placeholders",
|
||||
"fixExample": "// Before\ndb.query(`SELECT * FROM users WHERE id = ${userId}`);\n\n// After\ndb.query('SELECT * FROM users WHERE id = ?', [userId]);\n// or\ndb.query('SELECT * FROM users WHERE id = $1', [userId]);"
|
||||
},
|
||||
{
|
||||
"id": "command-injection",
|
||||
"category": "injection",
|
||||
"severity": "critical",
|
||||
"pattern": "exec\\s*\\(|execSync\\s*\\(|spawn\\s*\\([^,]*\\+|child_process",
|
||||
"patternType": "regex",
|
||||
"description": "Command execution with user input can lead to command injection attacks",
|
||||
"recommendation": "Validate and sanitize input, use parameterized commands, or avoid shell execution entirely",
|
||||
"fixExample": "// Before\nexec(`ls ${userInput}`);\n\n// After\nexecFile('ls', [sanitizedInput], options);\n// or use spawn with {shell: false}"
|
||||
},
|
||||
{
|
||||
"id": "insecure-random",
|
||||
"category": "cryptography",
|
||||
"severity": "high",
|
||||
"pattern": "Math\\.random\\(\\)",
|
||||
"patternType": "includes",
|
||||
"description": "Math.random() is not cryptographically secure and should not be used for security-sensitive operations",
|
||||
"recommendation": "Use crypto.randomBytes() or crypto.getRandomValues() for security-critical random generation",
|
||||
"fixExample": "// Before\nconst token = Math.random().toString(36);\n\n// After\nimport crypto from 'crypto';\nconst token = crypto.randomBytes(32).toString('hex');"
|
||||
}
|
||||
]
|
||||
}
|
||||
59
.claude/skills/review-code/specs/rules/testing-rules.json
Normal file
59
.claude/skills/review-code/specs/rules/testing-rules.json
Normal file
@@ -0,0 +1,59 @@
|
||||
{
|
||||
"dimension": "testing",
|
||||
"prefix": "TEST",
|
||||
"description": "Rules for detecting testing issues including coverage gaps, test quality, and mock usage",
|
||||
"rules": [
|
||||
{
|
||||
"id": "missing-assertion",
|
||||
"category": "test-quality",
|
||||
"severity": "high",
|
||||
"pattern": "(?:it|test)\\s*\\([^)]+,\\s*(?:async\\s*)?\\(\\)\\s*=>\\s*\\{[^}]*\\}\\s*\\)",
|
||||
"patternType": "regex",
|
||||
"negativePatterns": ["expect", "assert", "should", "toBe", "toEqual"],
|
||||
"description": "Test case without assertions always passes and provides no value",
|
||||
"recommendation": "Add assertions to verify expected behavior. Each test should have at least one meaningful assertion",
|
||||
"fixExample": "// Before\nit('should process data', async () => {\n await processData(input);\n});\n\n// After\nit('should process data', async () => {\n const result = await processData(input);\n expect(result.success).toBe(true);\n expect(result.data).toHaveLength(3);\n});"
|
||||
},
|
||||
{
|
||||
"id": "hardcoded-test-data",
|
||||
"category": "test-maintainability",
|
||||
"severity": "low",
|
||||
"pattern": "expect\\s*\\([^)]+\\)\\.toBe\\s*\\(['\"][^'\"]{20,}['\"]\\)",
|
||||
"patternType": "regex",
|
||||
"description": "Long hardcoded strings in assertions are brittle and hard to maintain",
|
||||
"recommendation": "Use snapshots for large outputs, or extract expected values to test fixtures",
|
||||
"fixExample": "// Before\nexpect(result).toBe('very long expected string that is hard to maintain...');\n\n// After\nexpect(result).toMatchSnapshot();\n// or\nconst expected = loadFixture('expected-output.json');\nexpect(result).toEqual(expected);"
|
||||
},
|
||||
{
|
||||
"id": "no-error-test",
|
||||
"category": "coverage-gap",
|
||||
"severity": "medium",
|
||||
"pattern": "describe\\s*\\([^)]+",
|
||||
"patternType": "regex",
|
||||
"negativePatterns": ["throw", "reject", "error", "fail", "catch"],
|
||||
"description": "Test suite may be missing error path testing. Error handling is critical for reliability",
|
||||
"recommendation": "Add tests for error cases: invalid input, network failures, edge cases",
|
||||
"fixExample": "// Add error path tests\nit('should throw on invalid input', () => {\n expect(() => processData(null)).toThrow('Invalid input');\n});\n\nit('should handle network failure', async () => {\n mockApi.mockRejectedValue(new Error('Network error'));\n await expect(fetchData()).rejects.toThrow('Network error');\n});"
|
||||
},
|
||||
{
|
||||
"id": "test-implementation-detail",
|
||||
"category": "test-quality",
|
||||
"severity": "medium",
|
||||
"pattern": "toHaveBeenCalledWith|toHaveBeenCalledTimes",
|
||||
"patternType": "includes",
|
||||
"description": "Testing implementation details (call counts, exact parameters) makes tests brittle to refactoring",
|
||||
"recommendation": "Prefer testing observable behavior and outcomes over internal implementation",
|
||||
"fixExample": "// Before - brittle\nexpect(mockService.process).toHaveBeenCalledTimes(3);\nexpect(mockService.process).toHaveBeenCalledWith('exact-arg');\n\n// After - behavior-focused\nexpect(result.items).toHaveLength(3);\nexpect(result.processed).toBe(true);"
|
||||
},
|
||||
{
|
||||
"id": "skip-test",
|
||||
"category": "test-coverage",
|
||||
"severity": "high",
|
||||
"pattern": "it\\.skip|test\\.skip|xit|xdescribe|describe\\.skip",
|
||||
"patternType": "regex",
|
||||
"description": "Skipped tests indicate untested code paths or broken functionality",
|
||||
"recommendation": "Fix or remove skipped tests. If temporarily skipped, add TODO comment with issue reference",
|
||||
"fixExample": "// Before\nit.skip('should handle edge case', () => { ... });\n\n// After - either fix it\nit('should handle edge case', () => {\n // fixed implementation\n});\n\n// Or document why skipped\n// TODO(#123): Re-enable after API migration\nit.skip('should handle edge case', () => { ... });"
|
||||
}
|
||||
]
|
||||
}
|
||||
Reference in New Issue
Block a user