Add code-audit skill for systematic anti-pattern detection
Project-specific skill to audit codebase for: - Error handling: silent failures, swallowed exceptions, hidden errors - Security: missing auth, data exposure, injection risks - Architecture: DI violations, import boundaries, coupling Pattern definitions in YAML with verdict hints (ISSUE_IF/OK_IF) for context-aware analysis. Tailored to Mantimon TCG architecture patterns (core independence, stateless backend, repository pattern). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
Cal Corum
parent
55e02ceb21
commit
1346853869
148
.claude/skills/code-audit/SKILL.md
Normal file
148
.claude/skills/code-audit/SKILL.md
Normal file
@ -0,0 +1,148 @@
|
|||||||
|
# Code Audit Skill
|
||||||
|
|
||||||
|
Perform systematic code audits to find anti-patterns, hidden errors, security issues, and architecture violations.
|
||||||
|
|
||||||
|
## Usage
|
||||||
|
|
||||||
|
```
|
||||||
|
/code-audit [category] [path]
|
||||||
|
```
|
||||||
|
|
||||||
|
**Categories:**
|
||||||
|
- `errors` - Silent failures, swallowed exceptions, hidden errors
|
||||||
|
- `security` - Input validation, auth checks, data exposure
|
||||||
|
- `architecture` - DI violations, import boundaries, coupling
|
||||||
|
- `all` - Run all audit categories (default)
|
||||||
|
|
||||||
|
**Path:** Optional path to audit (defaults to `backend/app/`)
|
||||||
|
|
||||||
|
## Examples
|
||||||
|
|
||||||
|
```
|
||||||
|
/code-audit # Full audit of backend/app/
|
||||||
|
/code-audit errors # Just error handling patterns
|
||||||
|
/code-audit security services/ # Security audit of services/
|
||||||
|
```
|
||||||
|
|
||||||
|
## Instructions
|
||||||
|
|
||||||
|
When this skill is invoked:
|
||||||
|
|
||||||
|
1. **Load Pattern Definitions**
|
||||||
|
Read the YAML files in `patterns/` directory for the requested category.
|
||||||
|
|
||||||
|
2. **Search Codebase**
|
||||||
|
For each pattern, use Grep with the specified regex to find matches.
|
||||||
|
Focus on production code (skip test files unless pattern specifies otherwise).
|
||||||
|
|
||||||
|
3. **Analyze Context**
|
||||||
|
For each match, read surrounding code to determine verdict:
|
||||||
|
- **ISSUE**: Code should be changed (explain why)
|
||||||
|
- **WARNING**: Potential problem, needs review
|
||||||
|
- **OK**: Pattern detected but implementation is correct (explain why)
|
||||||
|
|
||||||
|
4. **Generate Report**
|
||||||
|
Output structured report with:
|
||||||
|
- Summary stats (issues/warnings/ok by category)
|
||||||
|
- Detailed findings with file:line references
|
||||||
|
- Recommendations for each issue
|
||||||
|
|
||||||
|
## Verdict Guidelines
|
||||||
|
|
||||||
|
### Error Handling
|
||||||
|
|
||||||
|
**ISSUE** when:
|
||||||
|
- Exception caught, logged, but execution continues with corrupted/partial state
|
||||||
|
- `None` returned for error condition without documentation
|
||||||
|
- Error logged but caller has no way to detect failure
|
||||||
|
|
||||||
|
**OK** when:
|
||||||
|
- Error logged AND exception re-raised with context
|
||||||
|
- Method returns explicit failure indicator (False, Result type) that caller checks
|
||||||
|
- Graceful degradation is intentional and documented
|
||||||
|
|
||||||
|
### Security
|
||||||
|
|
||||||
|
**ISSUE** when:
|
||||||
|
- User input used directly without validation
|
||||||
|
- Auth check missing on endpoint that modifies data
|
||||||
|
- Sensitive data (passwords, tokens) logged or exposed in errors
|
||||||
|
|
||||||
|
**OK** when:
|
||||||
|
- Input validated at API boundary before reaching service
|
||||||
|
- Auth handled by dependency injection (get_current_user)
|
||||||
|
- Sensitive fields excluded from serialization
|
||||||
|
|
||||||
|
### Architecture
|
||||||
|
|
||||||
|
**ISSUE** when:
|
||||||
|
- `app/core/` imports from `app/services/` or `app/api/`
|
||||||
|
- Service directly accesses ORM models instead of repository
|
||||||
|
- Global state modified without DI support for testing
|
||||||
|
|
||||||
|
**OK** when:
|
||||||
|
- Import is type-only (`if TYPE_CHECKING:`)
|
||||||
|
- Cross-layer access is through injected dependency
|
||||||
|
- Global has DI override capability in constructor
|
||||||
|
|
||||||
|
## Pattern File Format
|
||||||
|
|
||||||
|
```yaml
|
||||||
|
name: Category Name
|
||||||
|
description: What this category audits
|
||||||
|
|
||||||
|
patterns:
|
||||||
|
- id: unique-pattern-id
|
||||||
|
description: Human-readable description
|
||||||
|
grep_pattern: 'regex pattern for Grep tool'
|
||||||
|
file_glob: '*.py' # Optional, defaults to *.py
|
||||||
|
exclude_tests: true # Optional, defaults to true
|
||||||
|
multiline: false # Optional, for cross-line patterns
|
||||||
|
context_lines: 3 # Lines to read around match
|
||||||
|
verdict_hints:
|
||||||
|
ISSUE_IF: "condition that makes this an issue"
|
||||||
|
OK_IF: "condition that makes this acceptable"
|
||||||
|
```
|
||||||
|
|
||||||
|
## Output Format
|
||||||
|
|
||||||
|
```markdown
|
||||||
|
# Code Audit Report
|
||||||
|
|
||||||
|
**Scope:** backend/app/
|
||||||
|
**Categories:** errors, security, architecture
|
||||||
|
**Date:** YYYY-MM-DD
|
||||||
|
|
||||||
|
## Summary
|
||||||
|
|
||||||
|
| Category | Issues | Warnings | OK |
|
||||||
|
|----------|--------|----------|-----|
|
||||||
|
| Errors | 2 | 1 | 5 |
|
||||||
|
| Security | 0 | 2 | 3 |
|
||||||
|
| Architecture | 1 | 0 | 4 |
|
||||||
|
|
||||||
|
## Issues (Fix Required)
|
||||||
|
|
||||||
|
### [ISSUE] error-hiding.log-and-continue
|
||||||
|
**File:** app/services/card_service.py:209
|
||||||
|
**Pattern:** Log and continue on error
|
||||||
|
|
||||||
|
```python
|
||||||
|
except Exception as e:
|
||||||
|
logger.error(f"Failed to load card: {e}")
|
||||||
|
return None # Caller silently gets None
|
||||||
|
```
|
||||||
|
|
||||||
|
**Problem:** Caller has no way to detect failure. Cards silently missing.
|
||||||
|
**Recommendation:** Raise exception or return Result type with error info.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Warnings (Review Needed)
|
||||||
|
|
||||||
|
...
|
||||||
|
|
||||||
|
## OK (Verified Correct)
|
||||||
|
|
||||||
|
...
|
||||||
|
```
|
||||||
122
.claude/skills/code-audit/patterns/architecture.yaml
Normal file
122
.claude/skills/code-audit/patterns/architecture.yaml
Normal file
@ -0,0 +1,122 @@
|
|||||||
|
# Architecture Anti-Patterns
|
||||||
|
# Patterns that violate Mantimon TCG architecture principles
|
||||||
|
|
||||||
|
name: Architecture
|
||||||
|
description: |
|
||||||
|
Detects violations of the project's architecture principles:
|
||||||
|
- Core engine independence (offline fork support)
|
||||||
|
- Dependency injection (no monkey patching)
|
||||||
|
- Stateless backend (config from request)
|
||||||
|
- Repository pattern (services don't touch ORM directly)
|
||||||
|
|
||||||
|
patterns:
|
||||||
|
# Core Independence Violations
|
||||||
|
- id: core-imports-services
|
||||||
|
description: Core module importing from services layer
|
||||||
|
grep_pattern: 'from app\.services'
|
||||||
|
file_glob: 'app/core/**/*.py'
|
||||||
|
context_lines: 2
|
||||||
|
verdict_hints:
|
||||||
|
ISSUE_IF: "Core imports service - breaks offline fork capability"
|
||||||
|
OK_IF: "Never OK in core module"
|
||||||
|
|
||||||
|
- id: core-imports-api
|
||||||
|
description: Core module importing from API layer
|
||||||
|
grep_pattern: 'from app\.api'
|
||||||
|
file_glob: 'app/core/**/*.py'
|
||||||
|
context_lines: 2
|
||||||
|
verdict_hints:
|
||||||
|
ISSUE_IF: "Core imports API - breaks offline fork capability"
|
||||||
|
OK_IF: "Never OK in core module"
|
||||||
|
|
||||||
|
- id: core-imports-db
|
||||||
|
description: Core module importing database dependencies
|
||||||
|
grep_pattern: 'from (app\.db|sqlalchemy)'
|
||||||
|
file_glob: 'app/core/**/*.py'
|
||||||
|
context_lines: 2
|
||||||
|
verdict_hints:
|
||||||
|
ISSUE_IF: "Core has database dependency - breaks offline fork"
|
||||||
|
OK_IF: "Never OK in core module"
|
||||||
|
|
||||||
|
# Dependency Injection Violations
|
||||||
|
- id: service-global-in-method
|
||||||
|
description: Service accessing global inside method body
|
||||||
|
grep_pattern: '(get_db|get_session|get_redis)\(\)'
|
||||||
|
file_glob: 'app/services/**/*.py'
|
||||||
|
context_lines: 4
|
||||||
|
verdict_hints:
|
||||||
|
ISSUE_IF: "Global accessed in method - should be injected via constructor"
|
||||||
|
OK_IF: "In factory function or module-level singleton creation"
|
||||||
|
|
||||||
|
- id: patch-in-test
|
||||||
|
description: Using patch/monkeypatch instead of DI
|
||||||
|
grep_pattern: '(patch|monkeypatch)\.(object|setattr)'
|
||||||
|
file_glob: 'tests/**/*.py'
|
||||||
|
exclude_tests: false
|
||||||
|
context_lines: 4
|
||||||
|
verdict_hints:
|
||||||
|
ISSUE_IF: "Patching production code - use constructor injection"
|
||||||
|
OK_IF: "Patching external library; or testing legacy code marked for refactor"
|
||||||
|
|
||||||
|
- id: import-in-function
|
||||||
|
description: Import statement inside function body
|
||||||
|
grep_pattern: '^\s{4,}(from|import) '
|
||||||
|
context_lines: 3
|
||||||
|
verdict_hints:
|
||||||
|
ISSUE_IF: "Late import to avoid circular dep - restructure modules"
|
||||||
|
OK_IF: "Conditional import for optional dependency; TYPE_CHECKING guard"
|
||||||
|
|
||||||
|
# Stateless Backend Violations
|
||||||
|
- id: config-in-constructor
|
||||||
|
description: RulesConfig or DeckConfig stored in service constructor
|
||||||
|
grep_pattern: 'def __init__.*:\n.*self\.\w*(config|rules)\s*='
|
||||||
|
multiline: true
|
||||||
|
file_glob: 'app/services/**/*.py'
|
||||||
|
context_lines: 5
|
||||||
|
verdict_hints:
|
||||||
|
ISSUE_IF: "Config stored at construction - should come from request"
|
||||||
|
OK_IF: "Infrastructure config (timeouts, URLs) not game rules"
|
||||||
|
|
||||||
|
- id: default-config-creation
|
||||||
|
description: Creating default config instead of requiring it
|
||||||
|
grep_pattern: '(config|rules)\s*=\s*(config|rules)\s*or\s*(DeckConfig|RulesConfig)\(\)'
|
||||||
|
context_lines: 3
|
||||||
|
verdict_hints:
|
||||||
|
ISSUE_IF: "Hidden default config - caller should provide explicitly"
|
||||||
|
OK_IF: "In test fixtures or documented convenience wrapper"
|
||||||
|
|
||||||
|
# Repository Pattern Violations
|
||||||
|
- id: service-direct-orm
|
||||||
|
description: Service directly using SQLAlchemy session
|
||||||
|
grep_pattern: '(db|session)\.(execute|add|delete|commit)'
|
||||||
|
file_glob: 'app/services/**/*.py'
|
||||||
|
context_lines: 4
|
||||||
|
verdict_hints:
|
||||||
|
ISSUE_IF: "Service directly manipulates ORM - use repository"
|
||||||
|
OK_IF: "GameStateManager (special case); or transitional code"
|
||||||
|
|
||||||
|
- id: orm-model-in-service-return
|
||||||
|
description: Service returning ORM model instead of DTO
|
||||||
|
grep_pattern: 'return.*(User|Deck|Collection|ActiveGame)\b'
|
||||||
|
file_glob: 'app/services/**/*.py'
|
||||||
|
context_lines: 3
|
||||||
|
verdict_hints:
|
||||||
|
ISSUE_IF: "ORM model leaked to caller - return DTO/dataclass"
|
||||||
|
OK_IF: "Explicitly documented; or service in transition"
|
||||||
|
|
||||||
|
# Coupling Issues
|
||||||
|
- id: circular-import-guard
|
||||||
|
description: TYPE_CHECKING import guard (potential circular dep)
|
||||||
|
grep_pattern: 'if TYPE_CHECKING:'
|
||||||
|
context_lines: 5
|
||||||
|
verdict_hints:
|
||||||
|
ISSUE_IF: "Indicates circular dependency - consider restructuring"
|
||||||
|
OK_IF: "Type hints for complex generics; documented design choice"
|
||||||
|
|
||||||
|
- id: god-class
|
||||||
|
description: Class with too many methods (>15)
|
||||||
|
grep_pattern: 'class \w+.*:'
|
||||||
|
context_lines: 0
|
||||||
|
verdict_hints:
|
||||||
|
ISSUE_IF: "Class has >15 public methods - consider splitting"
|
||||||
|
OK_IF: "Cohesive responsibility; methods are related"
|
||||||
79
.claude/skills/code-audit/patterns/error-handling.yaml
Normal file
79
.claude/skills/code-audit/patterns/error-handling.yaml
Normal file
@ -0,0 +1,79 @@
|
|||||||
|
# Error Handling Anti-Patterns
|
||||||
|
# Patterns that hide errors instead of surfacing them
|
||||||
|
|
||||||
|
name: Error Handling
|
||||||
|
description: |
|
||||||
|
Detects patterns where errors are silently swallowed, logged but ignored,
|
||||||
|
or hidden behind default return values. These patterns make debugging hard
|
||||||
|
and can lead to systems running with corrupted or partial state.
|
||||||
|
|
||||||
|
patterns:
|
||||||
|
- id: log-and-continue
|
||||||
|
description: Logging error then continuing with default/None
|
||||||
|
grep_pattern: 'logger\.(warning|error|exception).*\n.*return (None|\[\]|\{\}|0|False)'
|
||||||
|
multiline: true
|
||||||
|
context_lines: 5
|
||||||
|
verdict_hints:
|
||||||
|
ISSUE_IF: "Caller cannot detect failure; continues with partial data"
|
||||||
|
OK_IF: "Method documents None as valid return; caller handles it"
|
||||||
|
|
||||||
|
- id: catch-log-pass
|
||||||
|
description: Exception caught, logged, but silently swallowed
|
||||||
|
grep_pattern: 'except.*:\n\s*(pass|logger\.(warning|error))'
|
||||||
|
multiline: true
|
||||||
|
context_lines: 5
|
||||||
|
verdict_hints:
|
||||||
|
ISSUE_IF: "Exception swallowed without re-raise or failure indicator"
|
||||||
|
OK_IF: "Exception is expected case (e.g., cache miss) with proper handling"
|
||||||
|
|
||||||
|
- id: bare-except
|
||||||
|
description: Bare except clause catches everything including SystemExit
|
||||||
|
grep_pattern: 'except:\s*$'
|
||||||
|
context_lines: 3
|
||||||
|
verdict_hints:
|
||||||
|
ISSUE_IF: "Catches KeyboardInterrupt, SystemExit - use Exception instead"
|
||||||
|
OK_IF: "Never OK - always specify exception type"
|
||||||
|
|
||||||
|
- id: broad-exception-catch
|
||||||
|
description: Catching Exception without re-raising
|
||||||
|
grep_pattern: 'except Exception.*:\n(?!.*raise)'
|
||||||
|
multiline: true
|
||||||
|
context_lines: 7
|
||||||
|
verdict_hints:
|
||||||
|
ISSUE_IF: "Broad catch without re-raise hides unexpected errors"
|
||||||
|
OK_IF: "Re-raises as domain exception with context preserved"
|
||||||
|
|
||||||
|
- id: silent-none-return
|
||||||
|
description: Returning None/empty on lookup failure without logging
|
||||||
|
grep_pattern: 'if .* is None:\n\s*return (None|\[\]|\{\})'
|
||||||
|
multiline: true
|
||||||
|
context_lines: 4
|
||||||
|
verdict_hints:
|
||||||
|
ISSUE_IF: "None indicates error condition but no logging/metric"
|
||||||
|
OK_IF: "None is valid business case (e.g., optional field not set)"
|
||||||
|
|
||||||
|
- id: continue-on-error
|
||||||
|
description: Loop continues past errors without tracking failures
|
||||||
|
grep_pattern: 'except.*:\n.*continue'
|
||||||
|
multiline: true
|
||||||
|
context_lines: 5
|
||||||
|
verdict_hints:
|
||||||
|
ISSUE_IF: "Errors silently skipped; loop completes with partial results"
|
||||||
|
OK_IF: "Failures tracked and reported after loop; caller informed"
|
||||||
|
|
||||||
|
- id: default-on-keyerror
|
||||||
|
description: Using .get() with default to hide missing keys
|
||||||
|
grep_pattern: '\.get\([^,]+,\s*(None|\[\]|\{\}|0|"")\)'
|
||||||
|
context_lines: 2
|
||||||
|
verdict_hints:
|
||||||
|
ISSUE_IF: "Missing key indicates data corruption; should raise"
|
||||||
|
OK_IF: "Key genuinely optional with documented default behavior"
|
||||||
|
|
||||||
|
- id: suppress-validation-error
|
||||||
|
description: Catching validation errors without surfacing to user
|
||||||
|
grep_pattern: 'except.*(ValidationError|ValueError|TypeError).*:\n.*return'
|
||||||
|
multiline: true
|
||||||
|
context_lines: 5
|
||||||
|
verdict_hints:
|
||||||
|
ISSUE_IF: "User provided bad input but gets generic response"
|
||||||
|
OK_IF: "Error converted to proper HTTP 400/422 with details"
|
||||||
96
.claude/skills/code-audit/patterns/security.yaml
Normal file
96
.claude/skills/code-audit/patterns/security.yaml
Normal file
@ -0,0 +1,96 @@
|
|||||||
|
# Security Anti-Patterns
|
||||||
|
# Patterns that may expose security vulnerabilities
|
||||||
|
|
||||||
|
name: Security
|
||||||
|
description: |
|
||||||
|
Detects patterns that could lead to security vulnerabilities including
|
||||||
|
missing authorization, data exposure, injection risks, and unsafe operations.
|
||||||
|
Specific to Mantimon TCG patterns and FastAPI/Python conventions.
|
||||||
|
|
||||||
|
patterns:
|
||||||
|
- id: missing-auth-dependency
|
||||||
|
description: Endpoint without authentication dependency
|
||||||
|
grep_pattern: '@router\.(post|put|patch|delete)\([^)]*\)\n(async )?def \w+\([^)]*\):'
|
||||||
|
multiline: true
|
||||||
|
context_lines: 5
|
||||||
|
file_glob: 'app/api/**/*.py'
|
||||||
|
verdict_hints:
|
||||||
|
ISSUE_IF: "Mutating endpoint without get_current_user dependency"
|
||||||
|
OK_IF: "Public endpoint (login, register, health) or has auth in deps"
|
||||||
|
|
||||||
|
- id: password-in-log
|
||||||
|
description: Password or secret potentially logged
|
||||||
|
grep_pattern: 'logger\.\w+\(.*["\']?.*(password|secret|token|key|credential).*["\']?'
|
||||||
|
context_lines: 2
|
||||||
|
verdict_hints:
|
||||||
|
ISSUE_IF: "Sensitive value included in log message"
|
||||||
|
OK_IF: "Just logging field name, not value; or intentionally masked"
|
||||||
|
|
||||||
|
- id: sql-string-format
|
||||||
|
description: SQL query built with string formatting (injection risk)
|
||||||
|
grep_pattern: '(execute|text)\([^)]*(%|\.format|f["\'])'
|
||||||
|
context_lines: 3
|
||||||
|
verdict_hints:
|
||||||
|
ISSUE_IF: "User input could be interpolated into SQL"
|
||||||
|
OK_IF: "Only constant values interpolated; user input uses params"
|
||||||
|
|
||||||
|
- id: hidden-game-info-exposure
|
||||||
|
description: Deck/hand contents potentially exposed to wrong player
|
||||||
|
grep_pattern: '(deck|hand|prize).*\.(cards|contents|order)'
|
||||||
|
context_lines: 5
|
||||||
|
file_glob: 'app/services/game_service.py'
|
||||||
|
verdict_hints:
|
||||||
|
ISSUE_IF: "Opponent's hidden zone contents sent to client"
|
||||||
|
OK_IF: "Only counts sent, or filtered through visibility layer"
|
||||||
|
|
||||||
|
- id: missing-ownership-check
|
||||||
|
description: Resource access without verifying ownership
|
||||||
|
grep_pattern: 'await.*(get_deck|get_collection|get_game).*\n(?!.*user_id|player_id)'
|
||||||
|
multiline: true
|
||||||
|
context_lines: 5
|
||||||
|
file_glob: 'app/api/**/*.py'
|
||||||
|
verdict_hints:
|
||||||
|
ISSUE_IF: "Resource fetched without checking user owns it"
|
||||||
|
OK_IF: "Ownership check in service layer or resource is public"
|
||||||
|
|
||||||
|
- id: hardcoded-secret
|
||||||
|
description: Hardcoded secret or API key in code
|
||||||
|
grep_pattern: '(api_key|secret|password|token)\s*=\s*["\'][^"\']{8,}["\']'
|
||||||
|
context_lines: 2
|
||||||
|
exclude_tests: false
|
||||||
|
verdict_hints:
|
||||||
|
ISSUE_IF: "Real credential hardcoded in source"
|
||||||
|
OK_IF: "Placeholder/example value; real value from environment"
|
||||||
|
|
||||||
|
- id: debug-mode-check
|
||||||
|
description: Debug mode check that might expose info in production
|
||||||
|
grep_pattern: 'if.*(debug|DEBUG|dev_mode).*:\n.*print|logger'
|
||||||
|
multiline: true
|
||||||
|
context_lines: 4
|
||||||
|
verdict_hints:
|
||||||
|
ISSUE_IF: "Sensitive data logged only when debug flag set"
|
||||||
|
OK_IF: "Debug info is non-sensitive; flag properly controlled"
|
||||||
|
|
||||||
|
- id: unsafe-pickle
|
||||||
|
description: Pickle usage (arbitrary code execution risk)
|
||||||
|
grep_pattern: 'pickle\.(load|loads)'
|
||||||
|
context_lines: 3
|
||||||
|
verdict_hints:
|
||||||
|
ISSUE_IF: "Unpickling data from untrusted source"
|
||||||
|
OK_IF: "Never OK for external data; use JSON/msgpack instead"
|
||||||
|
|
||||||
|
- id: eval-usage
|
||||||
|
description: eval() or exec() usage
|
||||||
|
grep_pattern: '(eval|exec)\s*\('
|
||||||
|
context_lines: 3
|
||||||
|
verdict_hints:
|
||||||
|
ISSUE_IF: "Evaluating user-controlled input"
|
||||||
|
OK_IF: "Extremely rare valid use; usually should refactor"
|
||||||
|
|
||||||
|
- id: cors-wildcard
|
||||||
|
description: CORS allowing all origins
|
||||||
|
grep_pattern: 'allow_origins\s*=\s*\["\*"\]|allow_origin_regex.*\.\*'
|
||||||
|
context_lines: 3
|
||||||
|
verdict_hints:
|
||||||
|
ISSUE_IF: "Production API allows any origin"
|
||||||
|
OK_IF: "Development only; production has specific origins"
|
||||||
Loading…
Reference in New Issue
Block a user