From 134685386932629e79f63ef6c563c1430c0f9e8a Mon Sep 17 00:00:00 2001 From: Cal Corum Date: Thu, 29 Jan 2026 18:59:17 -0600 Subject: [PATCH] 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 --- .claude/skills/code-audit/SKILL.md | 148 ++++++++++++++++++ .../code-audit/patterns/architecture.yaml | 122 +++++++++++++++ .../code-audit/patterns/error-handling.yaml | 79 ++++++++++ .../skills/code-audit/patterns/security.yaml | 96 ++++++++++++ 4 files changed, 445 insertions(+) create mode 100644 .claude/skills/code-audit/SKILL.md create mode 100644 .claude/skills/code-audit/patterns/architecture.yaml create mode 100644 .claude/skills/code-audit/patterns/error-handling.yaml create mode 100644 .claude/skills/code-audit/patterns/security.yaml diff --git a/.claude/skills/code-audit/SKILL.md b/.claude/skills/code-audit/SKILL.md new file mode 100644 index 0000000..d76d8ad --- /dev/null +++ b/.claude/skills/code-audit/SKILL.md @@ -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) + +... +``` diff --git a/.claude/skills/code-audit/patterns/architecture.yaml b/.claude/skills/code-audit/patterns/architecture.yaml new file mode 100644 index 0000000..4f83496 --- /dev/null +++ b/.claude/skills/code-audit/patterns/architecture.yaml @@ -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" diff --git a/.claude/skills/code-audit/patterns/error-handling.yaml b/.claude/skills/code-audit/patterns/error-handling.yaml new file mode 100644 index 0000000..e159d85 --- /dev/null +++ b/.claude/skills/code-audit/patterns/error-handling.yaml @@ -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" diff --git a/.claude/skills/code-audit/patterns/security.yaml b/.claude/skills/code-audit/patterns/security.yaml new file mode 100644 index 0000000..32612a9 --- /dev/null +++ b/.claude/skills/code-audit/patterns/security.yaml @@ -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"