mirror of
https://github.com/azaion/detections.git
synced 2026-04-22 22:46:31 +00:00
Update .gitignore to include additional file types and directories for Python projects, enhancing environment management and build artifacts exclusion.
This commit is contained in:
Oleksandr Bezdieniezhnykh
@@ -0,0 +1,154 @@
|
||||
---
|
||||
name: code-review
|
||||
description: |
|
||||
Multi-phase code review against task specs with structured findings output.
|
||||
6-phase workflow: context loading, spec compliance, code quality, security quick-scan, performance scan, cross-task consistency.
|
||||
Produces a structured report with severity-ranked findings and a PASS/FAIL/PASS_WITH_WARNINGS verdict.
|
||||
Invoked by /implement skill after each batch, or manually.
|
||||
Trigger phrases:
|
||||
- "code review", "review code", "review implementation"
|
||||
- "check code quality", "review against specs"
|
||||
category: review
|
||||
tags: [code-review, quality, security-scan, performance, SOLID]
|
||||
disable-model-invocation: true
|
||||
---
|
||||
|
||||
# Code Review
|
||||
|
||||
Multi-phase code review that verifies implementation against task specs, checks code quality, and produces structured findings.
|
||||
|
||||
## Core Principles
|
||||
|
||||
- **Understand intent first**: read the task specs before reviewing code — know what it should do before judging how
|
||||
- **Structured output**: every finding has severity, category, location, description, and suggestion
|
||||
- **Deduplicate**: same issue at the same location is reported once using `{file}:{line}:{title}` as key
|
||||
- **Severity-ranked**: findings sorted Critical > High > Medium > Low
|
||||
- **Verdict-driven**: clear PASS/FAIL/PASS_WITH_WARNINGS drives automation decisions
|
||||
|
||||
## Input
|
||||
|
||||
- List of task spec files that were just implemented (paths to `[JIRA-ID]_[short_name].md`)
|
||||
- Changed files (detected via `git diff` or provided by the `/implement` skill)
|
||||
- Project context: `_docs/00_problem/restrictions.md`, `_docs/01_solution/solution.md`
|
||||
|
||||
## Phase 1: Context Loading
|
||||
|
||||
Before reviewing code, build understanding of intent:
|
||||
|
||||
1. Read each task spec — acceptance criteria, scope, constraints, dependencies
|
||||
2. Read project restrictions and solution overview
|
||||
3. Map which changed files correspond to which task specs
|
||||
4. Understand what the code is supposed to do before judging how it does it
|
||||
|
||||
## Phase 2: Spec Compliance Review
|
||||
|
||||
For each task, verify implementation satisfies every acceptance criterion:
|
||||
|
||||
- Walk through each AC (Given/When/Then) and trace it in the code
|
||||
- Check that unit tests cover each AC
|
||||
- Check that integration tests exist where specified in the task spec
|
||||
- Flag any AC that is not demonstrably satisfied as a **Spec-Gap** finding (severity: High)
|
||||
- Flag any scope creep (implementation beyond what the spec asked for) as a **Scope** finding (severity: Low)
|
||||
|
||||
## Phase 3: Code Quality Review
|
||||
|
||||
Check implemented code against quality standards:
|
||||
|
||||
- **SOLID principles** — single responsibility, open/closed, Liskov, interface segregation, dependency inversion
|
||||
- **Error handling** — consistent strategy, no bare catch/except, meaningful error messages
|
||||
- **Naming** — clear intent, follows project conventions
|
||||
- **Complexity** — functions longer than 50 lines or cyclomatic complexity > 10
|
||||
- **DRY** — duplicated logic across files
|
||||
- **Test quality** — tests assert meaningful behavior, not just "no error thrown"
|
||||
- **Dead code** — unused imports, unreachable branches
|
||||
|
||||
## Phase 4: Security Quick-Scan
|
||||
|
||||
Lightweight security checks (defer deep analysis to the `/security` skill):
|
||||
|
||||
- SQL injection via string interpolation
|
||||
- Command injection (subprocess with shell=True, exec, eval)
|
||||
- Hardcoded secrets, API keys, passwords
|
||||
- Missing input validation on external inputs
|
||||
- Sensitive data in logs or error messages
|
||||
- Insecure deserialization
|
||||
|
||||
## Phase 5: Performance Scan
|
||||
|
||||
Check for common performance anti-patterns:
|
||||
|
||||
- O(n^2) or worse algorithms where O(n) is possible
|
||||
- N+1 query patterns
|
||||
- Unbounded data fetching (missing pagination/limits)
|
||||
- Blocking I/O in async contexts
|
||||
- Unnecessary memory copies or allocations in hot paths
|
||||
|
||||
## Phase 6: Cross-Task Consistency
|
||||
|
||||
When multiple tasks were implemented in the same batch:
|
||||
|
||||
- Interfaces between tasks are compatible (method signatures, DTOs match)
|
||||
- No conflicting patterns (e.g., one task uses repository pattern, another does raw SQL)
|
||||
- Shared code is not duplicated across task implementations
|
||||
- Dependencies declared in task specs are properly wired
|
||||
|
||||
## Output Format
|
||||
|
||||
Produce a structured report with findings deduplicated and sorted by severity:
|
||||
|
||||
```markdown
|
||||
# Code Review Report
|
||||
|
||||
**Batch**: [task list]
|
||||
**Date**: [YYYY-MM-DD]
|
||||
**Verdict**: PASS | PASS_WITH_WARNINGS | FAIL
|
||||
|
||||
## Findings
|
||||
|
||||
| # | Severity | Category | File:Line | Title |
|
||||
|---|----------|----------|-----------|-------|
|
||||
| 1 | Critical | Security | src/api/auth.py:42 | SQL injection via f-string |
|
||||
| 2 | High | Spec-Gap | src/service/orders.py | AC-3 not satisfied |
|
||||
|
||||
### Finding Details
|
||||
|
||||
**F1: SQL injection via f-string** (Critical / Security)
|
||||
- Location: `src/api/auth.py:42`
|
||||
- Description: User input interpolated directly into SQL query
|
||||
- Suggestion: Use parameterized query via bind parameters
|
||||
- Task: 04_auth_service
|
||||
|
||||
**F2: AC-3 not satisfied** (High / Spec-Gap)
|
||||
- Location: `src/service/orders.py`
|
||||
- Description: AC-3 requires order total recalculation on item removal, but no such logic exists
|
||||
- Suggestion: Add recalculation in remove_item() method
|
||||
- Task: 07_order_processing
|
||||
```
|
||||
|
||||
## Severity Definitions
|
||||
|
||||
| Severity | Meaning | Blocks? |
|
||||
|----------|---------|---------|
|
||||
| Critical | Security vulnerability, data loss, crash | Yes — verdict FAIL |
|
||||
| High | Spec gap, logic bug, broken test | Yes — verdict FAIL |
|
||||
| Medium | Performance issue, maintainability concern, missing validation | No — verdict PASS_WITH_WARNINGS |
|
||||
| Low | Style, minor improvement, scope creep | No — verdict PASS_WITH_WARNINGS |
|
||||
|
||||
## Category Values
|
||||
|
||||
Bug, Spec-Gap, Security, Performance, Maintainability, Style, Scope
|
||||
|
||||
## Verdict Logic
|
||||
|
||||
- **FAIL**: any Critical or High finding exists
|
||||
- **PASS_WITH_WARNINGS**: only Medium or Low findings
|
||||
- **PASS**: no findings
|
||||
|
||||
## Integration with /implement
|
||||
|
||||
The `/implement` skill invokes this skill after each batch completes:
|
||||
|
||||
1. Collects changed files from all implementer agents in the batch
|
||||
2. Passes task spec paths + changed files to this skill
|
||||
3. If verdict is FAIL — presents findings to user (BLOCKING), user fixes or confirms
|
||||
4. If verdict is PASS or PASS_WITH_WARNINGS — proceeds automatically (findings shown as info)
|
||||
Reference in New Issue
Block a user