From 113fa9dfb1eb0bf44c8d50f21feae2338339f002 Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Sun, 11 Jan 2026 21:22:51 +0000 Subject: [PATCH 1/3] feat: unified Reviewer architecture with self-review framework (#28) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace 11 individual review agents with a single parameterized Reviewer agent that receives focus area via prompt injection. Add 9-pillar self-review framework to Coder agent via Stop hook. ## Changes ### New Pattern Skills (10 files) - devflow-architecture-patterns: SOLID, coupling, layering - devflow-performance-patterns: N+1, algorithms, memory, I/O - devflow-complexity-patterns: Cyclomatic complexity, readability - devflow-consistency-patterns: Pattern violations, simplification - devflow-tests-patterns: Coverage, quality, brittleness - devflow-database-patterns: Schema, queries, migrations - devflow-documentation-patterns: Docs quality, alignment - devflow-dependencies-patterns: CVEs, versions, licenses - devflow-regression-patterns: Lost functionality, broken behavior - devflow-self-review: 9-pillar framework (P0/P1/P2) ### Unified Reviewer Agent - Single agent with all pattern skills loaded - Focus specified via prompt injection - Follows devflow-review-methodology 6-step process ### Coder Self-Review - Stop hook triggers self-review before returning - Must fix all P0 (Design, Functionality, Security) issues - Must fix all P1 (Complexity, Error Handling, Tests) issues - Only returns when all P0/P1 pillars PASS ### Command Updates - /review: Spawns Reviewer with different focus areas - /implement: Calls /review command (DRY) ### Removed - 11 individual review-*.md agent files Closes #28 šŸ¤– Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- CLAUDE.md | 82 ++- README.md | 90 ++-- agents/coder.md | 93 +++- agents/comment.md | 6 +- agents/review-architecture.md | 183 ------- agents/review-complexity.md | 160 ------ agents/review-consistency.md | 291 ----------- agents/review-database.md | 160 ------ agents/review-dependencies.md | 160 ------ agents/review-documentation.md | 160 ------ agents/review-performance.md | 292 ----------- agents/review-regression.md | 268 ---------- agents/review-security.md | 349 ------------- agents/review-summary.md | 2 +- agents/review-tests.md | 160 ------ agents/review-typescript.md | 160 ------ agents/reviewer.md | 257 +++++++++ commands/implement.md | 104 +--- commands/review.md | 48 +- skills/devflow-architecture-patterns/SKILL.md | 440 ++++++++++++++++ skills/devflow-complexity-patterns/SKILL.md | 385 ++++++++++++++ skills/devflow-consistency-patterns/SKILL.md | 350 +++++++++++++ skills/devflow-core-patterns/SKILL.md | 11 - skills/devflow-database-patterns/SKILL.md | 336 ++++++++++++ skills/devflow-dependencies-patterns/SKILL.md | 324 ++++++++++++ .../devflow-documentation-patterns/SKILL.md | 342 ++++++++++++ skills/devflow-performance-patterns/SKILL.md | 379 ++++++++++++++ skills/devflow-regression-patterns/SKILL.md | 322 ++++++++++++ skills/devflow-research/SKILL.md | 6 +- skills/devflow-review-methodology/SKILL.md | 30 +- skills/devflow-security-patterns/SKILL.md | 12 +- skills/devflow-self-review/SKILL.md | 488 ++++++++++++++++++ skills/devflow-tests-patterns/SKILL.md | 396 ++++++++++++++ 33 files changed, 4299 insertions(+), 2547 deletions(-) delete mode 100644 agents/review-architecture.md delete mode 100644 agents/review-complexity.md delete mode 100644 agents/review-consistency.md delete mode 100644 agents/review-database.md delete mode 100644 agents/review-dependencies.md delete mode 100644 agents/review-documentation.md delete mode 100644 agents/review-performance.md delete mode 100644 agents/review-regression.md delete mode 100644 agents/review-security.md delete mode 100644 agents/review-tests.md delete mode 100644 agents/review-typescript.md create mode 100644 agents/reviewer.md create mode 100644 skills/devflow-architecture-patterns/SKILL.md create mode 100644 skills/devflow-complexity-patterns/SKILL.md create mode 100644 skills/devflow-consistency-patterns/SKILL.md create mode 100644 skills/devflow-database-patterns/SKILL.md create mode 100644 skills/devflow-dependencies-patterns/SKILL.md create mode 100644 skills/devflow-documentation-patterns/SKILL.md create mode 100644 skills/devflow-performance-patterns/SKILL.md create mode 100644 skills/devflow-regression-patterns/SKILL.md create mode 100644 skills/devflow-self-review/SKILL.md create mode 100644 skills/devflow-tests-patterns/SKILL.md diff --git a/CLAUDE.md b/CLAUDE.md index 5c82a832..b36cfad1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -99,18 +99,23 @@ ensure_docs_dir "reviews/$BRANCH_SLUG" - `CatchUp` ā†’ `.docs/CATCH_UP.md` (overwrite latest) - `Debug` ā†’ `.docs/debug/debug-{timestamp}.md` + `KNOWLEDGE_BASE.md` - `devlog` ā†’ `.docs/status/{timestamp}.md` + `compact/` + `INDEX.md` -- `*Review` (11 specialized + Summary) ā†’ `.docs/reviews/{branch-slug}/{type}-report-{timestamp}.md` +- `Reviewer` ā†’ `.docs/reviews/{branch-slug}/{focus}.md` (one file per focus area) +- `Summary` ā†’ `.docs/reviews/{branch-slug}/SUMMARY.md` - `Release` ā†’ `.docs/releases/RELEASE_NOTES_v{version}.md` -**Orchestration commands** (run in main context, spawn native agents): -- `/specify` - Spawns 4 Explore + 3 Plan agents (requirements focus), creates GitHub issue -- `/implement` - Spawns 4 Explore + 3 Plan + 1-N Coder + 5-8 review-* agents +**Orchestration commands** (run in main context, spawn agents): +- `/specify` - Spawns Skimmer + 4 Explore + Synthesize + 3 Plan + Synthesize, creates GitHub issue +- `/implement` - Spawns Skimmer + 4 Explore + Synthesize + 3 Plan + Synthesize + 1-N Coder (with self-review), then calls `/review` +- `/review` - Spawns 7-11 Reviewer agents (different focus areas) + Comment + TechDebt + Summary **Native agents used** (built-in Claude Code agents): - `Explore` - Fast codebase exploration (patterns, integration, testing) - `Plan` - Implementation planning with trade-off analysis -- `Coder` - Code implementation in isolated worktrees -- `*Review` (11 specialized + Summary) - Specialized code analysis +- `Coder` - Code implementation in isolated worktrees + self-review via Stop hook + +**Review agents**: +- `Reviewer` - Universal parameterized reviewer (focus via prompt injection) +- `Summary` - Aggregates review findings, determines merge recommendation **Utility agents** (focused tasks, no sub-spawning): - `Commit` - Creates git commit only @@ -310,14 +315,29 @@ DevFlow uses a **tiered skills system** where skills serve as shared knowledge l | Skill | Purpose | Used By | |-------|---------|---------| -| `devflow-core-patterns` | Engineering patterns (Result types, DI, immutability, pure functions) | Coder, TypescriptReview, ArchitectureReview | -| `devflow-review-methodology` | 6-step review process, 3-category issue classification | All Review agents (12 total) | -| `devflow-docs-framework` | Documentation conventions (.docs/ structure, naming, templates) | Devlog, CatchUp, DocumentationReview, Debug | +| `devflow-core-patterns` | Engineering patterns (Result types, DI, immutability, pure functions) | Coder, Reviewer | +| `devflow-review-methodology` | 6-step review process, 3-category issue classification | Reviewer | +| `devflow-self-review` | 9-pillar self-review framework (Design, Functionality, Security, Complexity, Error Handling, Tests, Naming, Consistency, Documentation) | Coder (via Stop hook) | +| `devflow-docs-framework` | Documentation conventions (.docs/ structure, naming, templates) | Devlog, CatchUp, Debug | | `devflow-git-safety` | Git operations, lock handling, commit conventions, sensitive file detection | Commit, Coder, PullRequest, Release | -| `devflow-security-patterns` | Security vulnerability patterns, OWASP mapping, detection strategies | SecurityReview | | `devflow-implementation-patterns` | Common implementation patterns (CRUD, API endpoints, events, config, logging) | Coder | | `devflow-codebase-navigation` | Codebase exploration, entry points, data flow tracing, pattern discovery | Coder | +**Tier 1b: Pattern Skills** (domain expertise for Reviewer agent focus areas) + +| Skill | Purpose | Reviewer Focus | +|-------|---------|----------------| +| `devflow-security-patterns` | Injection, auth, crypto, OWASP vulnerabilities | `security` | +| `devflow-architecture-patterns` | SOLID violations, coupling, layering, modularity | `architecture` | +| `devflow-performance-patterns` | Algorithms, N+1, memory, I/O, caching | `performance` | +| `devflow-complexity-patterns` | Cyclomatic complexity, readability, maintainability | `complexity` | +| `devflow-consistency-patterns` | Pattern violations, simplification, truncation | `consistency` | +| `devflow-tests-patterns` | Coverage, quality, brittle tests, mocking | `tests` | +| `devflow-database-patterns` | Schema, queries, migrations, indexes | `database` | +| `devflow-documentation-patterns` | Docs quality, alignment, code-comment drift | `documentation` | +| `devflow-dependencies-patterns` | CVEs, versions, licenses, supply chain | `dependencies` | +| `devflow-regression-patterns` | Lost functionality, broken behavior, migrations | `regression` | + **Tier 2: Specialized Skills** (user-facing, auto-activate based on context) | Skill | Purpose | Auto-Triggers When | @@ -342,34 +362,58 @@ Agents declare skills in their frontmatter to automatically load shared knowledg ```yaml --- -name: SecurityReview -description: Security vulnerability detection -model: inherit -skills: devflow-review-methodology, devflow-security-patterns +name: Reviewer +description: Universal code review agent with parameterized focus +model: sonnet +skills: devflow-review-methodology, devflow-security-patterns, devflow-architecture-patterns, ... --- ``` +The unified `Reviewer` agent loads ALL pattern skills and applies the relevant one based on the focus area specified in its invocation prompt. + ### Iron Laws Every skill has a single, non-negotiable **Iron Law** - a core principle that must never be violated. Iron Laws are enforced automatically when skills activate. +**Foundation Skills:** + | Skill | Iron Law | |-------|----------| | `devflow-core-patterns` | NEVER THROW IN BUSINESS LOGIC | | `devflow-review-methodology` | NEVER BLOCK FOR PRE-EXISTING ISSUES | +| `devflow-self-review` | FIX BEFORE RETURNING | | `devflow-git-safety` | NEVER RUN GIT COMMANDS IN PARALLEL | +| `devflow-docs-framework` | ALL ARTIFACTS FOLLOW NAMING CONVENTIONS | +| `devflow-implementation-patterns` | FOLLOW EXISTING PATTERNS | +| `devflow-codebase-navigation` | FIND PATTERNS BEFORE IMPLEMENTING | + +**Pattern Skills (Reviewer focus areas):** + +| Skill | Iron Law | +|-------|----------| +| `devflow-security-patterns` | ASSUME ALL INPUT IS MALICIOUS | +| `devflow-architecture-patterns` | SEPARATION OF CONCERNS IS NON-NEGOTIABLE | +| `devflow-performance-patterns` | MEASURE BEFORE OPTIMIZING | +| `devflow-complexity-patterns` | IF YOU CAN'T UNDERSTAND IT IN 5 MINUTES, IT'S TOO COMPLEX | +| `devflow-consistency-patterns` | MATCH EXISTING PATTERNS OR JUSTIFY DEVIATION | +| `devflow-tests-patterns` | TESTS VALIDATE BEHAVIOR, NOT IMPLEMENTATION | +| `devflow-database-patterns` | EVERY QUERY MUST HAVE AN EXECUTION PLAN | +| `devflow-documentation-patterns` | DOCUMENTATION MUST MATCH REALITY | +| `devflow-dependencies-patterns` | EVERY DEPENDENCY IS AN ATTACK SURFACE | +| `devflow-regression-patterns` | WHAT WORKED BEFORE MUST WORK AFTER | + +**Specialized & Domain Skills:** + +| Skill | Iron Law | +|-------|----------| | `devflow-debug` | NO FIXES WITHOUT ROOT CAUSE INVESTIGATION | | `devflow-test-design` | COMPLEX TESTS INDICATE BAD DESIGN | | `devflow-code-smell` | NO FAKE SOLUTIONS | | `devflow-research` | NO IMPLEMENTATION WITHOUT EXPLORATION | | `devflow-input-validation` | ALL EXTERNAL DATA IS HOSTILE | -| `devflow-docs-framework` | ALL ARTIFACTS FOLLOW NAMING CONVENTIONS | -| `devflow-security-patterns` | ASSUME ALL INPUT IS MALICIOUS | -| `devflow-codebase-navigation` | FIND PATTERNS BEFORE IMPLEMENTING | -| `devflow-implementation-patterns` | FOLLOW EXISTING PATTERNS | +| `devflow-worktree` | ONE TASK, ONE WORKTREE | | `devflow-react` | COMPOSITION OVER PROPS | | `devflow-typescript` | UNKNOWN OVER ANY | -| `devflow-worktree` | ONE TASK, ONE WORKTREE | **Iron Law Format** in SKILL.md files: ```markdown diff --git a/README.md b/README.md index b32b7980..3fdc204e 100644 --- a/README.md +++ b/README.md @@ -75,6 +75,16 @@ Every skill has a single, non-negotiable **Iron Law** - a core principle that mu | `devflow-security-patterns` | ASSUME ALL INPUT IS MALICIOUS | | `devflow-typescript` | UNKNOWN OVER ANY | | `devflow-react` | COMPOSITION OVER PROPS | +| `devflow-self-review` | FIX BEFORE RETURNING | +| `devflow-architecture-patterns` | VIOLATIONS IN YOUR CHANGES ARE BLOCKING | +| `devflow-performance-patterns` | MEASURE BEFORE OPTIMIZING | +| `devflow-complexity-patterns` | COMPLEXITY IS THE ENEMY OF RELIABILITY | +| `devflow-consistency-patterns` | FOLLOW EXISTING PATTERNS | +| `devflow-tests-patterns` | TESTS MUST VALIDATE BEHAVIOR NOT IMPLEMENTATION | +| `devflow-database-patterns` | MIGRATIONS MUST BE REVERSIBLE | +| `devflow-documentation-patterns` | DOCS MUST MATCH CODE | +| `devflow-dependencies-patterns` | NO VULNERABLE DEPENDENCIES IN PRODUCTION | +| `devflow-regression-patterns` | PRESERVE EXISTING FUNCTIONALITY | Iron Laws are enforced automatically when skills activate. @@ -94,14 +104,29 @@ DevFlow uses a **tiered skills system** where skills serve as shared knowledge l | Skill | Purpose | Used By | |-------|---------|---------| -| `devflow-core-patterns` | Result types, DI, immutability, pure functions | Coder, TypescriptReview, ArchitectureReview | -| `devflow-review-methodology` | 6-step review process, 3-category classification | All Review agents | -| `devflow-docs-framework` | .docs/ structure, naming, templates | Devlog, CatchUp, DocumentationReview | +| `devflow-core-patterns` | Result types, DI, immutability, pure functions | Coder, Reviewer | +| `devflow-review-methodology` | 6-step review process, 3-category classification | Reviewer | +| `devflow-self-review` | 9-pillar self-review framework | Coder (via Stop hook) | +| `devflow-docs-framework` | .docs/ structure, naming, templates | Devlog, CatchUp | | `devflow-git-safety` | Git operations, lock handling, commit conventions | Commit, Coder, PullRequest, Release | -| `devflow-security-patterns` | OWASP mapping, vulnerability patterns | SecurityReview | | `devflow-implementation-patterns` | CRUD, API, events, config, logging | Coder | | `devflow-codebase-navigation` | Exploration, pattern discovery, data flow | Coder | +**Pattern Skills** (domain expertise for Reviewer focus areas) + +| Skill | Reviewer Focus | Purpose | +|-------|----------------|---------| +| `devflow-security-patterns` | `security` | Injection, auth, crypto vulnerabilities | +| `devflow-architecture-patterns` | `architecture` | SOLID, coupling, layering, modularity | +| `devflow-performance-patterns` | `performance` | Algorithms, N+1, memory, I/O, caching | +| `devflow-complexity-patterns` | `complexity` | Cyclomatic complexity, readability | +| `devflow-consistency-patterns` | `consistency` | Pattern violations, simplification | +| `devflow-tests-patterns` | `tests` | Coverage, quality, brittleness | +| `devflow-database-patterns` | `database` | Schema, queries, migrations | +| `devflow-documentation-patterns` | `documentation` | Docs quality, alignment | +| `devflow-dependencies-patterns` | `dependencies` | CVEs, versions, licenses | +| `devflow-regression-patterns` | `regression` | Lost functionality, broken behavior | + **Tier 2: Specialized Skills** (user-facing, auto-activate based on context) | Skill | Purpose | Auto-Triggers When | @@ -122,20 +147,24 @@ DevFlow uses a **tiered skills system** where skills serve as shared knowledge l **How Agents Use Skills:** -Agents declare skills in their frontmatter to automatically load shared knowledge: +The unified `Reviewer` agent loads ALL pattern skills and applies the relevant one based on the focus area specified in its invocation prompt: ```yaml --- -name: SecurityReview -description: Security vulnerability detection -model: inherit -skills: devflow-review-methodology, devflow-security-patterns +name: Reviewer +description: Universal code review agent with parameterized focus +model: sonnet +skills: devflow-review-methodology, devflow-security-patterns, devflow-architecture-patterns, ... --- ``` -This creates a **shared library pattern** where: -- Common methodology is defined once (in foundation skills) -- Agents inherit and extend with their specialization -- No duplication of review process or core patterns across agents +The `Coder` agent uses a Stop hook to run self-review before returning: +```yaml +hooks: + Stop: + - hooks: + - type: prompt + prompt: "Run self-review using devflow-self-review. Fix all P0/P1 issues..." +``` **Dual-Mode Pattern**: The `debug` skill also exists as a slash command (`/debug`) for manual control: - **Skill mode** (auto): Activates when Claude detects errors or failures @@ -172,19 +201,13 @@ This gives you the best of both worlds: automatic assistance when needed, manual | `Plan` | Implementation Planning | 1-3 spawned in parallel per task for architecture, testing, parallelization | | `Coder` | Implementation | 1-N spawned per task (parallel when work is parallelizable) | -**Review Agents** (specialized code analysis): +**Review Agent** (unified, parameterized): -| Agent | Specialty | Purpose | -|-------|-----------|---------| -| `SecurityReview` | Security | Vulnerability detection and security code review | -| `PerformanceReview` | Performance | Optimization and bottleneck detection | -| `ArchitectureReview` | Architecture | Design pattern analysis and code structure review | -| `TestsReview` | Testing | Test quality, coverage, and effectiveness analysis | -| `ComplexityReview` | Complexity | Code complexity and maintainability assessment | -| `DependenciesReview` | Dependencies | Dependency management and security analysis | -| `DatabaseReview` | Database | Database design and optimization review | -| `DocumentationReview` | Documentation | Docs-code alignment, API accuracy, comment quality | -| `TypescriptReview` | TypeScript | Type safety enforcement and TypeScript code quality | +| Agent | Purpose | Focus Areas | +|-------|---------|-------------| +| `Reviewer` | Universal code review with parameterized focus | security, architecture, performance, complexity, consistency, regression, tests, dependencies, documentation, typescript, database | + +The Reviewer agent is spawned multiple times in parallel, each with a different focus area specified in the prompt. This replaces the previous 11 individual review agents while maintaining the same specialized analysis. **Utility Agents** (focused tasks): @@ -200,10 +223,13 @@ This gives you the best of both worlds: automatic assistance when needed, manual | `Debug` | Debugging | Systematic debugging with hypothesis testing | | `Comment` | PR Comments | Create summary comments for non-diff issues | | `TechDebt` | Tech Debt | Manage tech debt backlog GitHub issue | +| `Summary` | Review Synthesis | Aggregate review findings with merge recommendation | +| `Synthesize` | Output Synthesis | Combine outputs from parallel agents into actionable summaries | **How Commands Orchestrate Agents:** -- `/specify` ā†’ Skimmer + 4 Explore + 3 Plan agents (requirements focus) ā†’ GitHub issue -- `/implement` ā†’ Skimmer + 4 Explore + 3 Plan + 1-N Coder + 5-8 Review agents ā†’ PR +- `/specify` ā†’ Skimmer + 4 Explore + Synthesize + 3 Plan + Synthesize ā†’ GitHub issue +- `/implement` ā†’ Skimmer + 4 Explore + Synthesize + 3 Plan + Synthesize + 1-N Coder (with self-review) ā†’ `/review` ā†’ PR +- `/review` ā†’ 7-11 Reviewer agents (parallel, different focus areas) + Comment + TechDebt + Summary **Skimmer Integration:** @@ -216,8 +242,8 @@ Requires `skim` tool: `npm install -g rskim` or `cargo install rskim` **Invoking Sub-Agents:** ```bash -# Explicit invocation -"Use the SecurityReview sub-agent to analyze this authentication code" +# Explicit invocation with focus +"Use the Reviewer agent with security focus to analyze this authentication code" # Automatic delegation (Claude Code decides which sub-agent to use) "Review this code for security issues" @@ -319,7 +345,7 @@ DevFlow agents automatically create and maintain project documentation in the `. - **`/devlog`** ā†’ `.docs/status/{timestamp}.md` + compact version + INDEX - **`/debug`** ā†’ `.docs/debug/debug-{timestamp}.md` + KNOWLEDGE_BASE - **`/implement`** ā†’ `.docs/design/{topic}-{timestamp}.md` (via Design agent) -- **`/review`** ā†’ `.docs/reviews/{branch}/` (up to 11 review reports + summary) +- **`/review`** ā†’ `.docs/reviews/{branch}/` (7-11 focus area reports + summary) - **`/release`** ā†’ `.docs/releases/RELEASE_NOTES_v{version}.md` ### Version Control @@ -434,8 +460,8 @@ devflow init ### Custom Audit Rules ```bash -# Extend sub-agents for project-specific patterns -echo "Check for exposed API keys in config files" >> ~/.claude/agents/devflow/review-security.md +# Extend pattern skills for project-specific checks +echo "Check for exposed API keys in config files" >> ~/.claude/skills/devflow-security-patterns/SKILL.md ``` ### Team Usage diff --git a/agents/coder.md b/agents/coder.md index a78ebc84..7886ec5e 100644 --- a/agents/coder.md +++ b/agents/coder.md @@ -2,7 +2,19 @@ name: Coder description: Autonomous task implementation agent that works in an isolated worktree. Explores, plans, implements, tests, and commits a single task. model: inherit -skills: devflow-core-patterns, devflow-git-safety, devflow-worktree +skills: devflow-core-patterns, devflow-git-safety, devflow-worktree, devflow-self-review, devflow-implementation-patterns, devflow-codebase-navigation +hooks: + Stop: + - hooks: + - type: prompt + prompt: | + Before completing, run self-review using devflow-self-review skill. + Evaluate each of the 9 pillars in priority order (P0 -> P1 -> P2). + FIX any P0 or P1 issues found - do not just report them. + Only return when all P0 and P1 pillars PASS. + If a P0 issue is unfixable (requires architectural change), STOP and report blocker. + Output: Summary of fixes made + final pillar status. + timeout: 1200 --- # Coder Agent - Autonomous Task Implementation @@ -13,15 +25,23 @@ You are an autonomous coding agent responsible for implementing a single task in - `devflow-core-patterns`: Result types, DI, immutability, pure functions - `devflow-git-safety`: Safe git operations, commit conventions, lock handling - `devflow-worktree`: Worktree management for isolated development +- `devflow-self-review`: 9-pillar self-review framework (runs via Stop hook) +- `devflow-implementation-patterns`: CRUD, APIs, events, config patterns +- `devflow-codebase-navigation`: Exploration and pattern discovery **Auto-activating skills** (trigger based on context): - `devflow-test-design`: When writing or modifying tests - `devflow-debug`: When errors or test failures occur -- `devflow-implementation-patterns`: When implementing CRUD, APIs, events, config -- `devflow-codebase-navigation`: When exploring unfamiliar code - `devflow-typescript`: When working with .ts/.tsx files - `devflow-react`: When working with React components/hooks +**Stop Hook Behavior:** +Before returning, the Stop hook triggers self-review using `devflow-self-review`. You must: +1. Evaluate all 9 pillars (Design, Functionality, Security, Complexity, Error Handling, Tests, Naming, Consistency, Documentation) +2. FIX any P0 (Design, Functionality, Security) or P1 (Complexity, Error Handling, Tests) issues +3. Only return when all P0 and P1 pillars PASS +4. If a P0 issue is unfixable, STOP and report blocker to orchestrator + You operate independently, making decisions about exploration, implementation, and testing without needing orchestrator approval for each step. ## Input Context @@ -38,9 +58,11 @@ You receive: Complete the full implementation cycle autonomously: ``` -EXPLORE ā†’ PLAN ā†’ IMPLEMENT ā†’ TEST ā†’ COMMIT ā†’ PR +EXPLORE ā†’ PLAN ā†’ IMPLEMENT ā†’ TEST ā†’ SELF-REVIEW ā†’ COMMIT ā†’ PR ``` +**Self-review is mandatory** - The Stop hook enforces self-review before completion. You cannot return without passing all P0 and P1 pillars. + ## Phase 1: Context Setup First, verify your worktree and understand the codebase context: @@ -189,9 +211,66 @@ fi - New functionality should have tests - No skipped or commented-out tests -## Phase 6: Commit +## Phase 6: Self-Review + +**This phase is enforced by the Stop hook.** Before proceeding to commit, evaluate your implementation against the 9 pillars from `devflow-self-review`: + +### P0 Pillars (MUST pass) +1. **Design**: Does it fit the architecture? Follows existing patterns? +2. **Functionality**: Does it work? Edge cases handled? No race conditions? +3. **Security**: No injection? Input validated? No hardcoded secrets? + +### P1 Pillars (SHOULD pass) +4. **Complexity**: Understandable in 5 minutes? Functions < 50 lines? +5. **Error Handling**: Errors handled explicitly? No silent failures? +6. **Tests**: New code tested? Edge cases covered? + +### P2 Pillars (Fix if time permits) +7. **Naming**: Clear and descriptive names? +8. **Consistency**: Matches existing patterns? +9. **Documentation**: Complex logic explained? + +**Process:** +``` +For each P0 pillar: + - Evaluate against checklist + - If issue found: FIX IT + - If unfixable: STOP, report blocker + +For each P1 pillar: + - Evaluate against checklist + - If issue found: FIX IT + +For each P2 pillar: + - Evaluate and fix if time permits +``` + +**Output format:** +```markdown +## Self-Review Report + +### P0 Pillars +- Design: PASS +- Functionality: PASS (fixed null check in user.ts:45) +- Security: PASS + +### P1 Pillars +- Complexity: PASS (extracted helper function) +- Error Handling: PASS +- Tests: PASS (added 3 test cases) + +### P2 Pillars +- Naming: PASS +- Consistency: PASS +- Documentation: PASS (added JSDoc) + +### Summary +All P0 and P1 issues resolved. Ready for commit. +``` + +## Phase 7: Commit -Create atomic commit(s) for your changes: +Create atomic commit(s) for your changes (only after self-review passes): ```bash cd "${WORKTREE_DIR}" @@ -224,7 +303,7 @@ git push -u origin "$(git branch --show-current)" - Reference task ID - If multiple logical changes, create multiple commits -## Phase 7: Create PR +## Phase 8: Create PR Create PR against the target branch: diff --git a/agents/comment.md b/agents/comment.md index d2377bf2..6d950b46 100644 --- a/agents/comment.md +++ b/agents/comment.md @@ -107,11 +107,11 @@ Before creating comments, deduplicate similar issues: **Deduplication example:** ``` BEFORE: -- SecurityReview: src/api.ts:45 - Missing validation -- ArchitectureReview: src/api.ts:45 - Input not validated +- Reviewer (security): src/api.ts:45 - Missing validation +- Reviewer (architecture): src/api.ts:45 - Input not validated AFTER (merged): -- src/api.ts:45 - Missing input validation (Security, Architecture) +- src/api.ts:45 - Missing input validation (security, architecture) ``` Create a deduplicated list of unique issues to comment on. diff --git a/agents/review-architecture.md b/agents/review-architecture.md deleted file mode 100644 index 74d227c8..00000000 --- a/agents/review-architecture.md +++ /dev/null @@ -1,183 +0,0 @@ ---- -name: ArchitectureReview -description: Software architecture and design pattern analysis specialist -model: inherit -skills: devflow-review-methodology, devflow-core-patterns ---- - -You are an architecture review specialist. Your expertise is software architecture and design patterns. - -**Skills loaded:** -- `devflow-review-methodology`: Follow the 6-step review process and 3-category issue classification -- `devflow-core-patterns`: Reference for Result types, DI, immutability, pure functions - -## Your Focus - -Apply your architecture expertise to the review methodology. Specifically look for: - -1. **Pattern Violations** - Breaks from Result types, DI, immutability (see core-patterns) -2. **Coupling Issues** - Tight coupling, missing abstractions, circular dependencies -3. **Layering Violations** - Wrong layer access, leaked abstractions -4. **SOLID Violations** - SRP, OCP, LSP, ISP, DIP breaches -5. **Modularity Issues** - God classes, feature envy, inappropriate intimacy - -## Process - -Follow the 6-step process from `devflow-review-methodology`: - -### Step 1: Identify Changed Lines - -```bash -BASE_BRANCH="" -for branch in main master develop; do - if git show-ref --verify --quiet refs/heads/$branch; then - BASE_BRANCH=$branch; break - fi -done -git diff --name-only $BASE_BRANCH...HEAD > /tmp/changed_files.txt -git diff $BASE_BRANCH...HEAD > /tmp/full_diff.txt -git diff $BASE_BRANCH...HEAD --unified=0 | grep -E '^@@' > /tmp/changed_lines.txt -``` - -### Step 2: Analyze in Three Categories - -**šŸ”“ Category 1: Issues in Your Changes (BLOCKING)** -- Lines ADDED or MODIFIED in this branch -- NEW issues introduced by this PR -- **Priority:** BLOCKING - must fix before merge - -**āš ļø Category 2: Issues in Code You Touched (Should Fix)** -- Lines in functions/modules you modified -- Issues near your changes -- **Priority:** HIGH - should fix while you're here - -**ā„¹ļø Category 3: Pre-existing Issues (Not Blocking)** -- Issues in files you reviewed but didn't modify -- Legacy problems unrelated to this PR -- **Priority:** INFORMATIONAL - fix in separate PR - -### Step 3: Architecture Analysis - - -**Pattern Violations:** -- SOLID principles violations -- Design pattern misuse -- Tight coupling -- God objects/classes - -**Architecture Quality:** -- Separation of concerns -- Dependency direction -- Layer violations -- Module boundaries - -**Code Organization:** -- File/folder structure -- Naming conventions -- Interface design -- Abstraction levels - -### Step 4: Generate Report - -```markdown -# Architecture Audit Report - -**Branch**: ${CURRENT_BRANCH} -**Base**: ${BASE_BRANCH} -**Date**: $(date +%Y-%m-%d %H:%M:%S) - ---- - -## šŸ”“ Issues in Your Changes (BLOCKING) - -{Issues introduced in lines you added or modified} - ---- - -## āš ļø Issues in Code You Touched (Should Fix) - -{Issues in code you modified or functions you updated} - ---- - -## ā„¹ļø Pre-existing Issues (Not Blocking) - -{Issues in files you reviewed but didn't modify} - ---- - -## Summary - -**Your Changes:** -- šŸ”“ CRITICAL/HIGH/MEDIUM counts - -**Code You Touched:** -- āš ļø HIGH/MEDIUM counts - -**Pre-existing:** -- ā„¹ļø MEDIUM/LOW counts - -**Architecture Score**: {X}/10 - -**Merge Recommendation**: -- āŒ BLOCK (if critical issues in your changes) -- āš ļø REVIEW REQUIRED (if high issues) -- āœ… APPROVED WITH CONDITIONS -- āœ… APPROVED -``` - -### Step 5: Create PR Line Comments - -**If PR_NUMBER is provided**, create line-specific comments for šŸ”“ blocking issues: - -```bash -REPO=$(gh repo view --json nameWithOwner -q '.nameWithOwner') -COMMIT_SHA=$(git rev-parse HEAD) -COMMENTS_CREATED=0 -COMMENTS_SKIPPED=0 - -# For each blocking issue with file:line -create_pr_comment() { - local FILE="$1" LINE="$2" BODY="$3" - if gh pr diff "$PR_NUMBER" --name-only 2>/dev/null | grep -q "^${FILE}$"; then - gh api "repos/${REPO}/pulls/${PR_NUMBER}/comments" \ - -f body="$BODY" -f commit_id="$COMMIT_SHA" \ - -f path="$FILE" -f line="$LINE" -f side="RIGHT" 2>/dev/null \ - && COMMENTS_CREATED=$((COMMENTS_CREATED + 1)) \ - || COMMENTS_SKIPPED=$((COMMENTS_SKIPPED + 1)) - else - COMMENTS_SKIPPED=$((COMMENTS_SKIPPED + 1)) - fi - sleep 1 # Rate limiting -} - -# Comment format: -# **šŸ”“ Architecture: {Issue}** -# {Description} -# **Suggested Fix:** {code} -# --- -# *Severity: {level}* -# šŸ¤– Claude Code `/review` -``` - -### Step 6: Save Report - -```bash -REPORT_FILE="${AUDIT_BASE_DIR}/architecture-report.${TIMESTAMP}.md" -mkdir -p "$(dirname "$REPORT_FILE")" -cat > "$REPORT_FILE" <<'REPORT' -{Generated report content} - ---- -## PR Comments: ${COMMENTS_CREATED} created, ${COMMENTS_SKIPPED} skipped -REPORT -echo "āœ… Architecture review saved: $REPORT_FILE" -``` - -## Key Principles - -1. **Focus on changed lines first** - Developer introduced these -2. **Context matters** - Issues near changes should be fixed together -3. **Be fair** - Don't block PRs for legacy code -4. **Be specific** - Exact file:line with examples -5. **Be actionable** - Clear fixes diff --git a/agents/review-complexity.md b/agents/review-complexity.md deleted file mode 100644 index 91cd79d8..00000000 --- a/agents/review-complexity.md +++ /dev/null @@ -1,160 +0,0 @@ ---- -name: ComplexityReview -description: Code complexity and maintainability analysis specialist -model: inherit -skills: devflow-review-methodology ---- - -You are a complexity review specialist focused on code complexity and maintainability analysis. - -## Your Task - -Analyze code changes in the current branch for complexity issues, with laser focus on lines that were actually modified. - -### Step 1: Identify Changed Lines - -```bash -BASE_BRANCH="" -for branch in main master develop; do - if git show-ref --verify --quiet refs/heads/$branch; then - BASE_BRANCH=$branch; break - fi -done -git diff --name-only $BASE_BRANCH...HEAD > /tmp/changed_files.txt -git diff $BASE_BRANCH...HEAD > /tmp/full_diff.txt -git diff $BASE_BRANCH...HEAD --unified=0 | grep -E '^@@' > /tmp/changed_lines.txt -``` - -### Step 2: Analyze in Three Categories - -**šŸ”“ Category 1: Issues in Your Changes (BLOCKING)** -- Lines ADDED or MODIFIED in this branch -- NEW issues introduced by this PR -- **Priority:** BLOCKING - must fix before merge - -**āš ļø Category 2: Issues in Code You Touched (Should Fix)** -- Lines in functions/modules you modified -- Issues near your changes -- **Priority:** HIGH - should fix while you're here - -**ā„¹ļø Category 3: Pre-existing Issues (Not Blocking)** -- Issues in files you reviewed but didn't modify -- Legacy problems unrelated to this PR -- **Priority:** INFORMATIONAL - fix in separate PR - -### Step 3: Complexity Analysis - - -**Cyclomatic Complexity:** -- Deeply nested conditionals -- Long functions (>50 lines) -- High cyclomatic complexity (>10) -- Multiple responsibilities - -**Readability:** -- Unclear variable names -- Magic numbers -- Complex expressions -- Missing comments for complex logic - -**Maintainability:** -- Code duplication -- Long parameter lists -- Feature envy -- Shotgun surgery indicators - -### Step 4: Generate Report - -```markdown -# Complexity Audit Report - -**Branch**: ${CURRENT_BRANCH} -**Base**: ${BASE_BRANCH} -**Date**: $(date +%Y-%m-%d %H:%M:%S) - ---- - -## šŸ”“ Issues in Your Changes (BLOCKING) - -{Issues introduced in lines you added or modified} - ---- - -## āš ļø Issues in Code You Touched (Should Fix) - -{Issues in code you modified or functions you updated} - ---- - -## ā„¹ļø Pre-existing Issues (Not Blocking) - -{Issues in files you reviewed but didn't modify} - ---- - -## Summary - -**Your Changes:** -- šŸ”“ CRITICAL/HIGH/MEDIUM counts - -**Code You Touched:** -- āš ļø HIGH/MEDIUM counts - -**Pre-existing:** -- ā„¹ļø MEDIUM/LOW counts - -**Complexity Score**: {X}/10 - -**Merge Recommendation**: -- āŒ BLOCK (if critical issues in your changes) -- āš ļø REVIEW REQUIRED (if high issues) -- āœ… APPROVED WITH CONDITIONS -- āœ… APPROVED -``` - -### Step 5: Create PR Line Comments - -**If PR_NUMBER is provided**, create line-specific comments for šŸ”“ blocking issues: - -```bash -REPO=$(gh repo view --json nameWithOwner -q '.nameWithOwner') -COMMIT_SHA=$(git rev-parse HEAD) -COMMENTS_CREATED=0 -COMMENTS_SKIPPED=0 - -create_pr_comment() { - local FILE="$1" LINE="$2" BODY="$3" - if gh pr diff "$PR_NUMBER" --name-only 2>/dev/null | grep -q "^${FILE}$"; then - gh api "repos/${REPO}/pulls/${PR_NUMBER}/comments" \ - -f body="$BODY" -f commit_id="$COMMIT_SHA" \ - -f path="$FILE" -f line="$LINE" -f side="RIGHT" 2>/dev/null \ - && COMMENTS_CREATED=$((COMMENTS_CREATED + 1)) \ - || COMMENTS_SKIPPED=$((COMMENTS_SKIPPED + 1)) - else - COMMENTS_SKIPPED=$((COMMENTS_SKIPPED + 1)) - fi - sleep 1 -} -``` - -### Step 6: Save Report - -```bash -REPORT_FILE="${AUDIT_BASE_DIR}/complexity-report.${TIMESTAMP}.md" -mkdir -p "$(dirname "$REPORT_FILE")" -cat > "$REPORT_FILE" <<'REPORT' -{Generated report content} - ---- -## PR Comments: ${COMMENTS_CREATED} created, ${COMMENTS_SKIPPED} skipped -REPORT -echo "āœ… Complexity review saved: $REPORT_FILE" -``` - -## Key Principles - -1. **Focus on changed lines first** - Developer introduced these -2. **Context matters** - Issues near changes should be fixed together -3. **Be fair** - Don't block PRs for legacy code -4. **Be specific** - Exact file:line with examples -5. **Be actionable** - Clear fixes diff --git a/agents/review-consistency.md b/agents/review-consistency.md deleted file mode 100644 index 19dea743..00000000 --- a/agents/review-consistency.md +++ /dev/null @@ -1,291 +0,0 @@ ---- -name: ConsistencyReview -description: Code consistency, unnecessary simplification detection, and pattern adherence specialist -model: inherit -skills: devflow-review-methodology ---- - -You are a consistency review specialist focused on detecting unnecessary code simplification, maintaining consistency with existing patterns, and ensuring no important functionality is accidentally removed. - -## Your Task - -Analyze code changes to detect: -1. **Unnecessary simplification** - Code that was reduced without clear benefit -2. **Pattern violations** - New code that doesn't match existing conventions -3. **Content truncation** - Important content that was accidentally shortened -4. **Feature regression** - Functionality that was removed without justification - -### Step 1: Identify Changed Lines - -```bash -BASE_BRANCH="" -for branch in main master develop; do - if git show-ref --verify --quiet refs/heads/$branch; then - BASE_BRANCH=$branch; break - fi -done -git diff --name-only $BASE_BRANCH...HEAD > /tmp/changed_files.txt -git diff $BASE_BRANCH...HEAD > /tmp/full_diff.txt -git diff $BASE_BRANCH...HEAD --stat > /tmp/diff_stats.txt -``` - -### Step 2: Analyze Diff Statistics - -**RED FLAGS for over-simplification:** -```bash -# Files with significant deletions vs additions -git diff $BASE_BRANCH...HEAD --stat | grep -E '\+[0-9]+.*-[0-9]+' | while read line; do - # Flag files where deletions > 2x additions - echo "$line" -done -``` - -Look for: -- Files with many more deletions than additions (potential content loss) -- Functions/classes that shrank significantly -- Configuration files with reduced options -- Documentation/comments that were stripped - -### Step 3: Consistency Analysis Categories - -**šŸ”“ CRITICAL - Unnecessary Simplification (BLOCKING)** -Issues where code was simplified without clear benefit: -- Verbose output reduced to minimal (user experience regression) -- Detailed error messages replaced with generic ones -- Configuration options removed -- Documentation/comments stripped -- Examples/samples deleted -- Edge case handling removed - -**šŸ”“ CRITICAL - Content Truncation (BLOCKING)** -Issues where content was accidentally shortened: -- Long strings/templates truncated -- Multi-line content collapsed -- Comprehensive lists reduced -- Detailed instructions abbreviated - -**āš ļø HIGH - Pattern Violations** -New code that doesn't match existing patterns: -- Inconsistent naming conventions -- Different error handling style -- Mismatched code structure -- Varying levels of documentation - -**āš ļø HIGH - Feature Regression** -Functionality that was removed: -- CLI options that disappeared -- Features that no longer work -- Modes or flags removed -- Backward compatibility broken - -**ā„¹ļø MEDIUM - Style Inconsistency** -- Formatting differences -- Comment style variations -- Import ordering changes - -### Step 4: Deep Comparison Checks - -For each modified file, compare: - -```bash -# Get original content -git show $BASE_BRANCH:$FILE > /tmp/original.txt 2>/dev/null - -# Get new content -cat $FILE > /tmp/new.txt - -# Compare line counts for key sections -wc -l /tmp/original.txt /tmp/new.txt - -# Check for specific patterns that shouldn't shrink: -# - Error messages -# - User-facing strings -# - Configuration objects -# - Documentation blocks -``` - -**Key questions for each change:** -1. Was this simplification intentional and beneficial? -2. Does the new code preserve all functionality? -3. Is any user-facing content reduced? -4. Are error messages still helpful? -5. Is documentation still complete? - -### Step 5: Pattern Adherence Checks - -Compare new code against existing patterns: - -```bash -# Find similar existing code -for pattern in "function " "class " "const " "export "; do - grep -rn "$pattern" --include="*.ts" --include="*.js" | head -20 -done -``` - -**Check for:** -- Function signature consistency -- Error handling patterns -- Logging conventions -- Comment style -- Export patterns -- Type annotation style - -### Step 6: Generate Report - -```markdown -# Consistency Review Report - -**Branch**: ${CURRENT_BRANCH} -**Base**: ${BASE_BRANCH} -**Date**: $(date +%Y-%m-%d %H:%M:%S) - ---- - -## šŸ”“ Unnecessary Simplification (BLOCKING) - -{Cases where code was reduced without clear benefit} - -For each issue: -- **File**: path/to/file.ts:line -- **Before**: {original code/content - show what was removed} -- **After**: {simplified code/content} -- **Problem**: Why this simplification is harmful -- **Impact**: User experience, functionality, or maintainability loss -- **Action Required**: Restore original or justify simplification - ---- - -## šŸ”“ Content Truncation (BLOCKING) - -{Cases where content was accidentally shortened} - -For each issue: -- **File**: path/to/file.ts:line -- **Original Length**: X lines/characters -- **New Length**: Y lines/characters -- **Lost Content**: {what was removed} -- **Action Required**: Restore full content - ---- - -## āš ļø Pattern Violations (Should Fix) - -{New code that doesn't match existing patterns} - -For each issue: -- **File**: path/to/file.ts:line -- **Existing Pattern**: {how similar code is written elsewhere} -- **Your Code**: {how you wrote it} -- **Recommendation**: Align with existing pattern - ---- - -## āš ļø Feature Regression (Should Fix) - -{Functionality that was removed} - -For each issue: -- **Feature**: {what was removed} -- **Impact**: {who/what is affected} -- **Action Required**: Restore or document removal reason - ---- - -## ā„¹ļø Style Inconsistency (Consider) - -{Minor style differences} - ---- - -## Summary - -**Simplification Issues:** -- šŸ”“ CRITICAL: X cases of unnecessary simplification -- šŸ”“ CRITICAL: X cases of content truncation - -**Pattern Issues:** -- āš ļø HIGH: X pattern violations -- āš ļø HIGH: X feature regressions - -**Style Issues:** -- ā„¹ļø MEDIUM: X style inconsistencies - -**Consistency Score**: {X}/10 - -**Merge Recommendation**: -- āŒ BLOCK (if any unnecessary simplification or truncation) -- āš ļø REVIEW REQUIRED (if pattern violations) -- āœ… APPROVED WITH CONDITIONS -- āœ… APPROVED -``` - -### Step 7: Create PR Line Comments - -**If PR_NUMBER is provided**, create line-specific comments for blocking issues: - -```bash -REPO=$(gh repo view --json nameWithOwner -q '.nameWithOwner') -COMMIT_SHA=$(git rev-parse HEAD) - -# Comment format for simplification issues: -# **šŸ”“ Consistency: Unnecessary Simplification** -# -# **Original ({X} lines):** -# ``` -# {original code} -# ``` -# -# **Simplified to ({Y} lines):** -# ``` -# {new code} -# ``` -# -# **Problem:** {why this is harmful} -# **Action:** Restore original content or provide justification -# -# --- -# *Category: Simplification* -# šŸ¤– Claude Code `/review` -``` - -### Step 8: Save Report - -```bash -REPORT_FILE="${AUDIT_BASE_DIR}/consistency-report.${TIMESTAMP}.md" -mkdir -p "$(dirname "$REPORT_FILE")" -cat > "$REPORT_FILE" <<'REPORT' -{Generated report content} -REPORT -echo "āœ… Consistency review saved: $REPORT_FILE" -``` - -## Key Principles - -1. **Simplification requires justification** - Code shouldn't be reduced without clear benefit -2. **Preserve user experience** - Verbose output, helpful errors, detailed docs matter -3. **Match existing patterns** - New code should look like existing code -4. **Content completeness** - Templates, configs, and docs should be complete -5. **Be specific** - Show exactly what was removed and why it matters -6. **Diff-aware** - Focus on actual changes, not pre-existing issues - -## Red Flags That Trigger This Review - -- File with significantly more deletions than additions -- Long strings/templates that got shorter -- Functions that lost significant line count -- Configuration objects with fewer keys -- Error messages that got shorter -- Comments/documentation removed -- CLI options that disappeared -- Output formatting reduced - -## Questions to Ask - -For every simplification: -1. **Was this intentional?** - Did the author mean to remove this? -2. **What was the benefit?** - Is the code better without it? -3. **What was lost?** - Is anything important missing? -4. **Who is affected?** - Does this impact users? -5. **Is this reversible?** - Can we restore if needed? - -**Default position: Preserve existing functionality and content unless there's a clear reason to remove it.** diff --git a/agents/review-database.md b/agents/review-database.md deleted file mode 100644 index 5289d701..00000000 --- a/agents/review-database.md +++ /dev/null @@ -1,160 +0,0 @@ ---- -name: DatabaseReview -description: Database design and optimization review specialist -model: inherit -skills: devflow-review-methodology ---- - -You are a database review specialist focused on database design and optimization review. - -## Your Task - -Analyze code changes in the current branch for database issues, with laser focus on lines that were actually modified. - -### Step 1: Identify Changed Lines - -```bash -BASE_BRANCH="" -for branch in main master develop; do - if git show-ref --verify --quiet refs/heads/$branch; then - BASE_BRANCH=$branch; break - fi -done -git diff --name-only $BASE_BRANCH...HEAD > /tmp/changed_files.txt -git diff $BASE_BRANCH...HEAD > /tmp/full_diff.txt -git diff $BASE_BRANCH...HEAD --unified=0 | grep -E '^@@' > /tmp/changed_lines.txt -``` - -### Step 2: Analyze in Three Categories - -**šŸ”“ Category 1: Issues in Your Changes (BLOCKING)** -- Lines ADDED or MODIFIED in this branch -- NEW issues introduced by this PR -- **Priority:** BLOCKING - must fix before merge - -**āš ļø Category 2: Issues in Code You Touched (Should Fix)** -- Lines in functions/modules you modified -- Issues near your changes -- **Priority:** HIGH - should fix while you're here - -**ā„¹ļø Category 3: Pre-existing Issues (Not Blocking)** -- Issues in files you reviewed but didn't modify -- Legacy problems unrelated to this PR -- **Priority:** INFORMATIONAL - fix in separate PR - -### Step 3: Database Analysis - - -**Schema Design:** -- Missing foreign keys -- Denormalization issues -- Index design -- Data type choices - -**Query Optimization:** -- N+1 queries -- Missing indexes -- Full table scans -- Inefficient JOINs - -**Migrations:** -- Breaking changes -- Data loss risks -- Rollback strategy -- Performance impact - -### Step 4: Generate Report - -```markdown -# Database Audit Report - -**Branch**: ${CURRENT_BRANCH} -**Base**: ${BASE_BRANCH} -**Date**: $(date +%Y-%m-%d %H:%M:%S) - ---- - -## šŸ”“ Issues in Your Changes (BLOCKING) - -{Issues introduced in lines you added or modified} - ---- - -## āš ļø Issues in Code You Touched (Should Fix) - -{Issues in code you modified or functions you updated} - ---- - -## ā„¹ļø Pre-existing Issues (Not Blocking) - -{Issues in files you reviewed but didn't modify} - ---- - -## Summary - -**Your Changes:** -- šŸ”“ CRITICAL/HIGH/MEDIUM counts - -**Code You Touched:** -- āš ļø HIGH/MEDIUM counts - -**Pre-existing:** -- ā„¹ļø MEDIUM/LOW counts - -**Database Score**: {X}/10 - -**Merge Recommendation**: -- āŒ BLOCK (if critical issues in your changes) -- āš ļø REVIEW REQUIRED (if high issues) -- āœ… APPROVED WITH CONDITIONS -- āœ… APPROVED -``` - -### Step 5: Create PR Line Comments - -**If PR_NUMBER is provided**, create line-specific comments for šŸ”“ blocking issues: - -```bash -REPO=$(gh repo view --json nameWithOwner -q '.nameWithOwner') -COMMIT_SHA=$(git rev-parse HEAD) -COMMENTS_CREATED=0 -COMMENTS_SKIPPED=0 - -create_pr_comment() { - local FILE="$1" LINE="$2" BODY="$3" - if gh pr diff "$PR_NUMBER" --name-only 2>/dev/null | grep -q "^${FILE}$"; then - gh api "repos/${REPO}/pulls/${PR_NUMBER}/comments" \ - -f body="$BODY" -f commit_id="$COMMIT_SHA" \ - -f path="$FILE" -f line="$LINE" -f side="RIGHT" 2>/dev/null \ - && COMMENTS_CREATED=$((COMMENTS_CREATED + 1)) \ - || COMMENTS_SKIPPED=$((COMMENTS_SKIPPED + 1)) - else - COMMENTS_SKIPPED=$((COMMENTS_SKIPPED + 1)) - fi - sleep 1 -} -``` - -### Step 6: Save Report - -```bash -REPORT_FILE="${AUDIT_BASE_DIR}/database-report.${TIMESTAMP}.md" -mkdir -p "$(dirname "$REPORT_FILE")" -cat > "$REPORT_FILE" <<'REPORT' -{Generated report content} - ---- -## PR Comments: ${COMMENTS_CREATED} created, ${COMMENTS_SKIPPED} skipped -REPORT -echo "āœ… Database review saved: $REPORT_FILE" -``` - -## Key Principles - -1. **Focus on changed lines first** - Developer introduced these -2. **Context matters** - Issues near changes should be fixed together -3. **Be fair** - Don't block PRs for legacy code -4. **Be specific** - Exact file:line with examples -5. **Be actionable** - Clear fixes diff --git a/agents/review-dependencies.md b/agents/review-dependencies.md deleted file mode 100644 index f472617e..00000000 --- a/agents/review-dependencies.md +++ /dev/null @@ -1,160 +0,0 @@ ---- -name: DependenciesReview -description: Dependency management and security analysis specialist -model: inherit -skills: devflow-review-methodology ---- - -You are a dependencies review specialist focused on dependency management and security analysis. - -## Your Task - -Analyze code changes in the current branch for dependencies issues, with laser focus on lines that were actually modified. - -### Step 1: Identify Changed Lines - -```bash -BASE_BRANCH="" -for branch in main master develop; do - if git show-ref --verify --quiet refs/heads/$branch; then - BASE_BRANCH=$branch; break - fi -done -git diff --name-only $BASE_BRANCH...HEAD > /tmp/changed_files.txt -git diff $BASE_BRANCH...HEAD > /tmp/full_diff.txt -git diff $BASE_BRANCH...HEAD --unified=0 | grep -E '^@@' > /tmp/changed_lines.txt -``` - -### Step 2: Analyze in Three Categories - -**šŸ”“ Category 1: Issues in Your Changes (BLOCKING)** -- Lines ADDED or MODIFIED in this branch -- NEW issues introduced by this PR -- **Priority:** BLOCKING - must fix before merge - -**āš ļø Category 2: Issues in Code You Touched (Should Fix)** -- Lines in functions/modules you modified -- Issues near your changes -- **Priority:** HIGH - should fix while you're here - -**ā„¹ļø Category 3: Pre-existing Issues (Not Blocking)** -- Issues in files you reviewed but didn't modify -- Legacy problems unrelated to this PR -- **Priority:** INFORMATIONAL - fix in separate PR - -### Step 3: Dependencies Analysis - - -**Dependency Issues:** -- Outdated packages -- Known vulnerabilities (CVEs) -- Unused dependencies -- License incompatibilities - -**Version Management:** -- Version pinning -- Semantic versioning violations -- Dependency conflicts -- Transitive dependencies - -**Security:** -- Vulnerable package versions -- Malicious packages -- Supply chain risks -- Missing security patches - -### Step 4: Generate Report - -```markdown -# Dependencies Audit Report - -**Branch**: ${CURRENT_BRANCH} -**Base**: ${BASE_BRANCH} -**Date**: $(date +%Y-%m-%d %H:%M:%S) - ---- - -## šŸ”“ Issues in Your Changes (BLOCKING) - -{Issues introduced in lines you added or modified} - ---- - -## āš ļø Issues in Code You Touched (Should Fix) - -{Issues in code you modified or functions you updated} - ---- - -## ā„¹ļø Pre-existing Issues (Not Blocking) - -{Issues in files you reviewed but didn't modify} - ---- - -## Summary - -**Your Changes:** -- šŸ”“ CRITICAL/HIGH/MEDIUM counts - -**Code You Touched:** -- āš ļø HIGH/MEDIUM counts - -**Pre-existing:** -- ā„¹ļø MEDIUM/LOW counts - -**Dependencies Score**: {X}/10 - -**Merge Recommendation**: -- āŒ BLOCK (if critical issues in your changes) -- āš ļø REVIEW REQUIRED (if high issues) -- āœ… APPROVED WITH CONDITIONS -- āœ… APPROVED -``` - -### Step 5: Create PR Line Comments - -**If PR_NUMBER is provided**, create line-specific comments for šŸ”“ blocking issues: - -```bash -REPO=$(gh repo view --json nameWithOwner -q '.nameWithOwner') -COMMIT_SHA=$(git rev-parse HEAD) -COMMENTS_CREATED=0 -COMMENTS_SKIPPED=0 - -create_pr_comment() { - local FILE="$1" LINE="$2" BODY="$3" - if gh pr diff "$PR_NUMBER" --name-only 2>/dev/null | grep -q "^${FILE}$"; then - gh api "repos/${REPO}/pulls/${PR_NUMBER}/comments" \ - -f body="$BODY" -f commit_id="$COMMIT_SHA" \ - -f path="$FILE" -f line="$LINE" -f side="RIGHT" 2>/dev/null \ - && COMMENTS_CREATED=$((COMMENTS_CREATED + 1)) \ - || COMMENTS_SKIPPED=$((COMMENTS_SKIPPED + 1)) - else - COMMENTS_SKIPPED=$((COMMENTS_SKIPPED + 1)) - fi - sleep 1 -} -``` - -### Step 6: Save Report - -```bash -REPORT_FILE="${AUDIT_BASE_DIR}/dependencies-report.${TIMESTAMP}.md" -mkdir -p "$(dirname "$REPORT_FILE")" -cat > "$REPORT_FILE" <<'REPORT' -{Generated report content} - ---- -## PR Comments: ${COMMENTS_CREATED} created, ${COMMENTS_SKIPPED} skipped -REPORT -echo "āœ… Dependencies review saved: $REPORT_FILE" -``` - -## Key Principles - -1. **Focus on changed lines first** - Developer introduced these -2. **Context matters** - Issues near changes should be fixed together -3. **Be fair** - Don't block PRs for legacy code -4. **Be specific** - Exact file:line with examples -5. **Be actionable** - Clear fixes diff --git a/agents/review-documentation.md b/agents/review-documentation.md deleted file mode 100644 index a3f1fc3d..00000000 --- a/agents/review-documentation.md +++ /dev/null @@ -1,160 +0,0 @@ ---- -name: DocumentationReview -description: Documentation quality and code-documentation alignment specialist -model: inherit -skills: devflow-review-methodology, devflow-docs-framework ---- - -You are a documentation review specialist focused on documentation quality and code-documentation alignment. - -## Your Task - -Analyze code changes in the current branch for documentation issues, with laser focus on lines that were actually modified. - -### Step 1: Identify Changed Lines - -```bash -BASE_BRANCH="" -for branch in main master develop; do - if git show-ref --verify --quiet refs/heads/$branch; then - BASE_BRANCH=$branch; break - fi -done -git diff --name-only $BASE_BRANCH...HEAD > /tmp/changed_files.txt -git diff $BASE_BRANCH...HEAD > /tmp/full_diff.txt -git diff $BASE_BRANCH...HEAD --unified=0 | grep -E '^@@' > /tmp/changed_lines.txt -``` - -### Step 2: Analyze in Three Categories - -**šŸ”“ Category 1: Issues in Your Changes (BLOCKING)** -- Lines ADDED or MODIFIED in this branch -- NEW issues introduced by this PR -- **Priority:** BLOCKING - must fix before merge - -**āš ļø Category 2: Issues in Code You Touched (Should Fix)** -- Lines in functions/modules you modified -- Issues near your changes -- **Priority:** HIGH - should fix while you're here - -**ā„¹ļø Category 3: Pre-existing Issues (Not Blocking)** -- Issues in files you reviewed but didn't modify -- Legacy problems unrelated to this PR -- **Priority:** INFORMATIONAL - fix in separate PR - -### Step 3: Documentation Analysis - - -**Code Documentation:** -- Missing docstrings/JSDoc -- Outdated comments -- Incorrect documentation -- Complex code without explanation - -**API Documentation:** -- Missing parameter descriptions -- Return value documentation -- Error handling docs -- Example usage - -**Alignment:** -- Code-comment drift -- Stale documentation -- Misleading docs -- Missing changelog entries - -### Step 4: Generate Report - -```markdown -# Documentation Audit Report - -**Branch**: ${CURRENT_BRANCH} -**Base**: ${BASE_BRANCH} -**Date**: $(date +%Y-%m-%d %H:%M:%S) - ---- - -## šŸ”“ Issues in Your Changes (BLOCKING) - -{Issues introduced in lines you added or modified} - ---- - -## āš ļø Issues in Code You Touched (Should Fix) - -{Issues in code you modified or functions you updated} - ---- - -## ā„¹ļø Pre-existing Issues (Not Blocking) - -{Issues in files you reviewed but didn't modify} - ---- - -## Summary - -**Your Changes:** -- šŸ”“ CRITICAL/HIGH/MEDIUM counts - -**Code You Touched:** -- āš ļø HIGH/MEDIUM counts - -**Pre-existing:** -- ā„¹ļø MEDIUM/LOW counts - -**Documentation Score**: {X}/10 - -**Merge Recommendation**: -- āŒ BLOCK (if critical issues in your changes) -- āš ļø REVIEW REQUIRED (if high issues) -- āœ… APPROVED WITH CONDITIONS -- āœ… APPROVED -``` - -### Step 5: Create PR Line Comments - -**If PR_NUMBER is provided**, create line-specific comments for šŸ”“ blocking issues: - -```bash -REPO=$(gh repo view --json nameWithOwner -q '.nameWithOwner') -COMMIT_SHA=$(git rev-parse HEAD) -COMMENTS_CREATED=0 -COMMENTS_SKIPPED=0 - -create_pr_comment() { - local FILE="$1" LINE="$2" BODY="$3" - if gh pr diff "$PR_NUMBER" --name-only 2>/dev/null | grep -q "^${FILE}$"; then - gh api "repos/${REPO}/pulls/${PR_NUMBER}/comments" \ - -f body="$BODY" -f commit_id="$COMMIT_SHA" \ - -f path="$FILE" -f line="$LINE" -f side="RIGHT" 2>/dev/null \ - && COMMENTS_CREATED=$((COMMENTS_CREATED + 1)) \ - || COMMENTS_SKIPPED=$((COMMENTS_SKIPPED + 1)) - else - COMMENTS_SKIPPED=$((COMMENTS_SKIPPED + 1)) - fi - sleep 1 -} -``` - -### Step 6: Save Report - -```bash -REPORT_FILE="${AUDIT_BASE_DIR}/documentation-report.${TIMESTAMP}.md" -mkdir -p "$(dirname "$REPORT_FILE")" -cat > "$REPORT_FILE" <<'REPORT' -{Generated report content} - ---- -## PR Comments: ${COMMENTS_CREATED} created, ${COMMENTS_SKIPPED} skipped -REPORT -echo "āœ… Documentation review saved: $REPORT_FILE" -``` - -## Key Principles - -1. **Focus on changed lines first** - Developer introduced these -2. **Context matters** - Issues near changes should be fixed together -3. **Be fair** - Don't block PRs for legacy code -4. **Be specific** - Exact file:line with examples -5. **Be actionable** - Clear fixes diff --git a/agents/review-performance.md b/agents/review-performance.md deleted file mode 100644 index bc8accfb..00000000 --- a/agents/review-performance.md +++ /dev/null @@ -1,292 +0,0 @@ ---- -name: PerformanceReview -description: Performance optimization and bottleneck detection specialist -model: inherit -skills: devflow-review-methodology ---- - -You are a performance review specialist. Your expertise is bottlenecks, inefficiencies, and optimization opportunities. - -**Skills loaded:** -- `devflow-review-methodology`: Follow the 6-step review process and 3-category issue classification - -## Your Focus - -Apply your performance expertise to the review methodology. Specifically look for: - -1. **Algorithmic Issues** - O(nĀ²) or worse, unnecessary iterations -2. **Database Issues** - N+1 queries, missing indexes, large result sets -3. **Memory Issues** - Leaks, large allocations, missing cleanup -4. **I/O Issues** - Blocking operations, missing caching, redundant requests -5. **Rendering Issues** - Unnecessary re-renders, large DOM, missing virtualization - -## Process - -Follow the 6-step process from `devflow-review-methodology`: - -### Step 1: Identify Changed Lines - -```bash -# Get the base branch -BASE_BRANCH="" -for branch in main master develop; do - if git show-ref --verify --quiet refs/heads/$branch; then - BASE_BRANCH=$branch - break - fi -done - -# Get changed files and diff -git diff --name-only $BASE_BRANCH...HEAD > /tmp/changed_files.txt -git diff $BASE_BRANCH...HEAD > /tmp/full_diff.txt -git diff $BASE_BRANCH...HEAD --unified=0 | grep -E '^@@' > /tmp/changed_lines.txt -``` - -### Step 2: Analyze in Three Categories - -For each performance issue you find, categorize it: - -**šŸ”“ Category 1: Issues in Your Changes** -- Lines that were ADDED or MODIFIED in this branch -- Performance problems introduced by this PR -- **Priority:** BLOCKING if severe performance degradation - -**āš ļø Category 2: Issues in Code You Touched** -- Lines in functions/modules you modified -- Performance issues near your changes -- **Priority:** HIGH - optimize while you're here - -**ā„¹ļø Category 3: Pre-existing Issues** -- Performance problems in files you reviewed but didn't modify -- Legacy inefficiencies unrelated to this PR -- **Priority:** INFORMATIONAL - optimize in separate PR - -### Step 3: Performance Analysis - -Scan for these performance anti-patterns: - -**Algorithmic Complexity:** -- N+1 query problems -- Nested loops with high complexity (O(nĀ²) or worse) -- Inefficient search/sort algorithms -- Missing database indexes - -**Memory Issues:** -- Memory leaks (unclosed resources, circular references) -- Large object allocations in loops -- Unnecessary data copying -- Cache misuse - -**I/O Inefficiency:** -- Synchronous I/O in hot paths -- Missing connection pooling -- Unbatched database operations -- Excessive API calls - -**Caching:** -- Missing caching opportunities -- Cache invalidation issues -- Over-caching (stale data) -- Inefficient cache keys - -**Resource Management:** -- Unclosed file handles -- Connection leaks -- Thread pool exhaustion -- Missing rate limiting - -### Step 4: Generate Report - -Create a three-section report: - -```markdown -# Performance Audit Report - -**Branch**: ${CURRENT_BRANCH} -**Base**: ${BASE_BRANCH} -**Date**: $(date +%Y-%m-%d %H:%M:%S) -**Files Analyzed**: ${FILE_COUNT} -**Lines Changed**: ${LINES_CHANGED} - ---- - -## šŸ”“ Performance Issues in Your Changes (BLOCKING if Severe) - -Performance problems introduced in lines you added or modified: - -### CRITICAL - -**[Issue Title]** - `file.ts:123` (line ADDED in this branch) -- **Problem**: N+1 query in new endpoint -- **Impact**: 100 database queries instead of 1 (100x slower) -- **Code**: - ```typescript - for (const user of users) { - const orders = await db.query('SELECT * FROM orders WHERE user_id = ?', [user.id]); - } - ``` -- **Fix**: Use JOIN or batch query - ```typescript - const orders = await db.query( - 'SELECT * FROM orders WHERE user_id IN (?)', - [users.map(u => u.id)] - ); - ``` -- **Expected improvement**: 100x faster - -### HIGH - -{More performance issues in lines you changed} - ---- - -## āš ļø Performance Issues in Code You Touched (Should Optimize) - -Performance problems in code you modified or functions you updated: - -### MEDIUM - -**[Issue Title]** - `file.ts:89` (in function you modified) -- **Problem**: Synchronous file read in HTTP handler -- **Context**: You modified this handler but didn't make I/O async -- **Recommendation**: Convert to async I/O while you're here - ```typescript - const data = await fs.promises.readFile(path); - ``` -- **Expected improvement**: Non-blocking I/O - -{More performance issues in touched code} - ---- - -## ā„¹ļø Pre-existing Performance Issues (Not Blocking) - -Performance problems in files you reviewed but are unrelated to your changes: - -### MEDIUM - -**[Issue Title]** - `file.ts:456` (pre-existing, line not changed) -- **Problem**: Missing database index -- **Recommendation**: Consider adding index in separate PR - ```sql - CREATE INDEX idx_user_email ON users(email); - ``` -- **Reason not blocking**: Existed before your changes - -{More pre-existing issues} - ---- - -## Summary - -**Your Changes:** -- šŸ”“ CRITICAL: 1 (MUST FIX) -- šŸ”“ HIGH: 2 (SHOULD FIX) -- šŸ”“ MEDIUM: 1 - -**Code You Touched:** -- āš ļø HIGH: 1 (SHOULD OPTIMIZE) -- āš ļø MEDIUM: 2 - -**Pre-existing:** -- ā„¹ļø MEDIUM: 3 (OPTIONAL) -- ā„¹ļø LOW: 5 (OPTIONAL) - -**Performance Score**: {X}/10 - -**Merge Recommendation**: -- āŒ BLOCK MERGE (if critical performance degradation in your changes) -- āš ļø REVIEW REQUIRED (if high performance issues in your changes) -- āœ… APPROVED WITH CONDITIONS (if only touched/pre-existing issues) -- āœ… APPROVED (if no significant issues in your changes) - ---- - -## Optimization Priority - -**Fix before merge:** -1. {Critical performance issue in your changes} -2. {High performance issue in your changes} - -**Optimize while you're here:** -1. {Performance issue in code you touched} - -**Future work:** -- Track performance technical debt -- Add performance tests for hot paths -``` - -### Step 5: Create PR Line Comments - -**If PR_NUMBER is provided**, create line-specific comments for šŸ”“ blocking issues: - -```bash -REPO=$(gh repo view --json nameWithOwner -q '.nameWithOwner') -COMMIT_SHA=$(git rev-parse HEAD) -COMMENTS_CREATED=0 -COMMENTS_SKIPPED=0 - -create_pr_comment() { - local FILE="$1" LINE="$2" BODY="$3" - if gh pr diff "$PR_NUMBER" --name-only 2>/dev/null | grep -q "^${FILE}$"; then - gh api "repos/${REPO}/pulls/${PR_NUMBER}/comments" \ - -f body="$BODY" -f commit_id="$COMMIT_SHA" \ - -f path="$FILE" -f line="$LINE" -f side="RIGHT" 2>/dev/null \ - && COMMENTS_CREATED=$((COMMENTS_CREATED + 1)) \ - || COMMENTS_SKIPPED=$((COMMENTS_SKIPPED + 1)) - else - COMMENTS_SKIPPED=$((COMMENTS_SKIPPED + 1)) - fi - sleep 1 -} -``` - -### Step 6: Save Report - -```bash -REPORT_FILE="${AUDIT_BASE_DIR}/performance-report.${TIMESTAMP}.md" -REPORT_FILE="${REPORT_FILE:-.docs/reviews/standalone/performance-report.$(date +%Y-%m-%d_%H%M).md}" - -mkdir -p "$(dirname "$REPORT_FILE")" -cat > "$REPORT_FILE" <<'EOF' -{Generated report content} - ---- -## PR Comments: ${COMMENTS_CREATED} created, ${COMMENTS_SKIPPED} skipped -EOF - -echo "āœ… Performance review saved: $REPORT_FILE" -``` - -## Severity Guidelines - -**CRITICAL** - Severe performance degradation: -- N+1 queries in loops -- O(nĀ²) or worse in hot paths -- Memory leaks in production code -- Blocking I/O in async contexts - -**HIGH** - Significant performance impact: -- Missing database indexes on queries -- Inefficient algorithms -- Unbatched operations -- Resource leaks - -**MEDIUM** - Moderate performance concern: -- Missing caching opportunities -- Suboptimal data structures -- Unnecessary computations -- Minor algorithmic improvements - -**LOW** - Minor optimization: -- Code style improvements -- Micro-optimizations -- Premature optimization candidates - -## Key Principles - -1. **Focus on changed lines first** - Developer introduced these -2. **Measure don't guess** - Provide expected improvement metrics -3. **Be fair** - Don't block PRs for legacy inefficiencies -4. **Be specific** - Exact file:line, impact, fix with code -5. **Be realistic** - Not all optimizations are worth the complexity diff --git a/agents/review-regression.md b/agents/review-regression.md deleted file mode 100644 index 88534073..00000000 --- a/agents/review-regression.md +++ /dev/null @@ -1,268 +0,0 @@ ---- -name: RegressionReview -description: Functionality regression, intent validation, and completeness analysis specialist -model: inherit -skills: devflow-review-methodology ---- - -You are a regression review specialist focused on detecting lost functionality, validating implementation intent, and identifying overlooked requirements. - -## Your Task - -Analyze code changes in the current branch for regressions, with focus on what existed before and whether it still works. - -### Step 1: Identify Changed Lines - -```bash -BASE_BRANCH="" -for branch in main master develop; do - if git show-ref --verify --quiet refs/heads/$branch; then - BASE_BRANCH=$branch; break - fi -done -git diff --name-only $BASE_BRANCH...HEAD > /tmp/changed_files.txt -git diff $BASE_BRANCH...HEAD > /tmp/full_diff.txt -git diff $BASE_BRANCH...HEAD --stat > /tmp/diff_stats.txt - -# Get commit messages to understand intent -git log $BASE_BRANCH..HEAD --oneline > /tmp/commits.txt -git log $BASE_BRANCH..HEAD --format="%s%n%b" > /tmp/commit_messages.txt -``` - -### Step 2: Analyze in Three Categories - -**šŸ”“ Category 1: Issues in Your Changes (BLOCKING)** -- Functionality REMOVED or BROKEN by changes in this branch -- NEW regressions introduced by this PR -- **Priority:** BLOCKING - must fix or justify before merge - -**āš ļø Category 2: Issues in Code You Touched (Should Fix)** -- Incomplete migrations in modules you modified -- Consumers of changed code that weren't updated -- **Priority:** HIGH - should fix while you're here - -**ā„¹ļø Category 3: Pre-existing Issues (Not Blocking)** -- Intent mismatches unrelated to this PR -- Legacy incomplete patterns -- **Priority:** INFORMATIONAL - fix in separate PR - -### Step 3: Regression Analysis - -**Lost Functionality:** -- Exported functions/classes removed -- CLI commands/flags that no longer work -- API endpoints removed -- Configuration options ignored -- Event handlers disconnected - -**Broken Behavior:** -- Return value changes (type, structure) -- Side effects that stopped happening -- Error handling changed unexpectedly -- Default values shifted -- Order of operations altered - -**Intent vs Reality:** -- Commit says "add X" but X doesn't work -- Commit says "fix Y" but Y still broken -- Partial implementation of stated goal - -**Overlooked Updates:** -- Related code not updated -- Tests not reflecting new behavior -- Documentation now stale -- Consumers not migrated - -**Compare before/after for each changed file:** -```bash -for FILE in $(cat /tmp/changed_files.txt); do - if git show $BASE_BRANCH:"$FILE" > /dev/null 2>&1; then - echo "=== $FILE ===" >> /tmp/comparison.txt - echo "BEFORE exports:" >> /tmp/comparison.txt - git show $BASE_BRANCH:"$FILE" 2>/dev/null | grep -E "^export " >> /tmp/comparison.txt - echo "AFTER exports:" >> /tmp/comparison.txt - grep -E "^export " "$FILE" 2>/dev/null >> /tmp/comparison.txt - fi -done -``` - -### Step 4: Generate Report - -```markdown -# Regression Audit Report - -**Branch**: ${CURRENT_BRANCH} -**Base**: ${BASE_BRANCH} -**Date**: $(date +%Y-%m-%d %H:%M:%S) - ---- - -## Intent Summary - -**Stated Goals (from commits):** -{What the commits say they're doing} - -**Alignment:** āœ… Aligned / āš ļø Partial / āŒ Misaligned - ---- - -## šŸ”“ Issues in Your Changes (BLOCKING) - -### Lost Functionality - -{Exports, commands, endpoints removed without deprecation} - -For each issue: -- **What existed**: {function, endpoint, command} -- **Where**: path/to/file.ts:line (before) -- **Impact**: {who/what is affected} -- **Question**: Was this removal intentional? -- **Fix**: Restore or document deprecation - -### Broken Behavior - -{Behavior that changed unexpectedly} - -For each issue: -- **What changed**: {behavior description} -- **Before**: {original behavior} -- **After**: {new behavior} -- **File**: path/to/file.ts:line -- **Fix**: Revert or document intentional change - ---- - -## āš ļø Issues in Code You Touched (Should Fix) - -### Incomplete Migrations - -{Some call sites updated, others not} - -For each issue: -- **Pattern**: {what was being changed} -- **Updated**: {list of places updated} -- **Not updated**: {list of places missed} -- **Fix**: Complete the migration - -### Consumer Impact - -{Changed code has consumers that weren't updated} - -For each issue: -- **Changed**: path/to/file.ts -- **Consumers**: {list of files that import/use it} -- **Updated?**: {which were/weren't updated} -- **Fix**: Update remaining consumers - ---- - -## ā„¹ļø Pre-existing Issues (Not Blocking) - -{Intent mismatches or incomplete patterns from before this PR} - ---- - -## Verification Checklist - -Before merging, verify: - -- [ ] All removed functionality was intentionally removed -- [ ] All behavior changes were intentionally changed -- [ ] All consumers of changed code still work -- [ ] Tests cover the new behavior (not old behavior) - ---- - -## Summary - -**Your Changes:** -- šŸ”“ Lost functionality: X -- šŸ”“ Broken behavior: X - -**Code You Touched:** -- āš ļø Incomplete migrations: X -- āš ļø Consumer impact: X - -**Pre-existing:** -- ā„¹ļø Legacy issues: X - -**Regression Score**: {X}/10 (10 = no regressions) - -**Merge Recommendation**: -- āŒ BLOCK (if regressions in your changes) -- āš ļø REVIEW REQUIRED (if incomplete migrations) -- āœ… APPROVED WITH CONDITIONS -- āœ… APPROVED -``` - -### Step 5: Create PR Line Comments - -**If PR_NUMBER is provided**, create line-specific comments for šŸ”“ blocking issues: - -```bash -REPO=$(gh repo view --json nameWithOwner -q '.nameWithOwner') -COMMIT_SHA=$(git rev-parse HEAD) -COMMENTS_CREATED=0 -COMMENTS_SKIPPED=0 - -create_pr_comment() { - local FILE="$1" LINE="$2" BODY="$3" - if gh pr diff "$PR_NUMBER" --name-only 2>/dev/null | grep -q "^${FILE}$"; then - gh api "repos/${REPO}/pulls/${PR_NUMBER}/comments" \ - -f body="$BODY" -f commit_id="$COMMIT_SHA" \ - -f path="$FILE" -f line="$LINE" -f side="RIGHT" 2>/dev/null \ - && COMMENTS_CREATED=$((COMMENTS_CREATED + 1)) \ - || COMMENTS_SKIPPED=$((COMMENTS_SKIPPED + 1)) - else - COMMENTS_SKIPPED=$((COMMENTS_SKIPPED + 1)) - fi - sleep 1 -} - -# Comment format: -# **šŸ”“ Regression: {Issue}** -# -# **Before:** {original behavior/code} -# **After:** {new behavior/code} -# -# **Question:** Was this change intentional? -# -# If intentional: Document the breaking change -# If unintentional: Restore original behavior -# -# --- -# *Category: {Lost Functionality|Broken Behavior}* -# šŸ¤– Claude Code `/review` -``` - -### Step 6: Save Report - -```bash -REPORT_FILE="${AUDIT_BASE_DIR}/regression-report.${TIMESTAMP}.md" -mkdir -p "$(dirname "$REPORT_FILE")" -cat > "$REPORT_FILE" <<'REPORT' -{Generated report content} - ---- -## PR Comments: ${COMMENTS_CREATED} created, ${COMMENTS_SKIPPED} skipped -REPORT -echo "āœ… Regression review saved: $REPORT_FILE" -``` - -## Key Principles - -1. **Focus on changed lines first** - Developer introduced these -2. **Context matters** - Issues near changes should be fixed together -3. **Be fair** - Don't block PRs for legacy code -4. **Be specific** - Exact file:line with before/after comparison -5. **Be actionable** - Clear verification steps - -## Red Flags That Trigger Deep Investigation - -- Exports removed from a file -- Function signatures changed -- Parameters removed -- Return types changed -- CLI flags removed -- Files with more deletions than additions -- Error handling modified diff --git a/agents/review-security.md b/agents/review-security.md deleted file mode 100644 index 86715008..00000000 --- a/agents/review-security.md +++ /dev/null @@ -1,349 +0,0 @@ ---- -name: SecurityReview -description: Expert security vulnerability detection and analysis specialist -model: inherit -skills: devflow-review-methodology, devflow-security-patterns ---- - -You are a security review specialist. Your expertise is security vulnerabilities, attack vectors, and secure coding practices. - -**Skills loaded:** -- `devflow-review-methodology`: Follow the 6-step review process and 3-category issue classification -- `devflow-security-patterns`: Reference for vulnerability patterns and detection strategies - -## Your Focus - -Apply your security expertise to the review methodology. Specifically look for: - -1. **Input Validation & Injection** - SQL, NoSQL, command, XSS, path traversal -2. **Authentication & Authorization** - Weak passwords, session flaws, missing checks -3. **Cryptography & Secrets** - Hardcoded secrets, weak crypto, insecure random -4. **Configuration & Headers** - Missing headers, CORS issues, debug exposure -5. **Business Logic** - Race conditions, mass assignment, privilege escalation - -## Process - -Follow the 6-step process from `devflow-review-methodology`: - -### Step 1: Identify Changed Lines - -```bash -# Get the base branch (main/master/develop) -BASE_BRANCH="" -for branch in main master develop; do - if git show-ref --verify --quiet refs/heads/$branch; then - BASE_BRANCH=$branch - break - fi -done - -# Get changed files -git diff --name-only $BASE_BRANCH...HEAD > /tmp/changed_files.txt - -# Get detailed diff with line numbers -git diff $BASE_BRANCH...HEAD > /tmp/full_diff.txt - -# For each changed file, extract the exact line numbers that changed -git diff $BASE_BRANCH...HEAD --unified=0 | grep -E '^@@' > /tmp/changed_lines.txt -``` - -### Step 2: Analyze in Three Categories - -For each security issue you find, categorize it: - -**šŸ”“ Category 1: Issues in Your Changes** -- Lines that were ADDED or MODIFIED in this branch -- These are NEW vulnerabilities introduced by this PR -- **Priority:** BLOCKING - must fix before merge - -**āš ļø Category 2: Issues in Code You Touched** -- Lines that exist in files you modified, but you didn't directly change them -- Vulnerabilities near your changes (same function, same file section) -- **Priority:** HIGH - should fix while you're here - -**ā„¹ļø Category 3: Pre-existing Issues** -- Lines in files you reviewed but didn't modify at all -- Legacy vulnerabilities unrelated to this PR -- **Priority:** INFORMATIONAL - fix in separate PR - -### Step 3: Security Analysis - -Scan for these vulnerability patterns: - -**Input Validation & Injection:** -- SQL injection (string concatenation in queries) -- NoSQL injection (unsanitized object properties) -- Command injection (shell command construction) -- XSS vulnerabilities (unescaped output) -- Path traversal (user-controlled file paths) - -**Authentication & Authorization:** -- Weak password policies -- Session management flaws -- JWT token issues (weak secrets, no expiration) -- Missing authentication checks -- Privilege escalation paths - -**Cryptography & Secrets:** -- Hardcoded secrets, API keys, passwords -- Weak encryption algorithms (MD5, SHA1 for passwords) -- Insecure random number generation -- Exposed private keys - -**Configuration & Headers:** -- Missing security headers (CSP, HSTS, X-Frame-Options) -- CORS misconfigurations (overly permissive origins) -- Exposed debugging information -- Insecure defaults - -**Business Logic:** -- Race conditions -- State manipulation -- Price/quantity manipulation -- Workflow bypasses - -### Step 4: Generate Report - -Create a three-section report: - -```markdown -# Security Audit Report - -**Branch**: ${CURRENT_BRANCH} -**Base**: ${BASE_BRANCH} -**Date**: $(date +%Y-%m-%d %H:%M:%S) -**Files Analyzed**: ${FILE_COUNT} -**Lines Changed**: ${LINES_CHANGED} - ---- - -## šŸ”“ Issues in Your Changes (BLOCKING) - -These vulnerabilities were introduced in lines you added or modified: - -### CRITICAL - -**[Issue Title]** - `file.ts:123` (line ADDED in this branch) -- **Vulnerability**: SQL injection in new login query -- **Attack Scenario**: Attacker can input `' OR '1'='1` to bypass authentication -- **Code**: - ```typescript - const query = "SELECT * FROM users WHERE email = '" + email + "'"; - ``` -- **Fix**: Use parameterized queries - ```typescript - const query = "SELECT * FROM users WHERE email = ?"; - db.execute(query, [email]); - ``` -- **Standard**: OWASP A03:2021 - Injection - -### HIGH - -{More findings in lines you changed} - ---- - -## āš ļø Issues in Code You Touched (Should Fix) - -These vulnerabilities exist in code you modified or functions you updated: - -### HIGH - -**[Issue Title]** - `file.ts:89` (in function you modified) -- **Vulnerability**: Missing rate limiting on endpoint -- **Context**: You modified this endpoint but didn't add rate limiting -- **Recommendation**: Add rate limiting middleware while you're here - ```typescript - app.post('/login', rateLimit({ max: 5, window: '15m' }), loginHandler); - ``` - -{More findings in touched code} - ---- - -## ā„¹ļø Pre-existing Issues Found (Not Blocking) - -These vulnerabilities exist in files you reviewed but are unrelated to your changes: - -### MEDIUM - -**[Issue Title]** - `file.ts:456` (pre-existing, line not changed) -- **Vulnerability**: Weak password validation -- **Recommendation**: Consider fixing in a separate PR -- **Reason not blocking**: This existed before your changes and isn't related to this PR's scope - -{More pre-existing findings} - ---- - -## Summary - -**Your Changes:** -- šŸ”“ CRITICAL: 1 (MUST FIX) -- šŸ”“ HIGH: 2 (MUST FIX) -- šŸ”“ MEDIUM: 0 - -**Code You Touched:** -- āš ļø HIGH: 1 (SHOULD FIX) -- āš ļø MEDIUM: 2 (SHOULD FIX) - -**Pre-existing:** -- ā„¹ļø MEDIUM: 3 (OPTIONAL) -- ā„¹ļø LOW: 5 (OPTIONAL) - -**Security Score**: {X}/10 - -**Merge Recommendation**: -- āŒ BLOCK MERGE (if critical issues in your changes) -- āš ļø REVIEW REQUIRED (if high issues in your changes) -- āœ… APPROVED WITH CONDITIONS (if only touched/pre-existing issues) -- āœ… APPROVED (if no issues in your changes) - ---- - -## Remediation Priority - -**Fix before merge:** -1. {Critical issue in your changes} -2. {High issue in your changes} - -**Fix while you're here:** -1. {Issue in code you touched} - -**Future work:** -- Create issues for pre-existing problems -- Track technical debt separately -``` - -### Step 5: Create PR Line Comments - -**If PR_NUMBER is provided**, create line-specific comments for issues in the diff: - -```bash -# Get repo info -REPO=$(gh repo view --json nameWithOwner -q '.nameWithOwner') -COMMIT_SHA=$(git rev-parse HEAD) - -# Function to create PR line comment -create_line_comment() { - local FILE="$1" - local LINE="$2" - local BODY="$3" - - # Check if line is in the PR diff - if gh pr diff "$PR_NUMBER" --name-only | grep -q "^${FILE}$"; then - gh api "repos/${REPO}/pulls/${PR_NUMBER}/comments" \ - -f body="$BODY" \ - -f commit_id="$COMMIT_SHA" \ - -f path="$FILE" \ - -f line="$LINE" \ - -f side="RIGHT" 2>/dev/null && echo "āœ… Comment: $FILE:$LINE" || echo "āš ļø Skipped (line not in diff): $FILE:$LINE" - else - echo "āš ļø Skipped (file not in diff): $FILE:$LINE" - fi - - # Rate limiting - sleep 1 -} -``` - -**Comment format for issues:** - -```markdown -**šŸ”“ Security: {Issue Title}** - -{Brief description of the vulnerability} - -**Suggested Fix:** -```{language} -{code fix} -``` - -**Why:** {Explanation} - ---- -*Severity: {CRITICAL/HIGH/MEDIUM} | Standard: {OWASP reference}* -šŸ¤– [Claude Code](https://claude.com/code) `/review` -``` - -**For each šŸ”“ BLOCKING issue found:** -1. Create line comment if file:line is in PR diff -2. Track as "skipped" if not in diff (will go to summary) - -```bash -COMMENTS_CREATED=0 -COMMENTS_SKIPPED=0 - -# For each blocking issue -for issue in blocking_issues; do - if create_line_comment "$FILE" "$LINE" "$COMMENT_BODY"; then - COMMENTS_CREATED=$((COMMENTS_CREATED + 1)) - else - COMMENTS_SKIPPED=$((COMMENTS_SKIPPED + 1)) - fi -done - -echo "Created: $COMMENTS_CREATED comments, Skipped: $COMMENTS_SKIPPED" -``` - -### Step 6: Save Report - -Save summary to standardized location: - -```bash -# When invoked by /review -REPORT_FILE="${AUDIT_BASE_DIR}/security-report.${TIMESTAMP}.md" - -# When invoked standalone -REPORT_FILE="${REPORT_FILE:-.docs/reviews/standalone/security-report.$(date +%Y-%m-%d_%H%M).md}" - -# Ensure directory exists -mkdir -p "$(dirname "$REPORT_FILE")" - -# Save report (include comment stats) -cat > "$REPORT_FILE" <<'EOF' -{Generated report content} - ---- - -## PR Comment Summary - -- **Comments Created**: ${COMMENTS_CREATED} -- **Comments Skipped**: ${COMMENTS_SKIPPED} (lines not in PR diff) -EOF - -echo "āœ… Security review saved: $REPORT_FILE" -``` - -## Severity Guidelines - -**CRITICAL** - Immediate exploitation possible: -- SQL injection in authentication -- Remote code execution -- Hardcoded admin credentials -- Authentication bypass - -**HIGH** - Significant security risk: -- XSS vulnerabilities -- Broken access control -- Weak cryptography -- Session fixation - -**MEDIUM** - Moderate risk with conditions: -- Missing security headers -- Insecure defaults -- Information disclosure -- Missing rate limiting - -**LOW** - Minor security improvement: -- Outdated dependencies (no known CVE) -- Verbose error messages -- Missing security logging - -## Key Principles - -1. **Focus on changed lines first** - Developer introduced these -2. **Context matters** - Issues near changes should be fixed together -3. **Be fair** - Don't block PRs for legacy code -4. **Be specific** - Exact file:line, attack scenario, fix -5. **Be actionable** - Clear remediation steps diff --git a/agents/review-summary.md b/agents/review-summary.md index d708c18b..7e7133f9 100644 --- a/agents/review-summary.md +++ b/agents/review-summary.md @@ -148,7 +148,7 @@ Create `${REVIEW_BASE_DIR}/review-summary.${TIMESTAMP}.md`: {For each blocking issue:} ### [{SEVERITY}] {Issue Title} -**Review**: {SecurityReview|PerformanceReview|etc.} +**Review**: {Reviewer (security)|Reviewer (performance)|etc.} **Location**: `{file}:{line}` {Description of the issue} diff --git a/agents/review-tests.md b/agents/review-tests.md deleted file mode 100644 index 3855ed5c..00000000 --- a/agents/review-tests.md +++ /dev/null @@ -1,160 +0,0 @@ ---- -name: TestsReview -description: Test quality, coverage, and effectiveness analysis specialist -model: inherit -skills: devflow-review-methodology, devflow-test-design ---- - -You are a tests review specialist focused on test quality, coverage, and effectiveness analysis. - -## Your Task - -Analyze code changes in the current branch for tests issues, with laser focus on lines that were actually modified. - -### Step 1: Identify Changed Lines - -```bash -BASE_BRANCH="" -for branch in main master develop; do - if git show-ref --verify --quiet refs/heads/$branch; then - BASE_BRANCH=$branch; break - fi -done -git diff --name-only $BASE_BRANCH...HEAD > /tmp/changed_files.txt -git diff $BASE_BRANCH...HEAD > /tmp/full_diff.txt -git diff $BASE_BRANCH...HEAD --unified=0 | grep -E '^@@' > /tmp/changed_lines.txt -``` - -### Step 2: Analyze in Three Categories - -**šŸ”“ Category 1: Issues in Your Changes (BLOCKING)** -- Lines ADDED or MODIFIED in this branch -- NEW issues introduced by this PR -- **Priority:** BLOCKING - must fix before merge - -**āš ļø Category 2: Issues in Code You Touched (Should Fix)** -- Lines in functions/modules you modified -- Issues near your changes -- **Priority:** HIGH - should fix while you're here - -**ā„¹ļø Category 3: Pre-existing Issues (Not Blocking)** -- Issues in files you reviewed but didn't modify -- Legacy problems unrelated to this PR -- **Priority:** INFORMATIONAL - fix in separate PR - -### Step 3: Tests Analysis - - -**Test Coverage:** -- Untested new code -- Missing edge cases -- No error path tests -- Low branch coverage - -**Test Quality:** -- Brittle tests -- Unclear test names -- No arrange-act-assert -- Testing implementation not behavior - -**Test Design:** -- Slow tests -- Flaky tests -- Hard to maintain -- Poor assertions - -### Step 4: Generate Report - -```markdown -# Tests Audit Report - -**Branch**: ${CURRENT_BRANCH} -**Base**: ${BASE_BRANCH} -**Date**: $(date +%Y-%m-%d %H:%M:%S) - ---- - -## šŸ”“ Issues in Your Changes (BLOCKING) - -{Issues introduced in lines you added or modified} - ---- - -## āš ļø Issues in Code You Touched (Should Fix) - -{Issues in code you modified or functions you updated} - ---- - -## ā„¹ļø Pre-existing Issues (Not Blocking) - -{Issues in files you reviewed but didn't modify} - ---- - -## Summary - -**Your Changes:** -- šŸ”“ CRITICAL/HIGH/MEDIUM counts - -**Code You Touched:** -- āš ļø HIGH/MEDIUM counts - -**Pre-existing:** -- ā„¹ļø MEDIUM/LOW counts - -**Tests Score**: {X}/10 - -**Merge Recommendation**: -- āŒ BLOCK (if critical issues in your changes) -- āš ļø REVIEW REQUIRED (if high issues) -- āœ… APPROVED WITH CONDITIONS -- āœ… APPROVED -``` - -### Step 5: Create PR Line Comments - -**If PR_NUMBER is provided**, create line-specific comments for šŸ”“ blocking issues: - -```bash -REPO=$(gh repo view --json nameWithOwner -q '.nameWithOwner') -COMMIT_SHA=$(git rev-parse HEAD) -COMMENTS_CREATED=0 -COMMENTS_SKIPPED=0 - -create_pr_comment() { - local FILE="$1" LINE="$2" BODY="$3" - if gh pr diff "$PR_NUMBER" --name-only 2>/dev/null | grep -q "^${FILE}$"; then - gh api "repos/${REPO}/pulls/${PR_NUMBER}/comments" \ - -f body="$BODY" -f commit_id="$COMMIT_SHA" \ - -f path="$FILE" -f line="$LINE" -f side="RIGHT" 2>/dev/null \ - && COMMENTS_CREATED=$((COMMENTS_CREATED + 1)) \ - || COMMENTS_SKIPPED=$((COMMENTS_SKIPPED + 1)) - else - COMMENTS_SKIPPED=$((COMMENTS_SKIPPED + 1)) - fi - sleep 1 -} -``` - -### Step 6: Save Report - -```bash -REPORT_FILE="${AUDIT_BASE_DIR}/tests-report.${TIMESTAMP}.md" -mkdir -p "$(dirname "$REPORT_FILE")" -cat > "$REPORT_FILE" <<'REPORT' -{Generated report content} - ---- -## PR Comments: ${COMMENTS_CREATED} created, ${COMMENTS_SKIPPED} skipped -REPORT -echo "āœ… Tests review saved: $REPORT_FILE" -``` - -## Key Principles - -1. **Focus on changed lines first** - Developer introduced these -2. **Context matters** - Issues near changes should be fixed together -3. **Be fair** - Don't block PRs for legacy code -4. **Be specific** - Exact file:line with examples -5. **Be actionable** - Clear fixes diff --git a/agents/review-typescript.md b/agents/review-typescript.md deleted file mode 100644 index 4fce7ff0..00000000 --- a/agents/review-typescript.md +++ /dev/null @@ -1,160 +0,0 @@ ---- -name: TypescriptReview -description: TypeScript code quality and type safety enforcement specialist -model: inherit -skills: devflow-review-methodology, devflow-core-patterns ---- - -You are a typescript review specialist focused on typescript code quality and type safety enforcement. - -## Your Task - -Analyze code changes in the current branch for typescript issues, with laser focus on lines that were actually modified. - -### Step 1: Identify Changed Lines - -```bash -BASE_BRANCH="" -for branch in main master develop; do - if git show-ref --verify --quiet refs/heads/$branch; then - BASE_BRANCH=$branch; break - fi -done -git diff --name-only $BASE_BRANCH...HEAD > /tmp/changed_files.txt -git diff $BASE_BRANCH...HEAD > /tmp/full_diff.txt -git diff $BASE_BRANCH...HEAD --unified=0 | grep -E '^@@' > /tmp/changed_lines.txt -``` - -### Step 2: Analyze in Three Categories - -**šŸ”“ Category 1: Issues in Your Changes (BLOCKING)** -- Lines ADDED or MODIFIED in this branch -- NEW issues introduced by this PR -- **Priority:** BLOCKING - must fix before merge - -**āš ļø Category 2: Issues in Code You Touched (Should Fix)** -- Lines in functions/modules you modified -- Issues near your changes -- **Priority:** HIGH - should fix while you're here - -**ā„¹ļø Category 3: Pre-existing Issues (Not Blocking)** -- Issues in files you reviewed but didn't modify -- Legacy problems unrelated to this PR -- **Priority:** INFORMATIONAL - fix in separate PR - -### Step 3: Typescript Analysis - - -**Type Safety:** -- Any types usage -- Type assertions without validation -- Missing generic constraints -- Implicit any - -**TypeScript Best Practices:** -- Enum vs union types -- Interface vs type alias -- Strict mode violations -- Non-null assertions - -**Type Quality:** -- Overly broad types -- Missing return types -- Incomplete type definitions -- Type pollution - -### Step 4: Generate Report - -```markdown -# Typescript Audit Report - -**Branch**: ${CURRENT_BRANCH} -**Base**: ${BASE_BRANCH} -**Date**: $(date +%Y-%m-%d %H:%M:%S) - ---- - -## šŸ”“ Issues in Your Changes (BLOCKING) - -{Issues introduced in lines you added or modified} - ---- - -## āš ļø Issues in Code You Touched (Should Fix) - -{Issues in code you modified or functions you updated} - ---- - -## ā„¹ļø Pre-existing Issues (Not Blocking) - -{Issues in files you reviewed but didn't modify} - ---- - -## Summary - -**Your Changes:** -- šŸ”“ CRITICAL/HIGH/MEDIUM counts - -**Code You Touched:** -- āš ļø HIGH/MEDIUM counts - -**Pre-existing:** -- ā„¹ļø MEDIUM/LOW counts - -**Typescript Score**: {X}/10 - -**Merge Recommendation**: -- āŒ BLOCK (if critical issues in your changes) -- āš ļø REVIEW REQUIRED (if high issues) -- āœ… APPROVED WITH CONDITIONS -- āœ… APPROVED -``` - -### Step 5: Create PR Line Comments - -**If PR_NUMBER is provided**, create line-specific comments for šŸ”“ blocking issues: - -```bash -REPO=$(gh repo view --json nameWithOwner -q '.nameWithOwner') -COMMIT_SHA=$(git rev-parse HEAD) -COMMENTS_CREATED=0 -COMMENTS_SKIPPED=0 - -create_pr_comment() { - local FILE="$1" LINE="$2" BODY="$3" - if gh pr diff "$PR_NUMBER" --name-only 2>/dev/null | grep -q "^${FILE}$"; then - gh api "repos/${REPO}/pulls/${PR_NUMBER}/comments" \ - -f body="$BODY" -f commit_id="$COMMIT_SHA" \ - -f path="$FILE" -f line="$LINE" -f side="RIGHT" 2>/dev/null \ - && COMMENTS_CREATED=$((COMMENTS_CREATED + 1)) \ - || COMMENTS_SKIPPED=$((COMMENTS_SKIPPED + 1)) - else - COMMENTS_SKIPPED=$((COMMENTS_SKIPPED + 1)) - fi - sleep 1 -} -``` - -### Step 6: Save Report - -```bash -REPORT_FILE="${AUDIT_BASE_DIR}/typescript-report.${TIMESTAMP}.md" -mkdir -p "$(dirname "$REPORT_FILE")" -cat > "$REPORT_FILE" <<'REPORT' -{Generated report content} - ---- -## PR Comments: ${COMMENTS_CREATED} created, ${COMMENTS_SKIPPED} skipped -REPORT -echo "āœ… Typescript review saved: $REPORT_FILE" -``` - -## Key Principles - -1. **Focus on changed lines first** - Developer introduced these -2. **Context matters** - Issues near changes should be fixed together -3. **Be fair** - Don't block PRs for legacy code -4. **Be specific** - Exact file:line with examples -5. **Be actionable** - Clear fixes diff --git a/agents/reviewer.md b/agents/reviewer.md new file mode 100644 index 00000000..4d75ea63 --- /dev/null +++ b/agents/reviewer.md @@ -0,0 +1,257 @@ +--- +name: Reviewer +description: Universal code review agent with parameterized focus. Applies specialized pattern skills based on focus area. Outputs findings to .docs/reviews/{branch}/{focus}.md. +model: sonnet +skills: devflow-review-methodology, devflow-security-patterns, devflow-architecture-patterns, devflow-performance-patterns, devflow-complexity-patterns, devflow-consistency-patterns, devflow-tests-patterns, devflow-database-patterns, devflow-documentation-patterns, devflow-dependencies-patterns, devflow-regression-patterns +--- + +# Reviewer Agent - Universal Code Review + +You are a universal code review agent. Your focus area is specified in the prompt that invokes you. + +**Skills loaded:** +- `devflow-review-methodology`: 6-step review process, 3-category issue classification +- All pattern skills for domain expertise (security, architecture, performance, etc.) + +## Focus Areas + +You will be invoked with a specific focus area. Apply the corresponding pattern skill: + +| Focus | Pattern Skill | What to Look For | +|-------|---------------|------------------| +| `security` | `devflow-security-patterns` | Injection, auth, crypto, config vulnerabilities | +| `architecture` | `devflow-architecture-patterns` | SOLID violations, coupling, layering, modularity | +| `performance` | `devflow-performance-patterns` | Algorithms, N+1, memory, I/O, caching | +| `complexity` | `devflow-complexity-patterns` | Cyclomatic complexity, readability, maintainability | +| `consistency` | `devflow-consistency-patterns` | Pattern violations, simplification, truncation | +| `tests` | `devflow-tests-patterns` | Coverage, quality, brittle tests, mocking | +| `database` | `devflow-database-patterns` | Schema, queries, migrations, indexes | +| `documentation` | `devflow-documentation-patterns` | Docs quality, alignment, code-comment drift | +| `dependencies` | `devflow-dependencies-patterns` | CVEs, versions, licenses, supply chain | +| `regression` | `devflow-regression-patterns` | Lost functionality, broken behavior, migrations | + +## Input + +Your prompt will specify: +1. **Focus area**: Which review type to perform +2. **Branch/PR context**: What changes to review +3. **Output path**: Where to save findings + +Example invocation prompt: +``` +Review this code focusing exclusively on SECURITY. +Apply patterns from devflow-security-patterns skill. +Follow the 6-step process from devflow-review-methodology. +Output findings to .docs/reviews/feature-auth/security.md +``` + +## Review Process + +Follow the 6-step process from `devflow-review-methodology`: + +### Step 1: Identify Changed Lines + +```bash +# Determine base branch +BASE_BRANCH="" +for branch in main master develop; do + if git show-ref --verify --quiet refs/heads/$branch; then + BASE_BRANCH=$branch; break + fi +done + +# Get changed files and lines +git diff --name-only $BASE_BRANCH...HEAD > /tmp/changed_files.txt +git diff $BASE_BRANCH...HEAD > /tmp/full_diff.txt +git diff $BASE_BRANCH...HEAD --unified=0 | grep -E '^@@' > /tmp/changed_lines.txt +``` + +### Step 2: Categorize Issues + +Apply the 3-category classification: + +**Category 1: Issues in Your Changes (BLOCKING)** +- Lines ADDED or MODIFIED in this branch +- NEW issues introduced by this PR +- Priority: BLOCKING - must fix before merge + +**Category 2: Issues in Code You Touched (Should Fix)** +- Lines in functions/modules you modified +- Issues near your changes +- Priority: HIGH - should fix while you're here + +**Category 3: Pre-existing Issues (Not Blocking)** +- Issues in files you reviewed but didn't modify +- Legacy problems unrelated to this PR +- Priority: INFORMATIONAL - fix in separate PR + +### Step 3: Apply Focus-Specific Analysis + +Based on your assigned focus, apply the corresponding pattern skill's detection patterns and checklists. + +**Example for security focus:** +- Check for SQL injection patterns +- Verify input validation at boundaries +- Look for hardcoded secrets +- Verify authentication on endpoints + +### Step 4: Generate Report + +Create the report in the specified output path: + +```markdown +# {Focus} Review Report + +**Branch**: {current_branch} +**Base**: {base_branch} +**Date**: {timestamp} +**Focus**: {focus_area} + +--- + +## Issues in Your Changes (BLOCKING) + +{Issues introduced in lines you added or modified, with file:line references} + +### CRITICAL + +**[Issue Title]** - `file.ts:123` +- **Problem**: {description} +- **Evidence**: {code snippet} +- **Fix**: {suggested fix with code} +- **Category**: {pattern category from skill} + +### HIGH + +{More issues...} + +--- + +## Issues in Code You Touched (Should Fix) + +{Issues in code you modified, with file:line references} + +--- + +## Pre-existing Issues (Not Blocking) + +{Issues in files reviewed but not modified} + +--- + +## Summary + +**Your Changes:** +- CRITICAL: X +- HIGH: Y +- MEDIUM: Z + +**Code You Touched:** +- HIGH: A +- MEDIUM: B + +**Pre-existing:** +- MEDIUM: C +- LOW: D + +**{Focus} Score**: X/10 + +**Merge Recommendation**: +- BLOCK MERGE (if critical issues in your changes) +- REVIEW REQUIRED (if high issues in your changes) +- APPROVED WITH CONDITIONS (if only medium/low) +- APPROVED (if clean) +``` + +### Step 5: Create PR Line Comments (if PR_NUMBER provided) + +For blocking issues, create inline PR comments: + +```bash +REPO=$(gh repo view --json nameWithOwner -q '.nameWithOwner') +COMMIT_SHA=$(git rev-parse HEAD) + +# For each blocking issue with file:line +gh api "repos/${REPO}/pulls/${PR_NUMBER}/comments" \ + -f body="**{Focus}: {Issue}** + +{Description} + +**Suggested Fix:** +\`\`\` +{code} +\`\`\` + +--- +*Severity: {level}* +Claude Code \`/review\`" \ + -f commit_id="$COMMIT_SHA" \ + -f path="$FILE" \ + -f line="$LINE" \ + -f side="RIGHT" +``` + +### Step 6: Save Report + +```bash +BRANCH_SLUG=$(git branch --show-current | sed 's/\//-/g') +REPORT_DIR=".docs/reviews/${BRANCH_SLUG}" +mkdir -p "$REPORT_DIR" + +# Save report to specified path +cat > "${REPORT_DIR}/${FOCUS}.md" <<'EOF' +{report content} +EOF + +echo "Review saved: ${REPORT_DIR}/${FOCUS}.md" +``` + +## Output + +Return a structured summary: + +```markdown +## Reviewer Report: {FOCUS} + +### Status: PASS | ISSUES_FOUND | BLOCKED + +### Findings Summary +- Blocking issues: X +- Should-fix issues: Y +- Informational: Z + +### Top Issues +1. {Most critical issue with file:line} +2. {Second issue} +3. {Third issue} + +### Report Location +`.docs/reviews/{branch}/{focus}.md` + +### Merge Recommendation +{BLOCK | REVIEW_REQUIRED | APPROVED_WITH_CONDITIONS | APPROVED} +``` + +## Key Principles + +1. **Focus on changed lines first** - Developer introduced these +2. **Context matters** - Issues near changes should be fixed together +3. **Be fair** - Don't block PRs for pre-existing issues +4. **Be specific** - Exact file:line with code examples +5. **Be actionable** - Clear, implementable fixes +6. **Apply expertise** - Use the pattern skill for your focus area + +## Conditional Activation + +This agent should be invoked: +- `security`: Always +- `architecture`: Always +- `performance`: Always +- `complexity`: Always +- `consistency`: Always +- `tests`: Always +- `regression`: Always +- `typescript`: If .ts/.tsx files changed +- `database`: If migration/schema files changed +- `documentation`: If docs changed +- `dependencies`: If package.json/lock files changed diff --git a/commands/implement.md b/commands/implement.md index b95b9bb2..551ee9ac 100644 --- a/commands/implement.md +++ b/commands/implement.md @@ -312,75 +312,30 @@ PR_URL=$(gh pr view --json url -q '.url') --- -## Phase 8: Review (Parallel) +## Phase 8: Code Review -Setup: +**Invoke the `/review` command** to run comprehensive review. This ensures review orchestration logic is not duplicated. -```bash -BRANCH_SLUG=$(echo "${TASK_BRANCH}" | sed 's/\//-/g') -REVIEW_TIMESTAMP=$(date +%Y-%m-%d_%H%M) -REVIEW_DIR=".docs/reviews/${BRANCH_SLUG}" -mkdir -p "$REVIEW_DIR" -``` - -Spawn 6 review agents **in a single message**: - -``` -Task(subagent_type="SecurityReview"): -"Review PR #${PR_NUMBER}. Save: ${REVIEW_DIR}/security-report.${REVIEW_TIMESTAMP}.md" - -Task(subagent_type="ArchitectureReview"): -"Review PR #${PR_NUMBER}. Save: ${REVIEW_DIR}/architecture-report.${REVIEW_TIMESTAMP}.md" - -Task(subagent_type="PerformanceReview"): -"Review PR #${PR_NUMBER}. Save: ${REVIEW_DIR}/performance-report.${REVIEW_TIMESTAMP}.md" - -Task(subagent_type="ComplexityReview"): -"Review PR #${PR_NUMBER}. Save: ${REVIEW_DIR}/complexity-report.${REVIEW_TIMESTAMP}.md" - -Task(subagent_type="ConsistencyReview"): -"Review PR #${PR_NUMBER}. Save: ${REVIEW_DIR}/consistency-report.${REVIEW_TIMESTAMP}.md" +The `/review` command will: +1. Spawn Reviewer agents in parallel (7 always + conditional) +2. Each Reviewer focuses on one area (security, architecture, performance, etc.) +3. Create PR inline comments for blocking issues +4. Track pre-existing issues in tech debt backlog +5. Synthesize findings with merge recommendation -Task(subagent_type="TestsReview"): -"Review PR #${PR_NUMBER}. Save: ${REVIEW_DIR}/tests-report.${REVIEW_TIMESTAMP}.md" +```bash +# /review handles: +# - Spawning Reviewer agents with focus areas +# - Comment agent for PR comments +# - TechDebt agent for backlog tracking +# - Summary agent for recommendation ``` -**Conditionally add** (based on changed files): -- `TypescriptReview` - if .ts/.tsx files -- `DatabaseReview` - if SQL/migration files -- `DependenciesReview` - if package.json/requirements.txt +The review outputs are captured from the `/review` command's final report. --- -## Phase 9: Review Synthesis (Parallel) - -**WAIT** for Phase 8, then spawn synthesis agents **in a single message**: - -``` -Task(subagent_type="Comment"): -"Create PR comments for swarm task. -PR_NUMBER: ${PR_NUMBER} -REVIEW_BASE_DIR: ${REVIEW_DIR} -TIMESTAMP: ${REVIEW_TIMESTAMP} -Create inline comments, consolidate skipped into summary" - -Task(subagent_type="TechDebt"): -"Track tech debt for swarm task. -REVIEW_DIR: ${REVIEW_DIR} -TIMESTAMP: ${REVIEW_TIMESTAMP} -Add pre-existing issues to backlog" - -Task(subagent_type="Summary"): -"Synthesize review findings for swarm task. -PR_NUMBER: ${PR_NUMBER} -REVIEW_BASE_DIR: ${REVIEW_DIR} -TIMESTAMP: ${REVIEW_TIMESTAMP} -Generate summary with merge recommendation" -``` - ---- - -## Phase 10: Final Report +## Phase 9: Final Report Display agent outputs: @@ -403,9 +358,8 @@ ${TASK_DESCRIPTION} | Orient | 1 (Skimmer) | āœ… | | Explore | 4 | āœ… | | Plan | 3 | āœ… | -| Implement | {n} ({parallel/sequential}) | āœ… | -| Review | {n} | āœ… | -| Synthesis | 3 | āœ… | +| Implement | {n} ({parallel/sequential}) + self-review | āœ… | +| Review | via /review command | āœ… | --- @@ -500,25 +454,19 @@ git worktree prune ā”‚ ā”œā”€ Phase 6: Implement ā”‚ ā””ā”€ 1-N Coder agents (parallel if beneficial) +ā”‚ ā””ā”€ Each Coder runs self-review via Stop hook (9 pillars) ā”‚ ā”œā”€ Phase 7: Create PR (if parallel coders) ā”‚ ā””ā”€ PullRequest agent ā”‚ -ā”œā”€ Phase 8: Review (PARALLEL) -ā”‚ ā”œā”€ SecurityReview -ā”‚ ā”œā”€ ArchitectureReview -ā”‚ ā”œā”€ PerformanceReview -ā”‚ ā”œā”€ ComplexityReview -ā”‚ ā”œā”€ ConsistencyReview -ā”‚ ā”œā”€ TestsReview -ā”‚ ā””ā”€ (conditional: TypeScript, Database, Dependencies) -ā”‚ -ā”œā”€ Phase 9: Review Synthesis (PARALLEL) -ā”‚ ā”œā”€ Comment agent -ā”‚ ā”œā”€ TechDebt agent -ā”‚ ā””ā”€ Summary agent +ā”œā”€ Phase 8: Code Review +ā”‚ ā””ā”€ Invokes /review command (DRY - no duplication) +ā”‚ ā”œā”€ Reviewer agents (7 focus areas + conditional) +ā”‚ ā”œā”€ Comment agent +ā”‚ ā”œā”€ TechDebt agent +ā”‚ ā””ā”€ Summary agent ā”‚ -ā””ā”€ Phase 10: Display agent outputs +ā””ā”€ Phase 9: Display agent outputs ``` --- diff --git a/commands/review.md b/commands/review.md index 1d9797b0..3f66cac1 100644 --- a/commands/review.md +++ b/commands/review.md @@ -110,25 +110,25 @@ echo "Documentation: $([ -n "$HAS_DOCS" ] && echo 'yes' || echo 'no')" echo "Tests: $([ -n "$HAS_TESTS" ] && echo 'yes' || echo 'no')" ``` -### Determine Audits to Run - -| Audit | Run When | -|-------|----------| -| SecurityReview | Always | -| PerformanceReview | Always | -| ArchitectureReview | Always | -| ComplexityReview | Always | -| ConsistencyReview | Always | -| RegressionReview | Always | -| TestsReview | Always | -| DependenciesReview | Dependencies changed | -| DocumentationReview | Docs or significant code changed | -| TypescriptReview | .ts/.tsx files changed | -| DatabaseReview | db/migration files changed | +### Determine Review Focus Areas + +| Focus | Run When | Pattern Skill Applied | +|-------|----------|----------------------| +| security | Always | devflow-security-patterns | +| architecture | Always | devflow-architecture-patterns | +| performance | Always | devflow-performance-patterns | +| complexity | Always | devflow-complexity-patterns | +| consistency | Always | devflow-consistency-patterns | +| regression | Always | devflow-regression-patterns | +| tests | Always | devflow-tests-patterns | +| dependencies | Dependencies changed | devflow-dependencies-patterns | +| documentation | Docs or significant code changed | devflow-documentation-patterns | +| typescript | .ts/.tsx files changed | devflow-typescript | +| database | db/migration files changed | devflow-database-patterns | --- -## Phase 3: Run Audits (Parallel) +## Phase 3: Run Reviews (Parallel) Setup coordination: @@ -139,25 +139,25 @@ REVIEW_DIR=".docs/reviews/${BRANCH_SLUG}" mkdir -p "$REVIEW_DIR" ``` -**Spawn review agents in parallel** using Task tool. For each review: +**Spawn Reviewer agents in parallel** using Task tool. Each Reviewer is invoked with a specific focus via prompt injection: ``` -Task(subagent_type="{AuditType}Review"): +Task(subagent_type="Reviewer"): -"Analyze branch for {type} issues. Create PR line comments for issues found. +"Review this code focusing exclusively on {FOCUS}. +Apply patterns from devflow-{focus}-patterns skill. +Follow the 6-step process from devflow-review-methodology. PR_NUMBER: ${PR_NUMBER} BASE_BRANCH: ${BASE_BRANCH} REVIEW_BASE_DIR: ${REVIEW_DIR} TIMESTAMP: ${TIMESTAMP} -Save report to: ${REVIEW_DIR}/{type}-report.${TIMESTAMP}.md -Report back: issues found, comments created, comments skipped" +Output findings to: ${REVIEW_DIR}/{focus}.md +Report back: issues found by category (blocking/should-fix/informational), merge recommendation" ``` -**Always run**: SecurityReview, PerformanceReview, ArchitectureReview, ComplexityReview, ConsistencyReview, RegressionReview, TestsReview (7 agents) - -**Conditionally run**: DependenciesReview, DocumentationReview, TypescriptReview, DatabaseReview +Spawn one Reviewer per focus area from the table above. Always run the 7 "Always" focus areas; conditionally run the others based on changed file types. --- diff --git a/skills/devflow-architecture-patterns/SKILL.md b/skills/devflow-architecture-patterns/SKILL.md new file mode 100644 index 00000000..a68bd09b --- /dev/null +++ b/skills/devflow-architecture-patterns/SKILL.md @@ -0,0 +1,440 @@ +--- +name: devflow-architecture-patterns +description: Software architecture and design pattern analysis. Load when reviewing code structure, SOLID violations, coupling issues, or module boundaries. Used by Reviewer agent with architecture focus. +allowed-tools: Read, Grep, Glob +--- + +# Architecture Patterns + +Domain expertise for software architecture and design pattern analysis. Use alongside `devflow-review-methodology` for complete architecture reviews. + +## Iron Law + +> **SEPARATION OF CONCERNS IS NON-NEGOTIABLE** +> +> Every module has one reason to change. Every layer has clear boundaries. Every dependency +> points in one direction. Violations compound into unmaintainable systems. A shortcut today +> becomes technical debt tomorrow. There are no exceptions. + +## Architecture Categories + +### 1. SOLID Violations + +**Single Responsibility Principle (SRP)** +```typescript +// VIOLATION: Class handles HTTP, validation, DB, and email +class UserController { + async createUser(req, res) { + // Validates input + if (!req.body.email.includes('@')) throw new Error('Invalid email'); + + // Hashes password + const hash = await bcrypt.hash(req.body.password, 12); + + // Saves to database + const user = await db.users.create({ ...req.body, password: hash }); + + // Sends welcome email + await sendEmail(user.email, 'Welcome!', template); + + res.json(user); + } +} + +// CORRECT: Separate concerns +class UserController { + constructor( + private userService: UserService, + private validator: UserValidator + ) {} + + async createUser(req, res) { + const data = this.validator.validateCreateUser(req.body); + const user = await this.userService.create(data); + res.json(user); + } +} +``` + +**Open/Closed Principle (OCP)** +```typescript +// VIOLATION: Modify existing code to add new types +function calculateDiscount(type: string, amount: number) { + if (type === 'regular') return amount * 0.1; + if (type === 'premium') return amount * 0.2; + if (type === 'vip') return amount * 0.3; // Adding new type = modifying + return 0; +} + +// CORRECT: Extend without modifying +interface DiscountStrategy { + calculate(amount: number): number; +} + +class RegularDiscount implements DiscountStrategy { + calculate(amount: number) { return amount * 0.1; } +} + +class PremiumDiscount implements DiscountStrategy { + calculate(amount: number) { return amount * 0.2; } +} + +// Adding VIP = new class, no modification to existing code +``` + +**Liskov Substitution Principle (LSP)** +```typescript +// VIOLATION: Subclass breaks parent contract +class Rectangle { + setWidth(w: number) { this.width = w; } + setHeight(h: number) { this.height = h; } + area() { return this.width * this.height; } +} + +class Square extends Rectangle { + setWidth(w: number) { + this.width = w; + this.height = w; // Breaks expectation! + } +} + +// CORRECT: Use composition or proper abstraction +interface Shape { + area(): number; +} + +class Rectangle implements Shape { /* ... */ } +class Square implements Shape { /* ... */ } +``` + +**Interface Segregation Principle (ISP)** +```typescript +// VIOLATION: Fat interface forces unnecessary implementations +interface Worker { + work(): void; + eat(): void; + sleep(): void; +} + +class Robot implements Worker { + work() { /* OK */ } + eat() { throw new Error('Robots do not eat'); } // Forced to implement + sleep() { throw new Error('Robots do not sleep'); } +} + +// CORRECT: Segregated interfaces +interface Workable { work(): void; } +interface Eatable { eat(): void; } +interface Sleepable { sleep(): void; } + +class Robot implements Workable { + work() { /* OK */ } +} +``` + +**Dependency Inversion Principle (DIP)** +```typescript +// VIOLATION: High-level depends on low-level +class OrderService { + private db = new PostgresDatabase(); // Concrete dependency + + async createOrder(data: OrderData) { + return this.db.insert('orders', data); + } +} + +// CORRECT: Depend on abstractions +interface OrderRepository { + create(data: OrderData): Promise; +} + +class OrderService { + constructor(private repository: OrderRepository) {} + + async createOrder(data: OrderData) { + return this.repository.create(data); + } +} +``` + +### 2. Coupling Issues + +**Tight Coupling** +```typescript +// VIOLATION: Direct instantiation creates coupling +class ReportGenerator { + generate() { + const data = new DatabaseService().query('SELECT...'); + const formatted = new FormattingService().format(data); + new EmailService().send('admin@example.com', formatted); + } +} + +// CORRECT: Inject dependencies +class ReportGenerator { + constructor( + private dataSource: DataSource, + private formatter: Formatter, + private notifier: Notifier + ) {} + + generate() { + const data = this.dataSource.fetch(); + const formatted = this.formatter.format(data); + this.notifier.notify(formatted); + } +} +``` + +**Circular Dependencies** +```typescript +// VIOLATION: A imports B, B imports A +// user.service.ts +import { OrderService } from './order.service'; +class UserService { + constructor(private orders: OrderService) {} +} + +// order.service.ts +import { UserService } from './user.service'; +class OrderService { + constructor(private users: UserService) {} +} + +// CORRECT: Extract shared concern or use events +// user-order.events.ts +interface UserOrderEvents { + onOrderCreated(userId: string, orderId: string): void; +} + +// Or extract to third module both depend on +``` + +**Feature Envy** +```typescript +// VIOLATION: Method uses another class's data excessively +class OrderPrinter { + print(order: Order) { + console.log(`Customer: ${order.customer.name}`); + console.log(`Address: ${order.customer.address.street}, ${order.customer.address.city}`); + console.log(`Phone: ${order.customer.phone}`); + console.log(`Items: ${order.items.length}`); + // Most access is to order.customer, not order + } +} + +// CORRECT: Move behavior to the class with the data +class Customer { + formatForPrint(): string { + return `${this.name}\n${this.address.format()}\n${this.phone}`; + } +} +``` + +### 3. Layering Violations + +**Skipping Layers** +```typescript +// VIOLATION: Controller directly accesses database +class UserController { + async getUser(req, res) { + const user = await db.query('SELECT * FROM users WHERE id = ?', [req.params.id]); + res.json(user); + } +} + +// CORRECT: Proper layering +// Controller -> Service -> Repository -> Database +class UserController { + constructor(private userService: UserService) {} + + async getUser(req, res) { + const user = await this.userService.findById(req.params.id); + res.json(user); + } +} +``` + +**Leaky Abstractions** +```typescript +// VIOLATION: Domain leaks infrastructure details +interface User { + id: string; + name: string; + _mongoId: ObjectId; // Infrastructure leak! + __v: number; // Infrastructure leak! +} + +// CORRECT: Clean domain model +interface User { + id: string; + name: string; + email: string; +} + +// Map in repository layer +class MongoUserRepository { + toDomain(doc: MongoDocument): User { + return { id: doc._id.toString(), name: doc.name, email: doc.email }; + } +} +``` + +**Wrong Direction Dependencies** +```typescript +// VIOLATION: Domain depends on infrastructure +// domain/user.ts +import { PostgresClient } from '../infrastructure/postgres'; + +class User { + save() { + PostgresClient.insert(this); // Domain knows about DB! + } +} + +// CORRECT: Infrastructure depends on domain +// domain/user.ts +interface UserRepository { + save(user: User): Promise; +} + +// infrastructure/postgres-user-repository.ts +import { User, UserRepository } from '../domain/user'; + +class PostgresUserRepository implements UserRepository { + async save(user: User) { /* ... */ } +} +``` + +### 4. Modularity Issues + +**God Class** +```typescript +// VIOLATION: Class does everything +class ApplicationManager { + // User management + createUser() {} + deleteUser() {} + updateUser() {} + + // Order management + createOrder() {} + cancelOrder() {} + shipOrder() {} + + // Reporting + generateReport() {} + exportToPdf() {} + + // Email + sendWelcomeEmail() {} + sendOrderConfirmation() {} + + // 1000+ more lines... +} + +// CORRECT: Separate bounded contexts +class UserService { /* user operations */ } +class OrderService { /* order operations */ } +class ReportService { /* reporting */ } +class EmailService { /* email sending */ } +``` + +**Inappropriate Intimacy** +```typescript +// VIOLATION: Class knows too much about another's internals +class Order { + calculateTotal() { + let total = 0; + for (const item of this.items) { + // Reaches deep into Product internals + total += item.product._pricing._basePrice * + item.product._pricing._taxRate * + (1 - item.product._pricing._discountPercent); + } + return total; + } +} + +// CORRECT: Ask, don't tell +class Product { + getPrice(): number { + return this.pricing.calculate(); + } +} + +class Order { + calculateTotal() { + return this.items.reduce((sum, item) => sum + item.product.getPrice(), 0); + } +} +``` + +--- + +## Severity Guidelines + +**CRITICAL** - Fundamental architecture broken: +- Circular dependencies between modules +- Domain layer depends on infrastructure +- No separation between HTTP and business logic +- God classes with 1000+ lines + +**HIGH** - Significant architecture issues: +- SOLID violations in core business logic +- Tight coupling between services +- Leaky abstractions exposing infrastructure +- Feature envy across module boundaries + +**MEDIUM** - Moderate architecture concerns: +- Some dependencies not injected +- Minor layering violations +- Inconsistent abstraction levels +- Missing interfaces for testability + +**LOW** - Minor architecture improvements: +- Naming doesn't reflect domain +- Minor code organization issues +- Could benefit from more abstraction +- Documentation missing for architecture decisions + +--- + +## Detection Patterns + +Search for these patterns in code: + +```bash +# God classes (large files) +find . -name "*.ts" -exec wc -l {} \; | sort -rn | head -20 + +# Direct database access in controllers +grep -rn "db\.\|prisma\.\|mongoose\." --include="*controller*.ts" + +# Circular imports (look for patterns) +grep -rn "import.*from.*service" --include="*service*.ts" | grep -v node_modules + +# Concrete instantiation (new keyword in services) +grep -rn "new [A-Z].*Service\|new [A-Z].*Repository" --include="*.ts" + +# Infrastructure in domain +grep -rn "import.*postgres\|import.*mongo\|import.*redis" --include="*/domain/*.ts" + +# Missing dependency injection +grep -rn "class.*{" -A5 --include="*.ts" | grep -B5 "private.*= new" +``` + +--- + +## Architecture Principles Reference + +| Principle | Violation Sign | Fix Pattern | +|-----------|----------------|-------------| +| SRP | Class has multiple reasons to change | Extract classes | +| OCP | Adding feature requires modifying existing code | Use strategy/plugin pattern | +| LSP | Subclass throws "not supported" | Use composition over inheritance | +| ISP | Implementing unused interface methods | Split interfaces | +| DIP | `new ConcreteClass()` in business logic | Inject via constructor | +| DRY | Copy-paste code blocks | Extract to shared module | +| YAGNI | Premature abstractions | Remove until needed | + diff --git a/skills/devflow-complexity-patterns/SKILL.md b/skills/devflow-complexity-patterns/SKILL.md new file mode 100644 index 00000000..533a4610 --- /dev/null +++ b/skills/devflow-complexity-patterns/SKILL.md @@ -0,0 +1,385 @@ +--- +name: devflow-complexity-patterns +description: Code complexity and maintainability analysis. Load when reviewing code for cyclomatic complexity, readability issues, or maintainability concerns. Used by Reviewer agent with complexity focus. +allowed-tools: Read, Grep, Glob +--- + +# Complexity Patterns + +Domain expertise for code complexity and maintainability analysis. Use alongside `devflow-review-methodology` for complete complexity reviews. + +## Iron Law + +> **IF YOU CAN'T UNDERSTAND IT IN 5 MINUTES, IT'S TOO COMPLEX** +> +> Every function should be explainable to a colleague in under 5 minutes. If you need a +> diagram to understand control flow, refactor. If you need comments to explain what +> (not why), the code is too clever. Simplicity is a feature, complexity is a bug. + +## Complexity Categories + +### 1. Cyclomatic Complexity + +**Deep Nesting** +```typescript +// PROBLEM: 5+ levels of nesting +function processOrder(order: Order) { + if (order) { + if (order.items) { + for (const item of order.items) { + if (item.quantity > 0) { + if (item.product) { + if (item.product.inStock) { + // Finally the actual logic... + } + } + } + } + } + } +} + +// SOLUTION: Early returns and extraction +function processOrder(order: Order) { + if (!order?.items) return; + + for (const item of order.items) { + processItem(item); + } +} + +function processItem(item: OrderItem) { + if (item.quantity <= 0) return; + if (!item.product?.inStock) return; + + // Actual logic at top level +} +``` + +**Long Functions (>50 lines)** +```typescript +// PROBLEM: Function does too many things +function handleCheckout(cart: Cart, user: User) { + // 150 lines of validation, pricing, inventory, + // payment, notifications, analytics... +} + +// SOLUTION: Extract logical units +async function handleCheckout(cart: Cart, user: User) { + const validated = validateCart(cart); + const priced = calculatePricing(validated); + const reserved = await reserveInventory(priced); + const payment = await processPayment(reserved, user); + await sendNotifications(payment); + trackAnalytics(payment); + return payment; +} +``` + +**High Cyclomatic Complexity (>10)** +```typescript +// PROBLEM: Too many decision paths +function categorize(item: Item): string { + if (item.type === 'A') { + if (item.size > 10) { + if (item.color === 'red') return 'A-large-red'; + else if (item.color === 'blue') return 'A-large-blue'; + else return 'A-large-other'; + } else { + if (item.color === 'red') return 'A-small-red'; + // ... many more branches + } + } else if (item.type === 'B') { + // ... another tree of conditions + } +} + +// SOLUTION: Data-driven or strategy pattern +const categoryRules: Record string> = { + 'A': categorizeTypeA, + 'B': categorizeTypeB, +}; + +function categorize(item: Item): string { + const handler = categoryRules[item.type]; + return handler ? handler(item) : 'unknown'; +} +``` + +### 2. Readability Issues + +**Magic Numbers/Strings** +```typescript +// PROBLEM: Unexplained literals +if (status === 3) { + setTimeout(callback, 86400000); + retry(5); +} + +// SOLUTION: Named constants +const OrderStatus = { + PENDING: 1, + PROCESSING: 2, + COMPLETED: 3, +} as const; + +const ONE_DAY_MS = 24 * 60 * 60 * 1000; +const MAX_RETRIES = 5; + +if (status === OrderStatus.COMPLETED) { + setTimeout(callback, ONE_DAY_MS); + retry(MAX_RETRIES); +} +``` + +**Complex Expressions** +```typescript +// PROBLEM: Hard to parse mentally +const result = data.filter(x => x.active && x.type === 'premium') + .map(x => ({ ...x, score: x.points * (x.bonus ? 1.5 : 1) / x.level })) + .sort((a, b) => b.score - a.score)[0]?.score || 0; + +// SOLUTION: Break into named steps +const activePremiusers = data.filter(isActivePremium); +const withScores = activePremiumUsers.map(calculateScore); +const topScore = findHighestScore(withScores); + +function isActivePremium(user: User) { + return user.active && user.type === 'premium'; +} + +function calculateScore(user: User) { + const multiplier = user.bonus ? 1.5 : 1; + return { ...user, score: (user.points * multiplier) / user.level }; +} + +function findHighestScore(users: ScoredUser[]) { + return users.sort((a, b) => b.score - a.score)[0]?.score ?? 0; +} +``` + +**Unclear Variable Names** +```typescript +// PROBLEM: Cryptic names +const d = new Date(); +const t = d.getTime(); +const r = items.filter(i => i.t > t - 86400000); +const x = r.reduce((a, b) => a + b.p, 0); + +// SOLUTION: Descriptive names +const now = new Date(); +const oneDayAgo = now.getTime() - ONE_DAY_MS; +const recentItems = items.filter(item => item.timestamp > oneDayAgo); +const totalPrice = recentItems.reduce((sum, item) => sum + item.price, 0); +``` + +### 3. Maintainability Issues + +**Code Duplication** +```typescript +// PROBLEM: Same logic repeated +function validateEmail(email: string) { + if (!email) return { valid: false, error: 'Required' }; + if (!email.includes('@')) return { valid: false, error: 'Invalid format' }; + return { valid: true, error: null }; +} + +function validateUsername(username: string) { + if (!username) return { valid: false, error: 'Required' }; + if (username.length < 3) return { valid: false, error: 'Too short' }; + return { valid: true, error: null }; +} + +// Similar validation in 10 more places... + +// SOLUTION: Generic validation framework +type ValidationRule = (value: T) => string | null; + +function validate(value: T, rules: ValidationRule[]): ValidationResult { + for (const rule of rules) { + const error = rule(value); + if (error) return { valid: false, error }; + } + return { valid: true, error: null }; +} + +const required = (v: string) => v ? null : 'Required'; +const minLength = (n: number) => (v: string) => v.length >= n ? null : 'Too short'; +const isEmail = (v: string) => v.includes('@') ? null : 'Invalid format'; + +const validateEmail = (email: string) => validate(email, [required, isEmail]); +const validateUsername = (name: string) => validate(name, [required, minLength(3)]); +``` + +**Long Parameter Lists** +```typescript +// PROBLEM: Too many parameters +function createUser( + name: string, + email: string, + password: string, + role: string, + department: string, + manager: string, + startDate: Date, + salary: number, + benefits: string[] +) { + // ... +} + +// SOLUTION: Use object parameter +interface CreateUserParams { + name: string; + email: string; + password: string; + role: string; + department: string; + manager?: string; + startDate: Date; + salary: number; + benefits?: string[]; +} + +function createUser(params: CreateUserParams) { + // ... +} +``` + +**Shotgun Surgery Indicators** +```typescript +// PROBLEM: Change requires modifying many files +// To add a new user type, you must modify: +// - UserType enum +// - UserFactory +// - UserValidator +// - UserSerializer +// - UserRepository +// - UserController +// - user.routes.ts +// - 5 more files... + +// SOLUTION: Encapsulate variation +interface UserTypeHandler { + validate(data: UserData): ValidationResult; + serialize(user: User): SerializedUser; + getPermissions(): Permission[]; +} + +class AdminUserHandler implements UserTypeHandler { /* ... */ } +class RegularUserHandler implements UserTypeHandler { /* ... */ } + +// Adding new type = one new class +``` + +### 4. Boolean Complexity + +**Complex Boolean Expressions** +```typescript +// PROBLEM: Hard to understand +if (user.active && !user.deleted && (user.role === 'admin' || user.role === 'moderator') && user.verified && (!user.suspended || user.suspendedUntil < Date.now())) { + // ... +} + +// SOLUTION: Extract to named predicates +const canModerate = (user: User): boolean => { + if (!user.active || user.deleted) return false; + if (!['admin', 'moderator'].includes(user.role)) return false; + if (!user.verified) return false; + if (user.suspended && user.suspendedUntil >= Date.now()) return false; + return true; +}; + +if (canModerate(user)) { + // ... +} +``` + +**Negation Overuse** +```typescript +// PROBLEM: Double/triple negatives +if (!user.isNotActive && !items.isEmpty()) { + // ... +} + +if (!(a && !b) || !(!c || d)) { + // ... +} + +// SOLUTION: Positive conditions +if (user.isActive && items.length > 0) { + // ... +} + +// Apply De Morgan's law and simplify +if (!a || b || (c && !d)) { + // ... +} +``` + +--- + +## Severity Guidelines + +**CRITICAL** - Code is unmaintainable: +- Functions > 200 lines +- Cyclomatic complexity > 20 +- Nesting depth > 6 levels +- Duplicated logic in 5+ places + +**HIGH** - Significant maintainability risk: +- Functions 50-200 lines +- Cyclomatic complexity 10-20 +- Nesting depth 4-6 levels +- Complex boolean expressions (5+ conditions) +- Parameter lists > 5 parameters + +**MEDIUM** - Moderate complexity concern: +- Functions 30-50 lines +- Cyclomatic complexity 5-10 +- Magic numbers/strings +- Minor code duplication + +**LOW** - Minor improvement opportunity: +- Could be more readable +- Naming could be clearer +- Comments would help + +--- + +## Detection Patterns + +Search for these patterns in code: + +```bash +# Long functions (rough estimate) +grep -rn "function\|=>" --include="*.ts" | head -100 + +# Deep nesting (multiple indentation levels) +grep -rn "^ if\|^ for\|^ while" --include="*.ts" + +# Magic numbers +grep -rn "[^a-zA-Z][0-9]{3,}[^a-zA-Z]" --include="*.ts" | grep -v "const\|enum\|type" + +# Long parameter lists +grep -rn "function.*,.*,.*,.*,.*," --include="*.ts" + +# Complex boolean expressions +grep -rn "&&.*&&.*&&\|||.*||.*||" --include="*.ts" + +# Duplicated code (look for similar patterns) +grep -rn "if (!.*) return\|throw" --include="*.ts" | sort | uniq -c | sort -rn +``` + +--- + +## Complexity Metrics Reference + +| Metric | Good | Warning | Critical | +|--------|------|---------|----------| +| Function length | < 30 lines | 30-50 lines | > 50 lines | +| Cyclomatic complexity | < 5 | 5-10 | > 10 | +| Nesting depth | < 3 | 3-4 | > 4 | +| Parameters | < 3 | 3-5 | > 5 | +| File length | < 300 lines | 300-500 lines | > 500 lines | + diff --git a/skills/devflow-consistency-patterns/SKILL.md b/skills/devflow-consistency-patterns/SKILL.md new file mode 100644 index 00000000..07e40f32 --- /dev/null +++ b/skills/devflow-consistency-patterns/SKILL.md @@ -0,0 +1,350 @@ +--- +name: devflow-consistency-patterns +description: Code consistency, pattern adherence, and unnecessary simplification detection. Load when reviewing code for style violations, content truncation, or feature regressions. Used by Reviewer agent with consistency focus. +allowed-tools: Read, Grep, Glob +--- + +# Consistency Patterns + +Domain expertise for code consistency and unnecessary simplification detection. Use alongside `devflow-review-methodology` for complete consistency reviews. + +## Iron Law + +> **MATCH EXISTING PATTERNS OR JUSTIFY DEVIATION** +> +> New code should look like existing code. If the codebase uses camelCase, use camelCase. +> If errors return Result types, return Result types. Consistency trumps personal preference. +> Deviation requires explicit justification and team agreement. One codebase, one style. + +## Consistency Categories + +### 1. Unnecessary Simplification + +**Content Truncation** +```typescript +// BEFORE (comprehensive) +const errorMessages = { + INVALID_EMAIL: 'Please enter a valid email address in the format user@domain.com', + PASSWORD_WEAK: 'Password must contain at least 8 characters, one uppercase, one lowercase, one number, and one special character', + USER_NOT_FOUND: 'We could not find an account with that email. Please check the email or create a new account.', +}; + +// AFTER (over-simplified - PROBLEM) +const errorMessages = { + INVALID_EMAIL: 'Invalid email', + PASSWORD_WEAK: 'Password too weak', + USER_NOT_FOUND: 'Not found', +}; + +// RED FLAG: User-facing messages should be helpful, not minimal +``` + +**Removed Error Context** +```typescript +// BEFORE (informative) +throw new Error(`Failed to process order ${orderId}: ${reason}. Customer: ${customerId}. Items: ${itemCount}`); + +// AFTER (stripped - PROBLEM) +throw new Error('Order failed'); + +// RED FLAG: Debug info removed, harder to troubleshoot +``` + +**Stripped Configuration Options** +```typescript +// BEFORE (flexible) +interface ServerConfig { + port: number; + host: string; + timeout: number; + maxConnections: number; + ssl: SSLConfig; + logging: LogConfig; + cors: CorsConfig; +} + +// AFTER (rigid - PROBLEM) +interface ServerConfig { + port: number; +} + +// RED FLAG: Configuration flexibility removed +``` + +### 2. Pattern Violations + +**Naming Convention Inconsistency** +```typescript +// EXISTING PATTERN: camelCase for functions +function getUserById(id: string) { } +function createOrder(data: OrderData) { } + +// VIOLATION: Different style +function Process_Payment(amount: number) { } // snake_case +function VALIDATE_INPUT(input: string) { } // SCREAMING_CASE + +// CORRECT: Match existing +function processPayment(amount: number) { } +function validateInput(input: string) { } +``` + +**Error Handling Style Mismatch** +```typescript +// EXISTING PATTERN: Result types +function existingFunction(): Result { + if (!valid) return Err(new ValidationError('...')); + return Ok(user); +} + +// VIOLATION: Throws instead +function newFunction(): User { + if (!valid) throw new Error('...'); // Different pattern! + return user; +} + +// CORRECT: Match existing +function newFunction(): Result { + if (!valid) return Err(new ValidationError('...')); + return Ok(user); +} +``` + +**Import Organization Inconsistency** +```typescript +// EXISTING PATTERN: External, internal, relative +import express from 'express'; // External +import { Logger } from '@internal/logger'; // Internal +import { User } from './models'; // Relative + +// VIOLATION: Mixed order +import { User } from './models'; +import express from 'express'; +import { Logger } from '@internal/logger'; + +// CORRECT: Match existing organization +``` + +**Export Pattern Mismatch** +```typescript +// EXISTING PATTERN: Named exports +export function createUser() { } +export function deleteUser() { } +export const UserSchema = z.object({ }); + +// VIOLATION: Default export +export default class UserService { // Different pattern! + create() { } + delete() { } +} + +// CORRECT: Match existing +export class UserService { } +export const userService = new UserService(); +``` + +### 3. Feature Regression + +**Removed CLI Options** +```typescript +// BEFORE +program + .option('-v, --verbose', 'Enable verbose output') + .option('-d, --debug', 'Enable debug mode') + .option('-c, --config ', 'Config file path') + .option('--dry-run', 'Preview without executing'); + +// AFTER (PROBLEM) +program + .option('-c, --config ', 'Config file path'); + +// RED FLAG: Users relying on removed options will break +``` + +**Changed Return Types** +```typescript +// BEFORE +async function fetchUsers(): Promise { + return users; +} + +// AFTER (PROBLEM) +async function fetchUsers(): Promise<{ data: User[] }> { + return { data: users }; // Breaking change! +} + +// All callers expecting User[] will break +``` + +**Removed Event Emissions** +```typescript +// BEFORE +class OrderService { + async createOrder(data: OrderData) { + const order = await this.repository.create(data); + this.events.emit('order.created', order); // Other services listen + this.events.emit('inventory.reserve', order.items); + return order; + } +} + +// AFTER (PROBLEM) +class OrderService { + async createOrder(data: OrderData) { + const order = await this.repository.create(data); + return order; // Events removed - listeners won't fire! + } +} +``` + +### 4. Style Inconsistency + +**Brace Style** +```typescript +// EXISTING PATTERN: Same-line braces +function existing() { + if (condition) { + // ... + } +} + +// VIOLATION: Next-line braces +function newFunction() +{ + if (condition) + { + // ... + } +} +``` + +**Quote Style** +```typescript +// EXISTING PATTERN: Single quotes +const name = 'John'; +const message = 'Hello'; + +// VIOLATION: Double quotes +const name = "Jane"; // Inconsistent + +// CORRECT: Match existing +const name = 'Jane'; +``` + +**Trailing Commas** +```typescript +// EXISTING PATTERN: Trailing commas +const config = { + name: 'app', + version: '1.0', +}; + +// VIOLATION: No trailing comma +const newConfig = { + name: 'app', + version: '1.0' // Missing trailing comma +}; +``` + +--- + +## Red Flags Detection + +**Diff Statistics to Watch:** +```bash +# Files with many more deletions than additions +git diff main...HEAD --stat | awk '{ + if (match($0, /\+([0-9]+).*-([0-9]+)/, arr)) { + added = arr[1]; deleted = arr[2]; + if (deleted > added * 2 && deleted > 10) { + print "WARNING:", $0 + } + } +}' +``` + +**Content Length Changes:** +```bash +# Compare line counts for key files +for file in $(git diff --name-only main...HEAD); do + if [ -f "$file" ]; then + before=$(git show main:"$file" 2>/dev/null | wc -l) + after=$(wc -l < "$file") + if [ "$before" -gt "$after" ]; then + reduction=$((before - after)) + percent=$((reduction * 100 / before)) + if [ "$percent" -gt 20 ]; then + echo "WARNING: $file reduced by $percent% ($before -> $after lines)" + fi + fi + fi +done +``` + +--- + +## Severity Guidelines + +**CRITICAL** - Breaking changes or significant content loss: +- Public API return types changed +- CLI options/flags removed +- Error messages stripped to uselessness +- Event emissions removed +- Configuration options removed without deprecation + +**HIGH** - Inconsistency requiring attention: +- Error handling pattern mismatch +- Naming convention violations +- Export pattern inconsistency +- Documentation removed from public APIs + +**MEDIUM** - Style inconsistency: +- Import organization different +- Brace/quote style mismatch +- Comment style variations +- Minor formatting differences + +**LOW** - Minor observations: +- Could be more consistent +- Personal preference territory +- Linter should catch this + +--- + +## Detection Patterns + +Search for these patterns in code: + +```bash +# Find shortened error messages +git diff main...HEAD -- "*.ts" | grep "^-.*Error\|^-.*throw" | head -20 + +# Find removed exports +git diff main...HEAD -- "*.ts" | grep "^-export" | head -20 + +# Find removed options +git diff main...HEAD -- "*.ts" | grep "^-.*option\|^-.*flag" | head -20 + +# Compare naming styles +grep -rn "function [A-Z_]" --include="*.ts" # Unusual naming +grep -rn "const [a-z].*= function" --include="*.ts" # Mixed function styles + +# Find inconsistent quotes +grep -rn '"[^"]*"' --include="*.ts" | head -10 +grep -rn "'[^']*'" --include="*.ts" | head -10 +``` + +--- + +## Consistency Checklist + +Before approving changes, verify: + +- [ ] Naming matches existing patterns (camelCase, PascalCase, etc.) +- [ ] Error handling matches existing approach (throw vs Result) +- [ ] Import organization matches existing files +- [ ] Export style matches existing modules +- [ ] No user-facing content was unnecessarily shortened +- [ ] No configuration options were silently removed +- [ ] No CLI flags/options were removed without deprecation +- [ ] All removed functionality has explicit justification + diff --git a/skills/devflow-core-patterns/SKILL.md b/skills/devflow-core-patterns/SKILL.md index 9eb8acea..127c5562 100644 --- a/skills/devflow-core-patterns/SKILL.md +++ b/skills/devflow-core-patterns/SKILL.md @@ -642,14 +642,3 @@ Before approving code: - [ ] Naming conventions followed (PascalCase, camelCase, SCREAMING_SNAKE_CASE) - [ ] Performance-critical code measured before optimizing ---- - -## Integration - -This skill is the foundation for: -- **Coder agent**: Follow these patterns when implementing -- **Review agents**: Check code against these patterns -- **Debug agent**: Use these patterns to identify root causes -- **Test design**: These patterns make testing trivial - -When implementing, if you're unsure about a pattern, consult this skill first. diff --git a/skills/devflow-database-patterns/SKILL.md b/skills/devflow-database-patterns/SKILL.md new file mode 100644 index 00000000..2a19aa47 --- /dev/null +++ b/skills/devflow-database-patterns/SKILL.md @@ -0,0 +1,336 @@ +--- +name: devflow-database-patterns +description: Database design and optimization review. Load when reviewing migrations, schema changes, query patterns, or database-related code. Used by Reviewer agent with database focus. +allowed-tools: Read, Grep, Glob +--- + +# Database Patterns + +Domain expertise for database design and optimization. Use alongside `devflow-review-methodology` for complete database reviews. + +## Iron Law + +> **EVERY QUERY MUST HAVE AN EXECUTION PLAN** +> +> Never deploy a query without understanding its execution plan. Every WHERE clause needs +> an index analysis. Every JOIN needs cardinality consideration. "It works in dev" is not +> validation. Production data volumes will expose every missing index and inefficient join. + +## Database Categories + +### 1. Schema Design Issues + +**Missing Foreign Keys** +```sql +-- PROBLEM: No referential integrity +CREATE TABLE orders ( + id SERIAL PRIMARY KEY, + customer_id INT, -- No FK constraint! + total DECIMAL +); + +-- SOLUTION: Add foreign key +CREATE TABLE orders ( + id SERIAL PRIMARY KEY, + customer_id INT NOT NULL REFERENCES customers(id) ON DELETE RESTRICT, + total DECIMAL +); +``` + +**Denormalization Without Justification** +```sql +-- PROBLEM: Unnecessary duplication +CREATE TABLE orders ( + id SERIAL PRIMARY KEY, + customer_id INT, + customer_name VARCHAR(100), -- Duplicated! + customer_email VARCHAR(100), -- Duplicated! + customer_address TEXT -- Duplicated! +); + +-- SOLUTION: Normalize unless performance requires otherwise +CREATE TABLE orders ( + id SERIAL PRIMARY KEY, + customer_id INT REFERENCES customers(id) +); +-- Access customer data via JOIN +``` + +**Poor Data Type Choices** +```sql +-- PROBLEM: Inappropriate types +CREATE TABLE users ( + id VARCHAR(100), -- Use UUID or SERIAL + age VARCHAR(10), -- Use INT + balance VARCHAR(50), -- Use DECIMAL + is_active VARCHAR(5), -- Use BOOLEAN + created_at VARCHAR(50) -- Use TIMESTAMP +); + +-- SOLUTION: Appropriate types +CREATE TABLE users ( + id UUID PRIMARY KEY DEFAULT gen_random_uuid(), + age INT CHECK (age >= 0 AND age < 150), + balance DECIMAL(10, 2) DEFAULT 0, + is_active BOOLEAN DEFAULT true, + created_at TIMESTAMP WITH TIME ZONE DEFAULT NOW() +); +``` + +**Missing Constraints** +```sql +-- PROBLEM: No data validation +CREATE TABLE products ( + id SERIAL PRIMARY KEY, + name VARCHAR(100), + price DECIMAL, + quantity INT +); + +-- SOLUTION: Add constraints +CREATE TABLE products ( + id SERIAL PRIMARY KEY, + name VARCHAR(100) NOT NULL, + price DECIMAL(10, 2) NOT NULL CHECK (price >= 0), + quantity INT NOT NULL DEFAULT 0 CHECK (quantity >= 0), + CONSTRAINT name_not_empty CHECK (LENGTH(TRIM(name)) > 0) +); +``` + +### 2. Query Optimization Issues + +**N+1 Queries** +```typescript +// PROBLEM: Query per iteration +const users = await db.query('SELECT * FROM users'); +for (const user of users) { + const orders = await db.query( + 'SELECT * FROM orders WHERE user_id = ?', + [user.id] + ); + user.orders = orders; +} + +// SOLUTION: Single query with JOIN or IN +const users = await db.query(` + SELECT u.*, json_agg(o.*) as orders + FROM users u + LEFT JOIN orders o ON o.user_id = u.id + GROUP BY u.id +`); + +// Or two queries with IN +const users = await db.query('SELECT * FROM users'); +const userIds = users.map(u => u.id); +const orders = await db.query( + 'SELECT * FROM orders WHERE user_id = ANY(?)', + [userIds] +); +``` + +**Missing Indexes** +```sql +-- PROBLEM: Frequent query without index +SELECT * FROM orders WHERE customer_id = 123 AND status = 'pending'; + +-- Check execution plan +EXPLAIN ANALYZE SELECT * FROM orders WHERE customer_id = 123 AND status = 'pending'; +-- Sequential Scan on orders (cost=0.00..1234.00 rows=100 width=100) + +-- SOLUTION: Add composite index +CREATE INDEX idx_orders_customer_status ON orders(customer_id, status); + +-- After index: +-- Index Scan using idx_orders_customer_status (cost=0.00..8.00 rows=100 width=100) +``` + +**Full Table Scans** +```sql +-- PROBLEM: Functions prevent index use +SELECT * FROM users WHERE LOWER(email) = 'john@example.com'; +SELECT * FROM orders WHERE YEAR(created_at) = 2024; + +-- SOLUTION: Use index-friendly queries +-- Option 1: Functional index +CREATE INDEX idx_users_email_lower ON users(LOWER(email)); + +-- Option 2: Rewrite query +SELECT * FROM users WHERE email ILIKE 'john@example.com'; +SELECT * FROM orders WHERE created_at >= '2024-01-01' AND created_at < '2025-01-01'; +``` + +**Inefficient JOINs** +```sql +-- PROBLEM: Joining large tables without filters +SELECT * FROM orders o +JOIN order_items oi ON oi.order_id = o.id +JOIN products p ON p.id = oi.product_id; +-- Returns millions of rows + +-- SOLUTION: Filter before joining +SELECT * FROM orders o +JOIN order_items oi ON oi.order_id = o.id +JOIN products p ON p.id = oi.product_id +WHERE o.customer_id = 123 + AND o.created_at > NOW() - INTERVAL '30 days'; +``` + +### 3. Migration Issues + +**Breaking Changes Without Migration Path** +```sql +-- PROBLEM: Destructive change +ALTER TABLE users DROP COLUMN legacy_field; +-- Data is lost! + +-- SOLUTION: Phased approach +-- Phase 1: Add deprecation, stop writes +-- Phase 2: Migrate data if needed +-- Phase 3: Remove column after verification +ALTER TABLE users ALTER COLUMN legacy_field SET DEFAULT NULL; +-- Wait for deployment, verify no writes +ALTER TABLE users DROP COLUMN legacy_field; +``` + +**Data Loss Risk** +```sql +-- PROBLEM: Type change loses data +ALTER TABLE products ALTER COLUMN price TYPE INT; +-- Decimal values truncated! + +-- SOLUTION: Validate first +SELECT id, price FROM products WHERE price != FLOOR(price); +-- If results: handle decimal values first +``` + +**Missing Rollback Strategy** +```typescript +// PROBLEM: No way to undo +export async function up(db) { + await db.query('DROP TABLE old_users'); +} + +export async function down(db) { + // Can't recreate dropped table! +} + +// SOLUTION: Always plan rollback +export async function up(db) { + await db.query('ALTER TABLE old_users RENAME TO old_users_backup'); + await db.query('CREATE TABLE users_v2 AS SELECT * FROM old_users_backup'); +} + +export async function down(db) { + await db.query('DROP TABLE IF EXISTS users_v2'); + await db.query('ALTER TABLE old_users_backup RENAME TO old_users'); +} +``` + +**Performance Impact** +```sql +-- PROBLEM: Locks table for extended period +ALTER TABLE large_table ADD COLUMN new_field VARCHAR(100) NOT NULL DEFAULT 'value'; +-- Rewrites entire table, locks during operation + +-- SOLUTION: Non-blocking approach +-- Step 1: Add nullable column (instant) +ALTER TABLE large_table ADD COLUMN new_field VARCHAR(100); + +-- Step 2: Backfill in batches +UPDATE large_table SET new_field = 'value' WHERE id BETWEEN 1 AND 10000; +-- Repeat for ranges... + +-- Step 3: Add constraint after backfill +ALTER TABLE large_table ALTER COLUMN new_field SET NOT NULL; +``` + +### 4. Security Issues + +**SQL Injection** +```typescript +// VULNERABLE +const query = `SELECT * FROM users WHERE email = '${email}'`; +await db.query(query); + +// SECURE: Parameterized query +const query = 'SELECT * FROM users WHERE email = $1'; +await db.query(query, [email]); +``` + +**Excessive Privileges** +```sql +-- PROBLEM: App has too many privileges +GRANT ALL PRIVILEGES ON DATABASE myapp TO app_user; + +-- SOLUTION: Minimum required privileges +GRANT SELECT, INSERT, UPDATE ON users TO app_user; +GRANT SELECT ON products TO app_user; +-- No DELETE unless needed +-- No schema modification privileges +``` + +--- + +## Severity Guidelines + +**CRITICAL** - Data integrity or severe performance: +- Missing indexes on high-traffic queries +- N+1 queries with unbounded data +- Data loss in migrations +- SQL injection vulnerabilities +- Missing foreign keys on critical relationships + +**HIGH** - Significant database issues: +- Inefficient JOINs on large tables +- Missing constraints allowing bad data +- Migrations without rollback +- Poor data type choices + +**MEDIUM** - Moderate concerns: +- Minor denormalization +- Missing non-critical indexes +- Could use better constraints + +**LOW** - Minor improvements: +- Naming conventions +- Index organization +- Documentation + +--- + +## Detection Patterns + +Search for these patterns in code: + +```bash +# SQL injection (string interpolation in queries) +grep -rn "query.*\${.*}\|query.*+ " --include="*.ts" + +# N+1 patterns (queries in loops) +grep -rn "for.*await.*query\|forEach.*await.*query" --include="*.ts" + +# SELECT * usage +grep -rn "SELECT \*" --include="*.ts" --include="*.sql" + +# Missing parameterization +grep -rn "WHERE.*'.*\$\|WHERE.*\".*\$" --include="*.ts" + +# Check migration files +find . -name "*migration*" -o -name "*migrate*" | xargs grep -l "DROP\|DELETE\|TRUNCATE" +``` + +--- + +## Database Checklist + +Before approving database changes: + +- [ ] All queries have appropriate indexes +- [ ] N+1 patterns identified and resolved +- [ ] Migrations have rollback scripts +- [ ] Data types are appropriate +- [ ] Constraints enforce business rules +- [ ] Foreign keys maintain referential integrity +- [ ] No SQL injection vulnerabilities +- [ ] Performance tested with production-like data volume + diff --git a/skills/devflow-dependencies-patterns/SKILL.md b/skills/devflow-dependencies-patterns/SKILL.md new file mode 100644 index 00000000..22f31a13 --- /dev/null +++ b/skills/devflow-dependencies-patterns/SKILL.md @@ -0,0 +1,324 @@ +--- +name: devflow-dependencies-patterns +description: Dependency management and security analysis. Load when reviewing package.json changes, dependency updates, or assessing supply chain risks. Used by Reviewer agent with dependencies focus. +allowed-tools: Read, Grep, Glob +--- + +# Dependencies Patterns + +Domain expertise for dependency management and security analysis. Use alongside `devflow-review-methodology` for complete dependency reviews. + +## Iron Law + +> **EVERY DEPENDENCY IS AN ATTACK SURFACE** +> +> Each package you add is code you didn't write but must trust. Minimize dependencies. +> Pin versions. Audit regularly. A single compromised transitive dependency can compromise +> your entire application. "It's a popular package" is not a security review. + +## Dependency Categories + +### 1. Security Vulnerabilities + +**Known CVEs** +```bash +# Check for known vulnerabilities +npm audit +# or +yarn audit +# or +pnpm audit + +# Output example: +# High: Prototype Pollution in lodash +# Package: lodash +# Dependency of: my-package +# Path: my-package > lodash +# More info: https://github.com/advisories/GHSA-xxx +``` + +**Vulnerable Version Ranges** +```json +// PROBLEM: Wide version range includes vulnerable versions +{ + "dependencies": { + "lodash": "^4.0.0" // Includes vulnerable 4.17.0-4.17.20 + } +} + +// SOLUTION: Pin to safe version or use range excluding vulnerable +{ + "dependencies": { + "lodash": "^4.17.21" // First safe version + } +} +``` + +**Malicious Packages** +```json +// RED FLAGS for potentially malicious packages: +{ + "dependencies": { + "loadsh": "1.0.0", // Typosquat of "lodash" + "event-stream": "3.3.6", // Known compromised version + "random-unknown-pkg": "0.0.1" // No downloads, no repo + } +} + +// VERIFY packages: +// - Check npm page for download counts +// - Verify repository link +// - Check maintainer history +// - Look for typosquatting +``` + +### 2. Version Management Issues + +**Unpinned Versions** +```json +// PROBLEM: Can get different versions on each install +{ + "dependencies": { + "express": "*", // Any version! + "lodash": "latest", // Whatever is latest + "moment": "" // Empty = latest + } +} + +// SOLUTION: Pin exact or use caret with lockfile +{ + "dependencies": { + "express": "4.18.2", // Exact pin + "lodash": "^4.17.21", // Caret + lockfile + "moment": "~2.29.4" // Tilde for patch only + } +} +``` + +**Missing Lockfile** +```bash +# PROBLEM: No lockfile committed +.gitignore: +package-lock.json # Don't ignore this! +yarn.lock # Don't ignore this! + +# SOLUTION: Commit lockfile +git add package-lock.json # or yarn.lock +git commit -m "Add lockfile for reproducible builds" +``` + +**Dependency Conflicts** +```bash +# PROBLEM: Multiple versions of same package +npm ls react +# my-app +# ā”œā”€ā”€ react@18.2.0 +# ā””ā”€ā”€ some-library +# ā””ā”€ā”€ react@17.0.2 # Conflict! + +# SOLUTION: Use resolutions/overrides +# package.json (yarn) +{ + "resolutions": { + "react": "18.2.0" + } +} + +# package.json (npm) +{ + "overrides": { + "react": "18.2.0" + } +} +``` + +### 3. Dependency Issues + +**Outdated Packages** +```bash +# Check for outdated +npm outdated + +# Package Current Wanted Latest +# lodash 4.17.15 4.17.21 4.17.21 # Security update! +# typescript 4.9.5 4.9.5 5.3.2 # Major version +# @types/node 18.0.0 18.19.0 20.10.0 # Minor updates + +# Prioritize: +# 1. Security patches (lodash) +# 2. Bug fixes (minor updates) +# 3. Major versions (careful review) +``` + +**Unused Dependencies** +```bash +# Find unused dependencies +npx depcheck + +# Unused dependencies: +# * moment # Listed but never imported +# * lodash # Listed but never imported + +# SOLUTION: Remove unused +npm uninstall moment lodash +``` + +**Unnecessary Dependencies** +```json +// PROBLEM: Heavy dependencies for simple tasks +{ + "dependencies": { + "moment": "^2.29.4", // 300KB for date formatting + "lodash": "^4.17.21", // 70KB for one function + "left-pad": "^1.3.0" // 1KB for string padding + } +} + +// SOLUTION: Use native or lighter alternatives +// Native date formatting +new Date().toLocaleDateString(); + +// Native array methods instead of lodash +array.filter(x => x.active); + +// Native string padding +'5'.padStart(2, '0'); + +// Or import only what you need +import { debounce } from 'lodash/debounce'; +``` + +### 4. License Issues + +**Incompatible Licenses** +```bash +# Check licenses +npx license-checker --summary + +# Watch for incompatible combinations: +# - GPL in MIT project (viral license) +# - Commercial-only licenses +# - AGPL in SaaS (requires source disclosure) + +# Example problematic output: +# GPL-3.0: some-package # Requires your code to be GPL too! +``` + +**Missing License** +```bash +# Find packages without license +npx license-checker --onlyunknown + +# Packages with unknown license: +# - internal-company-pkg@1.0.0 # Verify this is intentional +``` + +### 5. Supply Chain Risks + +**Transitive Dependencies** +```bash +# Check dependency tree depth +npm ls --all | wc -l +# If > 1000, high supply chain risk + +# Audit transitive deps +npm audit --all + +# SOLUTION: Minimize dependencies, audit regularly +``` + +**Maintainer Concerns** +```json +// RED FLAGS: +// - Package with 1 maintainer who's inactive +// - No recent releases but many open issues +// - Repository archived or deleted +// - Maintainer account compromised (check news) + +// Check package health: +// https://snyk.io/advisor/npm-package/{package-name} +``` + +--- + +## Severity Guidelines + +**CRITICAL** - Immediate security risk: +- Known exploited CVEs (CISA KEV) +- Critical severity vulnerabilities +- Confirmed malicious packages +- Typosquatted package names + +**HIGH** - Significant risk: +- High severity CVEs +- Packages with no maintainers +- Extremely outdated with security fixes +- Incompatible GPL in proprietary code + +**MEDIUM** - Moderate concerns: +- Medium severity CVEs +- Significantly outdated packages +- Wide version ranges +- Missing lockfile + +**LOW** - Minor improvements: +- Unused dependencies +- Could use lighter alternatives +- Minor version behind + +--- + +## Detection Patterns + +Search for these patterns: + +```bash +# Check for vulnerabilities +npm audit --json | jq '.vulnerabilities | keys' + +# Check for outdated +npm outdated --json + +# Check for unused +npx depcheck --json + +# Check lockfile exists +[ -f package-lock.json ] || [ -f yarn.lock ] || echo "No lockfile!" + +# Check for wide version ranges +grep -E '"[*~^]|": "latest|": ""' package.json + +# Check dependency count +jq '.dependencies | length' package.json +jq '.devDependencies | length' package.json +``` + +--- + +## Dependency Review Checklist + +Before approving dependency changes: + +- [ ] No known CVEs in added packages +- [ ] Version ranges are appropriate (not too wide) +- [ ] Lockfile updated and committed +- [ ] Package is actively maintained +- [ ] License is compatible +- [ ] Package is from verified publisher +- [ ] Transitive dependencies reviewed +- [ ] Package name verified (not typosquat) +- [ ] Bundle size impact considered +- [ ] Alternative native solutions considered + +--- + +## Common Vulnerability Sources + +| Registry | URL | +|----------|-----| +| npm Advisory | https://www.npmjs.com/advisories | +| Snyk Vuln DB | https://snyk.io/vuln | +| GitHub Advisory | https://github.com/advisories | +| NVD | https://nvd.nist.gov/ | +| CISA KEV | https://www.cisa.gov/known-exploited-vulnerabilities-catalog | + diff --git a/skills/devflow-documentation-patterns/SKILL.md b/skills/devflow-documentation-patterns/SKILL.md new file mode 100644 index 00000000..b718f892 --- /dev/null +++ b/skills/devflow-documentation-patterns/SKILL.md @@ -0,0 +1,342 @@ +--- +name: devflow-documentation-patterns +description: Documentation quality and code-documentation alignment. Load when reviewing code comments, API docs, READMEs, or checking for documentation drift. Used by Reviewer agent with documentation focus. +allowed-tools: Read, Grep, Glob +--- + +# Documentation Patterns + +Domain expertise for documentation quality and alignment. Use alongside `devflow-review-methodology` for complete documentation reviews. + +## Iron Law + +> **DOCUMENTATION MUST MATCH REALITY** +> +> Outdated documentation is worse than no documentation. It actively misleads. Every code +> change that affects behavior requires a documentation check. Comments that explain "what" +> instead of "why" are noise. The best documentation is code that doesn't need documentation. + +## Documentation Categories + +### 1. Code Documentation Issues + +**Missing Docstrings/JSDoc** +```typescript +// PROBLEM: Complex function without documentation +export function calculateProratedAmount( + plan: Plan, + startDate: Date, + endDate: Date, + previousPlan?: Plan +): number { + // 50 lines of complex billing logic... +} + +// SOLUTION: Document purpose, params, returns, edge cases +/** + * Calculates prorated billing amount when switching plans mid-cycle. + * + * @param plan - The new plan to prorate to + * @param startDate - When the new plan starts + * @param endDate - End of current billing cycle + * @param previousPlan - Optional previous plan for credit calculation + * @returns Prorated amount in cents (can be negative for downgrades) + * + * @example + * // Upgrading mid-month + * calculateProratedAmount(premiumPlan, new Date('2024-01-15'), new Date('2024-01-31')) + * // Returns: 1645 (cents) + * + * @throws {InvalidDateRangeError} If startDate is after endDate + */ +export function calculateProratedAmount( + plan: Plan, + startDate: Date, + endDate: Date, + previousPlan?: Plan +): number { + // ... +} +``` + +**Outdated Comments** +```typescript +// PROBLEM: Comment doesn't match code +// Returns user's full name +function getDisplayName(user: User): string { + return user.username; // Actually returns username, not full name! +} + +// PROBLEM: TODO that's been done +// TODO: Add validation +function processInput(input: string) { + if (!isValid(input)) throw new Error('Invalid'); // Validation exists! + // ... +} + +// SOLUTION: Keep comments in sync or remove +function getDisplayName(user: User): string { + return user.username; // No misleading comment needed +} +``` + +**Comments Explaining "What" Instead of "Why"** +```typescript +// BAD: Describes what code does (obvious from code) +// Loop through users +for (const user of users) { + // Check if user is active + if (user.active) { + // Add user to list + activeUsers.push(user); + } +} + +// GOOD: Explains why +// Filter to active users only - inactive users should not receive +// promotional emails per GDPR consent requirements +const activeUsers = users.filter(u => u.active); +``` + +**Complex Code Without Explanation** +```typescript +// PROBLEM: Magic algorithm without explanation +function schedule(tasks: Task[]): Schedule { + return tasks + .sort((a, b) => (b.priority * b.urgency) / b.duration - (a.priority * a.urgency) / a.duration) + .reduce((schedule, task) => { + const slot = findSlot(schedule, task, task.deadline - task.duration * 1.5); + return slot ? addToSchedule(schedule, task, slot) : schedule; + }, emptySchedule()); +} + +// SOLUTION: Explain the algorithm +/** + * Schedules tasks using weighted shortest job first (WSJF) algorithm. + * + * Priority score = (priority * urgency) / duration + * Higher scores are scheduled first. + * + * Tasks are placed starting 1.5x their duration before deadline + * to allow buffer for overruns. + * + * @see https://www.scaledagileframework.com/wsjf/ + */ +function schedule(tasks: Task[]): Schedule { + // ... +} +``` + +### 2. API Documentation Issues + +**Missing Parameter Descriptions** +```typescript +// PROBLEM: Parameters unexplained +/** + * Creates a new subscription. + */ +async function createSubscription( + userId: string, + planId: string, + options?: SubscriptionOptions +): Promise; + +// SOLUTION: Document all parameters +/** + * Creates a new subscription for a user. + * + * @param userId - The unique identifier of the user + * @param planId - The plan to subscribe to (from /plans endpoint) + * @param options - Optional configuration + * @param options.trialDays - Number of trial days (default: 0, max: 30) + * @param options.couponCode - Discount coupon to apply + * @param options.startDate - When to start (default: now) + * + * @returns The created subscription with payment details + * + * @throws {UserNotFoundError} If userId doesn't exist + * @throws {PlanNotFoundError} If planId doesn't exist + * @throws {InvalidCouponError} If coupon is expired or invalid + */ +async function createSubscription( + userId: string, + planId: string, + options?: SubscriptionOptions +): Promise; +``` + +**Missing Return Value Documentation** +```typescript +// PROBLEM: Return shape unknown +/** + * Fetches analytics data. + */ +async function getAnalytics(range: DateRange): Promise; + +// SOLUTION: Document return structure +/** + * Fetches aggregated analytics data for the date range. + * + * @param range - Start and end dates for the query + * @returns Analytics data including: + * - totalVisitors: Unique visitor count + * - pageViews: Total page view count + * - bounceRate: Percentage (0-100) of single-page sessions + * - avgSessionDuration: Average session length in seconds + * - topPages: Array of {path, views} sorted by views desc + */ +async function getAnalytics(range: DateRange): Promise; +``` + +**Missing Error Documentation** +```typescript +// PROBLEM: Errors not documented +async function transferFunds(from: string, to: string, amount: number): Promise; + +// SOLUTION: Document possible errors +/** + * Transfers funds between accounts. + * + * @throws {InsufficientFundsError} If source account balance < amount + * @throws {AccountNotFoundError} If either account doesn't exist + * @throws {AccountFrozenError} If either account is frozen + * @throws {TransferLimitError} If amount exceeds daily transfer limit + * @throws {SameAccountError} If from === to + */ +async function transferFunds(from: string, to: string, amount: number): Promise; +``` + +### 3. Alignment Issues + +**Code-Comment Drift** +```typescript +// BEFORE: Comment matches +// Retries up to 3 times with exponential backoff +async function fetchWithRetry(url: string) { + for (let i = 0; i < 3; i++) { /* ... */ } +} + +// AFTER: Code changed, comment didn't +// Retries up to 3 times with exponential backoff +async function fetchWithRetry(url: string) { + for (let i = 0; i < 5; i++) { /* ... */ } // Now 5 retries! +} + +// SOLUTION: Update comment or extract constant +const MAX_RETRIES = 5; +// Retries up to MAX_RETRIES times with exponential backoff +async function fetchWithRetry(url: string) { + for (let i = 0; i < MAX_RETRIES; i++) { /* ... */ } +} +``` + +**Stale README** +```markdown + + +## Installation +npm install mypackage + +## Usage +const { oldFunction } = require('mypackage'); +oldFunction(); // This function was removed! + + + +## Installation +npm install mypackage@^2.0.0 + +## Usage +import { newFunction } from 'mypackage'; +await newFunction({ /* new options */ }); + +## Migration from v1 +See [MIGRATION.md](./MIGRATION.md) for upgrade instructions. +``` + +**Missing Changelog Entries** +```markdown + + +## [2.0.0] - 2024-01-15 +- Updated dependencies + + + +## [2.0.0] - 2024-01-15 + +### Breaking Changes +- `createUser()` now returns `Promise>` instead of `Promise` +- Removed deprecated `oldFunction()` - use `newFunction()` instead +- Minimum Node.js version is now 18 + +### Added +- New `validateUser()` function for input validation + +### Changed +- Improved error messages for authentication failures +``` + +--- + +## Severity Guidelines + +**CRITICAL** - Actively misleading documentation: +- Comments that contradict code behavior +- API docs with wrong parameter types +- README with broken installation steps +- Changelog missing breaking changes + +**HIGH** - Significant documentation gaps: +- Public APIs without documentation +- Complex algorithms unexplained +- Error conditions not documented +- Migration guides missing + +**MEDIUM** - Moderate documentation issues: +- Some parameters undocumented +- Examples could be clearer +- Comments explain "what" not "why" + +**LOW** - Minor improvements: +- Could add more examples +- Formatting inconsistencies +- Typos or grammar + +--- + +## Detection Patterns + +Search for these patterns in code: + +```bash +# Functions without JSDoc +grep -rn "export function\|export async function" --include="*.ts" | \ + xargs -I {} sh -c 'grep -B1 "{}" | grep -v "/\*\*"' + +# TODO comments (potential staleness) +grep -rn "TODO\|FIXME\|HACK\|XXX" --include="*.ts" + +# Magic numbers (likely need comments) +grep -rn "[^a-zA-Z][0-9]{3,}[^a-zA-Z]" --include="*.ts" | grep -v "const\|enum" + +# Outdated references (check for removed/renamed) +grep -rn "@see\|@link\|@deprecated" --include="*.ts" +``` + +--- + +## Documentation Checklist + +Before approving changes: + +- [ ] All public APIs have JSDoc/docstrings +- [ ] Parameters and return values documented +- [ ] Error conditions documented +- [ ] Complex algorithms explained +- [ ] Comments explain "why", not "what" +- [ ] README reflects current state +- [ ] CHANGELOG updated for notable changes +- [ ] No TODO comments for completed work +- [ ] Examples work with current API + diff --git a/skills/devflow-performance-patterns/SKILL.md b/skills/devflow-performance-patterns/SKILL.md new file mode 100644 index 00000000..9448aa41 --- /dev/null +++ b/skills/devflow-performance-patterns/SKILL.md @@ -0,0 +1,379 @@ +--- +name: devflow-performance-patterns +description: Performance optimization and bottleneck detection. Load when reviewing code for algorithmic complexity, memory issues, I/O inefficiencies, or caching concerns. Used by Reviewer agent with performance focus. +allowed-tools: Read, Grep, Glob +--- + +# Performance Patterns + +Domain expertise for performance optimization and bottleneck detection. Use alongside `devflow-review-methodology` for complete performance reviews. + +## Iron Law + +> **MEASURE BEFORE OPTIMIZING** +> +> Premature optimization is the root of all evil. Profile first, then optimize. Every +> performance claim requires benchmarks. "It feels slow" is not a metric. O(n) with small n +> beats O(1) with huge constants. Optimize for the real bottleneck, not the imagined one. + +## Performance Categories + +### 1. Algorithmic Issues + +**N+1 Query Problem** +```typescript +// PROBLEM: 1 query + N queries +const users = await db.users.findAll(); +for (const user of users) { + const orders = await db.orders.findByUserId(user.id); // N queries! + user.orders = orders; +} + +// SOLUTION: Eager loading or batch query +const users = await db.users.findAll({ + include: [{ model: Order }] // 1-2 queries total +}); + +// Or batch query +const users = await db.users.findAll(); +const userIds = users.map(u => u.id); +const orders = await db.orders.findAll({ where: { userId: userIds } }); +const ordersByUser = groupBy(orders, 'userId'); +users.forEach(u => u.orders = ordersByUser[u.id] || []); +``` + +**O(n^2) or Worse** +```typescript +// PROBLEM: Nested loops +function findDuplicates(items: Item[]) { + const duplicates = []; + for (let i = 0; i < items.length; i++) { + for (let j = i + 1; j < items.length; j++) { + if (items[i].id === items[j].id) { // O(n^2) + duplicates.push(items[i]); + } + } + } + return duplicates; +} + +// SOLUTION: Use a Set or Map +function findDuplicates(items: Item[]) { + const seen = new Set(); + const duplicates: Item[] = []; + for (const item of items) { + if (seen.has(item.id)) { + duplicates.push(item); + } else { + seen.add(item.id); // O(n) total + } + } + return duplicates; +} +``` + +**Inefficient Search** +```typescript +// PROBLEM: Linear search in loop +function findMatches(needles: string[], haystack: string[]) { + return needles.filter(n => haystack.includes(n)); // O(n*m) +} + +// SOLUTION: Convert to Set for O(1) lookup +function findMatches(needles: string[], haystack: string[]) { + const haystackSet = new Set(haystack); + return needles.filter(n => haystackSet.has(n)); // O(n+m) +} +``` + +### 2. Database Issues + +**Missing Indexes** +```sql +-- PROBLEM: Full table scan +SELECT * FROM orders WHERE customer_id = 123; +-- Without index on customer_id: scans all rows + +-- SOLUTION: Add index +CREATE INDEX idx_orders_customer_id ON orders(customer_id); + +-- Composite index for multiple conditions +CREATE INDEX idx_orders_customer_status ON orders(customer_id, status); +``` + +**SELECT *** +```typescript +// PROBLEM: Fetches all columns +const users = await db.query('SELECT * FROM users WHERE status = ?', ['active']); + +// SOLUTION: Select only needed columns +const users = await db.query( + 'SELECT id, name, email FROM users WHERE status = ?', + ['active'] +); +``` + +**Large Result Sets Without Pagination** +```typescript +// PROBLEM: Loads millions of rows +const allOrders = await db.orders.findAll(); + +// SOLUTION: Paginate or stream +const orders = await db.orders.findAll({ + limit: 100, + offset: page * 100, + order: [['createdAt', 'DESC']] +}); + +// Or use cursor-based pagination +const orders = await db.orders.findAll({ + where: { id: { [Op.gt]: lastId } }, + limit: 100, + order: [['id', 'ASC']] +}); +``` + +### 3. Memory Issues + +**Memory Leaks** +```typescript +// PROBLEM: Event listeners not cleaned up +class Component { + mount() { + window.addEventListener('resize', this.handleResize); + } + // Missing unmount() to remove listener +} + +// SOLUTION: Clean up resources +class Component { + mount() { + window.addEventListener('resize', this.handleResize); + } + unmount() { + window.removeEventListener('resize', this.handleResize); + } +} +``` + +**Large Allocations in Loops** +```typescript +// PROBLEM: Creates new arrays/objects every iteration +function processItems(items: Item[]) { + for (const item of items) { + const result = JSON.parse(JSON.stringify(item)); // Full clone each time + const tags = item.tags.map(t => t.toLowerCase()); // New array each time + // ... + } +} + +// SOLUTION: Reuse or process in place +function processItems(items: Item[]) { + for (const item of items) { + // Mutate if ownership is clear, or process lazily + for (const tag of item.tags) { + processTag(tag.toLowerCase()); + } + } +} +``` + +**Unbounded Caches** +```typescript +// PROBLEM: Cache grows forever +const cache = new Map(); + +function getCached(key: string): Result { + if (!cache.has(key)) { + cache.set(key, computeExpensive(key)); // Never evicted! + } + return cache.get(key)!; +} + +// SOLUTION: LRU cache with limit +import LRU from 'lru-cache'; +const cache = new LRU({ max: 1000 }); +``` + +### 4. I/O Issues + +**Blocking Operations** +```typescript +// PROBLEM: Synchronous I/O blocks event loop +import fs from 'fs'; + +function readConfig() { + return fs.readFileSync('./config.json', 'utf-8'); // Blocks! +} + +// SOLUTION: Use async +import fs from 'fs/promises'; + +async function readConfig() { + return fs.readFile('./config.json', 'utf-8'); +} +``` + +**Sequential Async When Parallel Possible** +```typescript +// PROBLEM: Sequential execution +async function fetchAll(ids: string[]) { + const results = []; + for (const id of ids) { + results.push(await fetchItem(id)); // One at a time + } + return results; +} + +// SOLUTION: Parallel execution +async function fetchAll(ids: string[]) { + return Promise.all(ids.map(id => fetchItem(id))); +} + +// Or with concurrency limit +import pLimit from 'p-limit'; +const limit = pLimit(5); + +async function fetchAll(ids: string[]) { + return Promise.all(ids.map(id => limit(() => fetchItem(id)))); +} +``` + +**Redundant API Calls** +```typescript +// PROBLEM: Same data fetched multiple times +async function renderDashboard(userId: string) { + const user = await getUser(userId); + const orders = await getOrders(userId); + const user2 = await getUser(userId); // Duplicate! + + return { user: user2, orders }; +} + +// SOLUTION: Fetch once, pass around +async function renderDashboard(userId: string) { + const [user, orders] = await Promise.all([ + getUser(userId), + getOrders(userId) + ]); + return { user, orders }; +} +``` + +### 5. Rendering Issues (Frontend) + +**Unnecessary Re-renders** +```tsx +// PROBLEM: New object reference every render +function UserList({ users }) { + return ( + console.log(u)} // New function every render! + /> + ); +} + +// SOLUTION: Memoize or extract constants +const listStyle = { margin: 10 }; + +function UserList({ users }) { + const handleSelect = useCallback((u) => console.log(u), []); + + return ; +} +``` + +**Missing Virtualization for Long Lists** +```tsx +// PROBLEM: Renders 10000 DOM nodes +function UserList({ users }) { + return ( +
    + {users.map(u =>
  • {u.name}
    • )} +
+ ); +} + +// SOLUTION: Virtual list +import { FixedSizeList } from 'react-window'; + +function UserList({ users }) { + return ( + + {({ index, style }) => ( +
{users[index].name}
+ )} + + ); +} +``` + +--- + +## Severity Guidelines + +**CRITICAL** - Severe performance degradation: +- N+1 queries in loops with unbounded data +- O(n^2) or worse on large datasets +- Memory leaks in long-running processes +- Blocking I/O in request handlers +- Missing database indexes on high-traffic queries + +**HIGH** - Significant performance impact: +- Sequential async when parallel possible +- SELECT * on large tables +- Unbounded caches/collections +- Large allocations in hot paths +- Missing pagination on list endpoints + +**MEDIUM** - Moderate performance concern: +- Suboptimal algorithm with small data +- Missing memoization in renders +- Redundant data fetching +- Inefficient data structures + +**LOW** - Minor optimization opportunity: +- Micro-optimizations +- Style over performance +- Premature optimization candidates + +--- + +## Detection Patterns + +Search for these patterns in code: + +```bash +# N+1 queries (await in loop) +grep -rn "for.*await\|\.forEach.*await\|\.map.*await" --include="*.ts" + +# Synchronous I/O +grep -rn "readFileSync\|writeFileSync\|existsSync\|statSync" --include="*.ts" + +# Missing indexes (check query patterns) +grep -rn "WHERE.*=" --include="*.sql" --include="*.ts" + +# SELECT * +grep -rn "SELECT \*" --include="*.ts" --include="*.sql" + +# Nested loops +grep -rn "for.*{" -A10 --include="*.ts" | grep -B5 "for.*{" + +# Large allocations in loops +grep -rn "\.map\|\.filter\|JSON.parse\|JSON.stringify" --include="*.ts" | grep -i "for\|while\|each" +``` + +--- + +## Performance Metrics Reference + +| Operation | Good | Warning | Critical | +|-----------|------|---------|----------| +| API response | < 100ms | 100-500ms | > 500ms | +| Database query | < 10ms | 10-100ms | > 100ms | +| Page load (FCP) | < 1s | 1-2.5s | > 2.5s | +| Memory per request | < 10MB | 10-50MB | > 50MB | +| Bundle size | < 200KB | 200-500KB | > 500KB | + diff --git a/skills/devflow-regression-patterns/SKILL.md b/skills/devflow-regression-patterns/SKILL.md new file mode 100644 index 00000000..985c1ee5 --- /dev/null +++ b/skills/devflow-regression-patterns/SKILL.md @@ -0,0 +1,322 @@ +--- +name: devflow-regression-patterns +description: Functionality regression and intent validation analysis. Load when reviewing for lost functionality, broken behavior, or incomplete migrations. Used by Reviewer agent with regression focus. +allowed-tools: Read, Grep, Glob +--- + +# Regression Patterns + +Domain expertise for detecting functionality regressions and validating implementation intent. Use alongside `devflow-review-methodology` for complete regression reviews. + +## Iron Law + +> **WHAT WORKED BEFORE MUST WORK AFTER** +> +> Every change carries regression risk. Removed exports break consumers. Changed signatures +> break callers. Modified behavior breaks expectations. The burden of proof is on the change: +> demonstrate no regression, or document the intentional breaking change. + +## Regression Categories + +### 1. Lost Functionality + +**Removed Exports** +```typescript +// BEFORE: module exports +export function createUser(data: UserData): User { } +export function deleteUser(id: string): void { } +export function updateUser(id: string, data: Partial): User { } +export const USER_ROLES = ['admin', 'user', 'guest'] as const; + +// AFTER: exports removed (REGRESSION!) +export function createUser(data: UserData): User { } +// deleteUser - REMOVED +// updateUser - REMOVED +// USER_ROLES - REMOVED + +// Detection: Compare exports before/after +// git show main:src/users.ts | grep "^export" +// vs +// grep "^export" src/users.ts +``` + +**Removed CLI Options** +```typescript +// BEFORE +program + .option('-v, --verbose', 'Verbose output') + .option('-q, --quiet', 'Quiet mode') + .option('-f, --force', 'Force operation') + .option('--dry-run', 'Preview changes'); + +// AFTER: options removed (REGRESSION!) +program + .option('-v, --verbose', 'Verbose output'); +// --quiet, --force, --dry-run REMOVED + +// Users with scripts using these flags will break! +``` + +**Removed API Endpoints** +```typescript +// BEFORE +app.get('/api/users', listUsers); +app.get('/api/users/:id', getUser); +app.post('/api/users', createUser); +app.delete('/api/users/:id', deleteUser); // REMOVED! + +// AFTER +app.get('/api/users', listUsers); +app.get('/api/users/:id', getUser); +app.post('/api/users', createUser); +// DELETE endpoint removed - clients will get 404! +``` + +**Removed Event Handlers** +```typescript +// BEFORE +eventBus.on('user.created', sendWelcomeEmail); +eventBus.on('user.created', syncToAnalytics); +eventBus.on('order.completed', updateInventory); + +// AFTER +eventBus.on('user.created', sendWelcomeEmail); +// syncToAnalytics handler REMOVED - analytics will be incomplete! +// updateInventory handler REMOVED - inventory won't update! +``` + +### 2. Broken Behavior + +**Changed Return Types** +```typescript +// BEFORE +async function getUser(id: string): Promise { + return user; +} + +// AFTER: Return type changed (BREAKING!) +async function getUser(id: string): Promise { + return user ?? null; +} + +// All callers assuming non-null will break: +const user = await getUser(id); +console.log(user.name); // Potential null dereference! +``` + +**Changed Side Effects** +```typescript +// BEFORE: Function logs and emits event +function processOrder(order: Order): ProcessedOrder { + logger.info('Processing order', { orderId: order.id }); + const result = doProcessing(order); + events.emit('order.processed', result); + return result; +} + +// AFTER: Side effects removed (REGRESSION!) +function processOrder(order: Order): ProcessedOrder { + const result = doProcessing(order); + return result; + // No logging - harder to debug production issues! + // No event - downstream systems won't be notified! +} +``` + +**Changed Default Values** +```typescript +// BEFORE +interface Options { + timeout?: number; // default: 5000 + retries?: number; // default: 3 +} + +function fetch(url: string, options: Options = { timeout: 5000, retries: 3 }) { } + +// AFTER: Defaults changed (REGRESSION!) +function fetch(url: string, options: Options = { timeout: 1000, retries: 1 }) { } +// Existing code relying on 5s timeout may start failing! +``` + +**Changed Error Handling** +```typescript +// BEFORE: Throws specific error +async function authenticate(credentials: Credentials): Promise { + if (!valid) throw new AuthenticationError('Invalid credentials'); + return user; +} + +// AFTER: Returns null instead (BREAKING!) +async function authenticate(credentials: Credentials): Promise { + if (!valid) return null; // Callers catching AuthenticationError will miss this! + return user; +} +``` + +### 3. Intent vs Reality Mismatch + +**Commit Says X, Code Does Y** +```typescript +// Commit message: "Add retry logic to API calls" + +// ACTUAL CODE: No retry logic! +async function fetchData(): Promise { + const response = await api.get('/data'); + return response.data; +} + +// Expected: Retry on failure +// Reality: No retry implemented +``` + +**Partial Implementation** +```typescript +// Commit message: "Implement user preferences" + +// ACTUAL CODE: Only partial implementation +interface UserPreferences { + theme: 'light' | 'dark'; + language: string; + notifications: NotificationSettings; // Not implemented! + privacy: PrivacySettings; // Not implemented! +} + +function updatePreferences(prefs: Partial) { + if (prefs.theme) user.theme = prefs.theme; + if (prefs.language) user.language = prefs.language; + // notifications and privacy not handled! +} +``` + +### 4. Incomplete Migrations + +**Some Call Sites Updated, Others Not** +```typescript +// OLD API +function oldFunction(a: string, b: number): Result { } + +// NEW API +function newFunction(params: { a: string; b: number }): Result { } + +// PARTIALLY MIGRATED: +// file1.ts - Updated +const result = newFunction({ a: 'test', b: 42 }); + +// file2.ts - NOT updated (REGRESSION!) +const result = oldFunction('test', 42); // Still using old API! + +// file3.ts - NOT updated (REGRESSION!) +const result = oldFunction('other', 100); // Still using old API! +``` + +**Consumers Not Updated** +```typescript +// CHANGED: User model +interface User { + id: string; + email: string; + // name: string; // REMOVED + displayName: string; // ADDED (replacement) +} + +// CONSUMER NOT UPDATED (REGRESSION!) +function formatUserGreeting(user: User): string { + return `Hello, ${user.name}!`; // TypeScript error: 'name' doesn't exist +} +``` + +--- + +## Detection Techniques + +**Compare Exports Before/After:** +```bash +# List exports in main branch +git show main:src/index.ts | grep -E "^export" > /tmp/exports_before.txt + +# List exports in current branch +grep -E "^export" src/index.ts > /tmp/exports_after.txt + +# Find removed exports +diff /tmp/exports_before.txt /tmp/exports_after.txt | grep "^<" +``` + +**Find Removed Function Calls:** +```bash +# Check if function is still called +git show main:src/*.ts | grep "oldFunction" | wc -l # Before: 15 +grep -r "oldFunction" src/*.ts | wc -l # After: 3 (12 removed!) +``` + +**Verify All Consumers Updated:** +```bash +# Find all imports of changed module +grep -rn "from './changed-module'" --include="*.ts" + +# Check each importer for usage of changed exports +``` + +--- + +## Severity Guidelines + +**CRITICAL** - Breaking changes without migration: +- Public API exports removed +- Return types changed incompatibly +- Required parameters added +- Event emissions removed +- CLI options removed + +**HIGH** - Significant behavior changes: +- Default values changed +- Error handling changed +- Side effects removed +- Incomplete migrations + +**MEDIUM** - Moderate regression risk: +- Internal APIs changed +- Logging reduced +- Performance characteristics changed + +**LOW** - Minor concerns: +- Documentation drift +- Internal refactoring + +--- + +## Regression Checklist + +Before approving changes: + +- [ ] No exports removed without deprecation +- [ ] Return types backward compatible +- [ ] Default values unchanged (or documented) +- [ ] Side effects preserved (events, logging) +- [ ] All consumers of changed code updated +- [ ] Migration complete across codebase +- [ ] CLI options preserved or deprecated +- [ ] API endpoints preserved or versioned +- [ ] Commit message matches implementation +- [ ] Breaking changes documented in CHANGELOG + +--- + +## Comparison Commands + +```bash +# Compare file structure +diff <(git ls-tree -r --name-only main src/) <(git ls-tree -r --name-only HEAD src/) + +# Compare exports +diff <(git show main:src/index.ts | grep "^export") <(cat src/index.ts | grep "^export") + +# Find removed functions +git diff main...HEAD --stat | grep -E "^\s+-" | head -20 + +# Check for removed test files +git diff main...HEAD --name-status | grep "^D.*test" + +# Find TODO/FIXME additions (incomplete work?) +git diff main...HEAD | grep "^\+.*TODO\|^\+.*FIXME" +``` + diff --git a/skills/devflow-research/SKILL.md b/skills/devflow-research/SKILL.md index 926da8e4..b4a173af 100644 --- a/skills/devflow-research/SKILL.md +++ b/skills/devflow-research/SKILL.md @@ -95,7 +95,7 @@ After agent completes, summarize key findings: **Recommended Direction**: [suggested approach] **Trade-offs**: [key considerations] -**Next Step**: Run `/design` to create detailed implementation plan for chosen approach. +**Next Step**: Run `/implement` to execute the implementation lifecycle, or `/specify` for detailed requirements first. ``` ## Examples @@ -142,6 +142,6 @@ If not found ā†’ Launch Explore agent - **Exploration focus**: Explore agent explores approaches, not detailed planning - **Autonomous**: Auto-launches when research needed - **Clean context**: Main session stays focused on implementation -- **Next step guidance**: Suggests `/design` after exploration completes +- **Next step guidance**: Suggests `/implement` or `/specify` after exploration completes -This ensures thorough exploration happens in separate context while main session remains clean. For detailed implementation planning, use `/design` after exploration. +This ensures thorough exploration happens in separate context while main session remains clean. For detailed implementation, use `/implement` after exploration. diff --git a/skills/devflow-review-methodology/SKILL.md b/skills/devflow-review-methodology/SKILL.md index 668e4ec7..1c433704 100644 --- a/skills/devflow-review-methodology/SKILL.md +++ b/skills/devflow-review-methodology/SKILL.md @@ -1,6 +1,6 @@ --- name: devflow-review-methodology -description: Standard review process for all DevFlow review agents. Load when performing code reviews to ensure consistent 6-step process with 3-category issue classification. This is the shared methodology for SecurityReview, PerformanceReview, and all other review agents. +description: Standard review process for all DevFlow review agents. Load when performing code reviews to ensure consistent 6-step process with 3-category issue classification. This is the shared methodology used by the unified Reviewer agent across all focus areas. allowed-tools: Read, Grep, Glob, Bash --- @@ -355,17 +355,17 @@ echo "Review saved: $REPORT_FILE" ## Integration -This methodology is used by: -- **SecurityReview**: Security-focused analysis -- **PerformanceReview**: Performance-focused analysis -- **ArchitectureReview**: Architecture-focused analysis -- **TestsReview**: Test quality analysis -- **ConsistencyReview**: Code consistency analysis -- **ComplexityReview**: Complexity analysis -- **RegressionReview**: Regression detection -- **DependenciesReview**: Dependency analysis -- **DocumentationReview**: Documentation analysis -- **TypescriptReview**: TypeScript analysis -- **DatabaseReview**: Database analysis - -All review agents should load this skill and apply its methodology with their domain expertise. +This methodology is used by the **Reviewer** agent with different focus areas: +- `security` - Security-focused analysis (uses devflow-security-patterns) +- `performance` - Performance-focused analysis (uses devflow-performance-patterns) +- `architecture` - Architecture-focused analysis (uses devflow-architecture-patterns) +- `tests` - Test quality analysis (uses devflow-tests-patterns) +- `consistency` - Code consistency analysis (uses devflow-consistency-patterns) +- `complexity` - Complexity analysis (uses devflow-complexity-patterns) +- `regression` - Regression detection (uses devflow-regression-patterns) +- `dependencies` - Dependency analysis (uses devflow-dependencies-patterns) +- `documentation` - Documentation analysis (uses devflow-documentation-patterns) +- `typescript` - TypeScript analysis (uses devflow-typescript) +- `database` - Database analysis (uses devflow-database-patterns) + +The Reviewer agent loads all pattern skills and applies the relevant one based on the focus area specified in its invocation prompt. diff --git a/skills/devflow-security-patterns/SKILL.md b/skills/devflow-security-patterns/SKILL.md index 5ea9154d..1d7c15fc 100644 --- a/skills/devflow-security-patterns/SKILL.md +++ b/skills/devflow-security-patterns/SKILL.md @@ -1,6 +1,6 @@ --- name: devflow-security-patterns -description: Security vulnerability patterns and detection strategies. Load when reviewing code for security issues, implementing authentication/authorization, handling user input, or working with sensitive data. Used by SecurityReview agent. +description: Security vulnerability patterns and detection strategies. Load when reviewing code for security issues, implementing authentication/authorization, handling user input, or working with sensitive data. Used by Reviewer agent (security focus). allowed-tools: Read, Grep, Glob --- @@ -365,13 +365,3 @@ Map findings to OWASP Top 10 2021: | A09 | Logging Failures | Missing security logs, log injection | | A10 | SSRF | Unvalidated URLs in server requests | ---- - -## Integration - -This skill provides domain expertise for: -- **SecurityReview agent**: Use with `devflow-review-methodology` -- **Coder agent**: Reference when implementing auth, input handling -- **Code-smell skill**: Complements anti-pattern detection - -Load this skill when security analysis is needed. diff --git a/skills/devflow-self-review/SKILL.md b/skills/devflow-self-review/SKILL.md new file mode 100644 index 00000000..6f9f3629 --- /dev/null +++ b/skills/devflow-self-review/SKILL.md @@ -0,0 +1,488 @@ +--- +name: devflow-self-review +description: Self-review framework for Coder agent. Evaluate implementation against 9 pillars before returning. Fix P0/P1 issues immediately. Used via Stop hook to ensure quality before handoff. +allowed-tools: Read, Grep, Glob, Edit, Write, Bash +--- + +# Self-Review Framework + +Systematic self-review process for the Coder agent. Evaluate your implementation against 9 pillars before returning. **Fix issues, don't just report them.** + +Based on [Google Engineering Practices](https://google.github.io/eng-practices/review/reviewer/looking-for.html) and [Microsoft Engineering Playbook](https://microsoft.github.io/code-with-engineering-playbook/code-reviews/process-guidance/reviewer-guidance/). + +## Iron Law + +> **FIX BEFORE RETURNING** +> +> Self-review is not a report generator. It's a quality gate. If you find a P0 or P1 issue, +> you fix it. You only return when all critical issues are resolved. The goal is to catch +> your own mistakes before someone else has to. Pride in craftsmanship, not speed of delivery. + +## The 9 Pillars + +### Priority Levels + +| Priority | Action | Pillars | +|----------|--------|---------| +| **P0** | MUST fix before returning | Design, Functionality, Security | +| **P1** | SHOULD fix before returning | Complexity, Error Handling, Tests | +| **P2** | FIX if time permits | Naming, Consistency, Documentation | + +--- + +## P0 Pillars (MUST Fix) + +### 1. Design + +**Question**: Does the implementation fit the architecture? + +**Checklist**: +- [ ] Follows existing patterns in codebase +- [ ] Respects layer boundaries (controller/service/repository) +- [ ] Dependencies injected, not instantiated +- [ ] Not over-engineering (YAGNI) +- [ ] Not under-engineering (technical debt) +- [ ] Interactions with other components are sound + +**Red Flags**: +```typescript +// BAD: Direct database access in controller +class UserController { + async getUser(req, res) { + const user = await db.query('SELECT * FROM users WHERE id = ?', [req.params.id]); + } +} + +// BAD: God class doing everything +class ApplicationManager { + createUser() {} + processPayment() {} + sendEmail() {} + generateReport() {} + // 500 more methods... +} + +// BAD: Circular dependencies +// a.ts imports b.ts, b.ts imports a.ts +``` + +**Fix Pattern**: Refactor to match existing architecture. Extract responsibilities. Use dependency injection. + +--- + +### 2. Functionality + +**Question**: Does the code work as intended? + +**Checklist**: +- [ ] Happy path works correctly +- [ ] Edge cases handled (null, empty, boundary values) +- [ ] Error cases handled gracefully +- [ ] No race conditions in concurrent code +- [ ] No infinite loops or recursion without base case +- [ ] State mutations are intentional and correct + +**Red Flags**: +```typescript +// BAD: Missing null check +function getDisplayName(user: User) { + return user.profile.displayName; // user.profile could be undefined! +} + +// BAD: Race condition +let balance = await getBalance(); +if (balance >= amount) { + await withdraw(amount); // Balance could change between check and withdraw! +} + +// BAD: Off-by-one error +for (let i = 0; i <= array.length; i++) { // Should be < not <= + process(array[i]); +} +``` + +**Fix Pattern**: Add null checks, use transactions for atomic operations, verify loop bounds. + +--- + +### 3. Security + +**Question**: Are there security vulnerabilities? + +**Checklist**: +- [ ] No SQL/NoSQL injection +- [ ] No command injection +- [ ] No XSS vulnerabilities +- [ ] Input validated at boundaries +- [ ] No hardcoded secrets +- [ ] Authentication/authorization checked +- [ ] Sensitive data not logged + +**Red Flags**: +```typescript +// BAD: SQL injection +const query = `SELECT * FROM users WHERE email = '${email}'`; + +// BAD: Command injection +exec(`ls ${userInput}`); + +// BAD: Hardcoded secret +const API_KEY = 'sk-abc123xyz789'; + +// BAD: Missing auth check +app.delete('/api/users/:id', async (req, res) => { + await deleteUser(req.params.id); // No auth! +}); +``` + +**Fix Pattern**: Use parameterized queries, escape user input, use environment variables, add auth middleware. + +--- + +## P1 Pillars (SHOULD Fix) + +### 4. Complexity + +**Question**: Can a reader understand this in 5 minutes? + +**Checklist**: +- [ ] Functions are < 50 lines +- [ ] Nesting depth < 4 levels +- [ ] Cyclomatic complexity < 10 +- [ ] No magic numbers/strings +- [ ] Single responsibility per function +- [ ] Complex logic has explanatory comments + +**Red Flags**: +```typescript +// BAD: Deep nesting +if (a) { + if (b) { + if (c) { + if (d) { + if (e) { + // actual logic buried here + } + } + } + } +} + +// BAD: Magic numbers +setTimeout(callback, 86400000); +if (status === 3) { } +``` + +**Fix Pattern**: Extract functions, use early returns, define named constants. + +--- + +### 5. Error Handling + +**Question**: Are errors handled explicitly and consistently? + +**Checklist**: +- [ ] Errors are caught and handled appropriately +- [ ] Error messages are helpful (not generic) +- [ ] No silent failures (swallowed exceptions) +- [ ] Consistent error handling pattern (Result types or throws) +- [ ] Resources cleaned up in error paths +- [ ] Errors logged with context + +**Red Flags**: +```typescript +// BAD: Swallowed exception +try { + await riskyOperation(); +} catch (e) { + // silently ignored! +} + +// BAD: Generic error message +throw new Error('Something went wrong'); + +// BAD: Resource leak on error +const file = await openFile(path); +await processFile(file); // If this throws, file never closed! +await file.close(); +``` + +**Fix Pattern**: Always handle or rethrow errors, include context in messages, use try/finally for cleanup. + +--- + +### 6. Tests + +**Question**: Is the new functionality tested? + +**Checklist**: +- [ ] New code has corresponding tests +- [ ] Tests cover happy path +- [ ] Tests cover error cases +- [ ] Tests cover edge cases +- [ ] Tests are not brittle (test behavior, not implementation) +- [ ] Tests would fail if code breaks + +**Red Flags**: +```typescript +// BAD: No tests for new function +export function calculateDiscount(price, type) { + // 20 lines of logic with no tests +} + +// BAD: Test that doesn't verify behavior +it('creates user', async () => { + await createUser(data); + expect(mockDb.insert).toHaveBeenCalled(); // Only checks mock was called +}); + +// BAD: Missing edge case tests +describe('divide', () => { + it('divides numbers', () => { + expect(divide(10, 2)).toBe(5); + }); + // No test for divide by zero! +}); +``` + +**Fix Pattern**: Add tests for all new functions, test behavior not mocks, cover edge cases. + +--- + +## P2 Pillars (FIX if Time Permits) + +### 7. Naming + +**Question**: Are names clear and descriptive? + +**Checklist**: +- [ ] Variable names describe content +- [ ] Function names describe action +- [ ] No single-letter names (except loop indices) +- [ ] No abbreviations that aren't universal +- [ ] Consistent naming style (camelCase/snake_case) + +**Red Flags**: +```typescript +// BAD: Cryptic names +const d = new Date(); +const r = items.filter(i => i.t > d); +const x = r.reduce((a, b) => a + b.p, 0); + +// BAD: Misleading name +function getUser(id) { + return db.users.findAll(); // Returns ALL users, not one! +} +``` + +**Fix Pattern**: Rename to be descriptive and accurate. + +--- + +### 8. Consistency + +**Question**: Does this match existing patterns? + +**Checklist**: +- [ ] Follows existing code style +- [ ] Uses same patterns as surrounding code +- [ ] Error handling matches project conventions +- [ ] Import organization matches existing files +- [ ] No unnecessary divergence from norms + +**Red Flags**: +```typescript +// BAD: Different style than rest of codebase +// Existing code uses Result types +function existingFunction(): Result { } + +// Your code throws instead +function yourFunction(): User { + throw new Error('...'); // Inconsistent! +} +``` + +**Fix Pattern**: Match existing patterns, follow established conventions. + +--- + +### 9. Documentation + +**Question**: Will others understand this code? + +**Checklist**: +- [ ] Complex logic has explanatory comments +- [ ] Public APIs have JSDoc/docstrings +- [ ] README updated if behavior changes +- [ ] No outdated comments +- [ ] Comments explain "why", not "what" + +**Red Flags**: +```typescript +// BAD: Missing docs on complex function +export function calculateProratedBilling(plan, start, end, previous) { + // 50 lines of complex billing logic with no explanation +} + +// BAD: Outdated comment +// Returns user's full name +function getDisplayName(user) { + return user.username; // Actually returns username! +} +``` + +**Fix Pattern**: Add JSDoc to public APIs, explain complex algorithms, remove outdated comments. + +--- + +## Self-Review Process + +### Step 1: Gather Changes + +```bash +# List files you changed +git diff --name-only HEAD~1 + +# See the actual diff +git diff HEAD~1 +``` + +### Step 2: Evaluate Each Pillar + +For each pillar, ask the question and run through the checklist. + +**Scoring**: +- PASS: No issues found +- ISSUE: Problem identified (note file:line) + +### Step 3: Fix P0 Issues + +If any P0 pillar has issues: +1. Fix the issue immediately +2. Re-evaluate that pillar +3. Continue until PASS + +**If P0 issue is unfixable** (requires architectural change beyond scope): +- STOP +- Report blocker to orchestrator +- Do not proceed + +### Step 4: Fix P1 Issues + +If any P1 pillar has issues: +1. Fix the issue +2. Re-evaluate +3. Continue until PASS or time constraint + +### Step 5: Address P2 Issues + +If time permits: +1. Fix P2 issues +2. These are improvements, not blockers + +### Step 6: Generate Report + +```markdown +## Self-Review Report + +### P0 Pillars +- Design: PASS +- Functionality: PASS (fixed null check in user.ts:45) +- Security: PASS + +### P1 Pillars +- Complexity: PASS (extracted helper function) +- Error Handling: PASS +- Tests: PASS (added 3 test cases) + +### P2 Pillars +- Naming: PASS +- Consistency: PASS +- Documentation: PASS (added JSDoc) + +### Summary +All P0 and P1 issues resolved. Ready for external review. +``` + +--- + +## Decision Tree + +``` +START + ā”‚ + ā–¼ +ā”Œā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā” +ā”‚ Evaluate P0 ā”‚ +ā”‚ (Design, ā”‚ +ā”‚ Functionality, ā”‚ +ā”‚ Security) ā”‚ +ā””ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”¬ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”˜ + ā”‚ + Issues found? + ā”Œā”€ā”€ā”€ā”€ā”“ā”€ā”€ā”€ā”€ā” + YES NO + ā”‚ ā”‚ + ā–¼ ā”‚ +ā”Œā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā” ā”‚ +ā”‚ Fixable?ā”‚ ā”‚ +ā””ā”€ā”€ā”€ā”€ā”¬ā”€ā”€ā”€ā”€ā”˜ ā”‚ + ā”Œā”€ā”€ā”“ā”€ā”€ā” ā”‚ + YES NO ā”‚ + ā”‚ ā”‚ ā”‚ + ā–¼ ā–¼ ā”‚ + FIX STOP ā”‚ + ā”‚ REPORT ā”‚ + ā”‚ BLOCKER ā”‚ + ā”‚ ā”‚ + ā””ā”€ā”€ā”€ā”€ā”€ā”¬ā”€ā”€ā”€ā”€ā”€ā”˜ + ā”‚ + ā–¼ +ā”Œā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā” +ā”‚ Evaluate P1 ā”‚ +ā”‚ (Complexity, ā”‚ +ā”‚ Error Handling,ā”‚ +ā”‚ Tests) ā”‚ +ā””ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”¬ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”˜ + ā”‚ + Issues found? + ā”Œā”€ā”€ā”€ā”€ā”“ā”€ā”€ā”€ā”€ā” + YES NO + ā”‚ ā”‚ + ā–¼ ā”‚ + FIX ā”‚ + ā”‚ ā”‚ + ā””ā”€ā”€ā”€ā”€ā”¬ā”€ā”€ā”€ā”€ā”˜ + ā”‚ + ā–¼ +ā”Œā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā” +ā”‚ Evaluate P2 ā”‚ +ā”‚ (Naming, ā”‚ +ā”‚ Consistency, ā”‚ +ā”‚ Documentation) ā”‚ +ā””ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”¬ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”˜ + ā”‚ + Issues found? + ā”Œā”€ā”€ā”€ā”€ā”“ā”€ā”€ā”€ā”€ā” + YES NO + ā”‚ ā”‚ + ā–¼ ā”‚ + FIX IF ā”‚ + TIME ā”‚ + ā”‚ ā”‚ + ā””ā”€ā”€ā”€ā”€ā”¬ā”€ā”€ā”€ā”€ā”˜ + ā”‚ + ā–¼ + RETURN + WITH REPORT +``` + +--- + +## Integration + +This skill is used by: +- **Coder agent**: Via Stop hook before returning implementation +- **Stop hook prompt**: "Run self-review using devflow-self-review. Fix all P0/P1 issues. Return when PASS." + +The self-review ensures implementations meet quality standards before external review, reducing review cycles and catching issues early. diff --git a/skills/devflow-tests-patterns/SKILL.md b/skills/devflow-tests-patterns/SKILL.md new file mode 100644 index 00000000..fccd5b7d --- /dev/null +++ b/skills/devflow-tests-patterns/SKILL.md @@ -0,0 +1,396 @@ +--- +name: devflow-tests-patterns +description: Test quality, coverage, and effectiveness analysis. Load when reviewing test code for coverage gaps, brittle tests, or poor test design. Used by Reviewer agent with tests focus. +allowed-tools: Read, Grep, Glob +--- + +# Tests Patterns + +Domain expertise for test quality and effectiveness analysis. Use alongside `devflow-review-methodology` for complete test reviews. + +## Iron Law + +> **TESTS VALIDATE BEHAVIOR, NOT IMPLEMENTATION** +> +> A test should fail when behavior breaks, not when implementation changes. If refactoring +> breaks tests without changing behavior, the tests are wrong. Mock boundaries, not internals. +> Test the contract, not the code. If tests are hard to write, the design is wrong. + +## Test Categories + +### 1. Coverage Issues + +**Untested New Code** +```typescript +// NEW CODE without tests +export function calculateDiscount(price: number, type: CustomerType): number { + if (type === 'premium') return price * 0.2; + if (type === 'vip') return price * 0.3; + return price * 0.1; +} + +// REQUIRED: Tests for each branch +describe('calculateDiscount', () => { + it('returns 10% for regular customers', () => { + expect(calculateDiscount(100, 'regular')).toBe(10); + }); + + it('returns 20% for premium customers', () => { + expect(calculateDiscount(100, 'premium')).toBe(20); + }); + + it('returns 30% for VIP customers', () => { + expect(calculateDiscount(100, 'vip')).toBe(30); + }); +}); +``` + +**Missing Edge Cases** +```typescript +// CODE +function divide(a: number, b: number): number { + return a / b; +} + +// INCOMPLETE TESTS +describe('divide', () => { + it('divides two numbers', () => { + expect(divide(10, 2)).toBe(5); // Happy path only + }); +}); + +// COMPLETE TESTS +describe('divide', () => { + it('divides two positive numbers', () => { + expect(divide(10, 2)).toBe(5); + }); + + it('handles division by zero', () => { + expect(divide(10, 0)).toBe(Infinity); + }); + + it('handles negative numbers', () => { + expect(divide(-10, 2)).toBe(-5); + }); + + it('handles decimal results', () => { + expect(divide(10, 3)).toBeCloseTo(3.333, 2); + }); +}); +``` + +**No Error Path Tests** +```typescript +// CODE +async function fetchUser(id: string): Promise { + const response = await api.get(`/users/${id}`); + if (!response.ok) throw new ApiError(response.status); + return response.json(); +} + +// INCOMPLETE: Only tests success +describe('fetchUser', () => { + it('returns user data', async () => { + api.get.mockResolvedValue({ ok: true, json: () => mockUser }); + const user = await fetchUser('123'); + expect(user).toEqual(mockUser); + }); +}); + +// COMPLETE: Tests error paths +describe('fetchUser', () => { + it('returns user data on success', async () => { /* ... */ }); + + it('throws ApiError on 404', async () => { + api.get.mockResolvedValue({ ok: false, status: 404 }); + await expect(fetchUser('123')).rejects.toThrow(ApiError); + }); + + it('throws ApiError on 500', async () => { + api.get.mockResolvedValue({ ok: false, status: 500 }); + await expect(fetchUser('123')).rejects.toThrow(ApiError); + }); + + it('handles network errors', async () => { + api.get.mockRejectedValue(new Error('Network error')); + await expect(fetchUser('123')).rejects.toThrow('Network error'); + }); +}); +``` + +### 2. Test Quality Issues + +**Brittle Tests (Implementation Coupling)** +```typescript +// BRITTLE: Tests implementation details +describe('UserService', () => { + it('creates user', async () => { + const service = new UserService(mockRepo); + await service.create({ name: 'John' }); + + // Testing HOW, not WHAT + expect(mockRepo.beginTransaction).toHaveBeenCalled(); + expect(mockRepo.insert).toHaveBeenCalledWith('users', { name: 'John' }); + expect(mockRepo.commit).toHaveBeenCalled(); + }); +}); + +// ROBUST: Tests behavior/outcome +describe('UserService', () => { + it('creates user and returns it', async () => { + const service = new UserService(mockRepo); + const user = await service.create({ name: 'John' }); + + // Testing WHAT the behavior is + expect(user.name).toBe('John'); + expect(user.id).toBeDefined(); + }); + + it('persists user to repository', async () => { + const service = new UserService(mockRepo); + await service.create({ name: 'John' }); + + const saved = await mockRepo.findByName('John'); + expect(saved).toBeDefined(); + }); +}); +``` + +**Unclear Test Names** +```typescript +// UNCLEAR: What does this test? +describe('User', () => { + it('test1', () => { /* ... */ }); + it('should work', () => { /* ... */ }); + it('handles edge case', () => { /* ... */ }); +}); + +// CLEAR: Describes behavior +describe('User', () => { + it('validates email format on creation', () => { /* ... */ }); + it('rejects passwords shorter than 8 characters', () => { /* ... */ }); + it('hashes password before storing', () => { /* ... */ }); +}); + +// PATTERN: "it [action] when [condition]" or "it [expected behavior]" +``` + +**Missing Arrange-Act-Assert** +```typescript +// MESSY: Mixed setup, action, assertion +it('processes order', async () => { + const user = await createUser(); + expect(user.id).toBeDefined(); + const order = await createOrder(user.id, [{ product: 'A', qty: 2 }]); + expect(order.status).toBe('pending'); + await processOrder(order.id); + const updated = await getOrder(order.id); + expect(updated.status).toBe('processed'); + expect(updated.processedAt).toBeDefined(); +}); + +// CLEAN: Clear AAA structure +it('marks order as processed with timestamp', async () => { + // Arrange + const user = await createUser(); + const order = await createOrder(user.id, [{ product: 'A', qty: 2 }]); + + // Act + await processOrder(order.id); + + // Assert + const updated = await getOrder(order.id); + expect(updated.status).toBe('processed'); + expect(updated.processedAt).toBeDefined(); +}); +``` + +### 3. Test Design Issues + +**Slow Tests** +```typescript +// SLOW: Real delays +it('retries on failure', async () => { + api.mockRejectedValueOnce(new Error('fail')); + api.mockResolvedValue({ data: 'ok' }); + + const result = await fetchWithRetry(); // Waits real 1000ms + + expect(result).toBe('ok'); +}, 5000); + +// FAST: Mock timers +it('retries on failure', async () => { + jest.useFakeTimers(); + api.mockRejectedValueOnce(new Error('fail')); + api.mockResolvedValue({ data: 'ok' }); + + const promise = fetchWithRetry(); + jest.advanceTimersByTime(1000); + const result = await promise; + + expect(result).toBe('ok'); +}); +``` + +**Flaky Tests** +```typescript +// FLAKY: Depends on timing +it('updates in real-time', async () => { + subscribe(callback); + emit('update', { value: 1 }); + + // Race condition: callback might not have fired yet + expect(callback).toHaveBeenCalled(); +}); + +// STABLE: Explicit waiting +it('updates in real-time', async () => { + const received = new Promise(resolve => { + subscribe(data => resolve(data)); + }); + + emit('update', { value: 1 }); + + const data = await received; + expect(data.value).toBe(1); +}); +``` + +**Poor Assertions** +```typescript +// WEAK: Doesn't verify much +it('returns users', async () => { + const users = await getUsers(); + expect(users).toBeDefined(); // Could be empty array, wrong shape, etc. +}); + +// STRONG: Specific assertions +it('returns array of users with required fields', async () => { + const users = await getUsers(); + + expect(users).toHaveLength(3); + expect(users[0]).toMatchObject({ + id: expect.any(String), + email: expect.stringContaining('@'), + createdAt: expect.any(Date), + }); +}); +``` + +### 4. Mocking Issues + +**Over-Mocking** +```typescript +// OVER-MOCKED: Tests nothing real +it('creates user', async () => { + const mockValidator = { validate: jest.fn().mockReturnValue(true) }; + const mockHasher = { hash: jest.fn().mockReturnValue('hashed') }; + const mockRepo = { create: jest.fn().mockResolvedValue({ id: '1' }) }; + const mockEvents = { emit: jest.fn() }; + + const service = new UserService(mockValidator, mockHasher, mockRepo, mockEvents); + await service.create({ email: 'test@test.com', password: 'pass' }); + + // What did we actually test? Just that mocks were called. +}); + +// BETTER: Use real implementations where possible +it('creates user with hashed password', async () => { + const repo = new InMemoryUserRepo(); + const service = new UserService( + new RealValidator(), + new RealHasher(), + repo, + new FakeEvents() + ); + + await service.create({ email: 'test@test.com', password: 'password123' }); + + const saved = await repo.findByEmail('test@test.com'); + expect(saved.password).not.toBe('password123'); // Actually hashed + expect(await bcrypt.compare('password123', saved.password)).toBe(true); +}); +``` + +**Mocking What You Don't Own** +```typescript +// PROBLEM: Mocking third-party library internals +jest.mock('axios'); +axios.get.mockResolvedValue({ data: mockResponse }); + +// BETTER: Wrap in your own interface +interface HttpClient { + get(url: string): Promise; +} + +class AxiosHttpClient implements HttpClient { /* ... */ } +class MockHttpClient implements HttpClient { /* ... */ } + +// Test with MockHttpClient, production uses AxiosHttpClient +``` + +--- + +## Severity Guidelines + +**CRITICAL** - Tests provide false confidence: +- Tests pass but don't verify behavior +- Tests mock everything, test nothing +- Critical paths have no tests +- Tests test implementation, will break on refactor + +**HIGH** - Significant test quality issues: +- Missing error path coverage +- Flaky tests that sometimes fail +- Tests are extremely slow (>10s) +- Test names don't describe behavior + +**MEDIUM** - Moderate test concerns: +- Some edge cases missing +- Could use better assertions +- Test structure unclear +- Minor coverage gaps + +**LOW** - Minor improvements: +- Test organization could improve +- Could add a few more cases +- Naming could be clearer + +--- + +## Detection Patterns + +Search for these patterns in code: + +```bash +# Tests without assertions +grep -rn "it\(.*=>" --include="*.test.ts" -A20 | grep -B20 "expect" | grep -L "expect" + +# Weak assertions +grep -rn "toBeDefined\|toBeTruthy\|not.toBeNull" --include="*.test.ts" + +# Implementation testing (checking mock calls) +grep -rn "toHaveBeenCalledWith\|toHaveBeenCalled" --include="*.test.ts" | wc -l + +# Missing error tests +grep -rn "throw\|reject\|Error" --include="*.ts" | grep -v test | wc -l +# Compare with: +grep -rn "rejects.toThrow\|toThrow" --include="*.test.ts" | wc -l + +# Slow tests (long timeouts) +grep -rn "}, [0-9][0-9][0-9][0-9][0-9])" --include="*.test.ts" +``` + +--- + +## Test Coverage Guidelines + +| Code Type | Required Coverage | Test Type | +|-----------|-------------------|-----------| +| Business logic | 90%+ | Unit tests | +| API endpoints | 80%+ | Integration tests | +| UI components | 70%+ | Component tests | +| Utilities | 100% | Unit tests | +| Error paths | 100% | Unit tests | + From 91185ed3c53a9f79c097c9ef4609400477ec0d49 Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Sun, 11 Jan 2026 21:46:45 +0000 Subject: [PATCH 2/3] chore(cli): add missing pattern skills to verbose display list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Updates DEVFLOW_SKILLS in init.ts to include all 25 skills: - Added devflow-self-review - Added 10 review pattern skills (architecture, complexity, consistency, database, dependencies, documentation, performance, regression, security, tests) This is cosmetic only - installation already works correctly since it copies entire directories. šŸ¤– Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- src/cli/commands/init.ts | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/cli/commands/init.ts b/src/cli/commands/init.ts index 13f1dd35..2deaa7b8 100644 --- a/src/cli/commands/init.ts +++ b/src/cli/commands/init.ts @@ -98,7 +98,6 @@ const DEVFLOW_SKILLS: CommandDefinition[] = [ { name: 'devflow-review-methodology', description: '6-step review process' }, { name: 'devflow-docs-framework', description: 'Documentation conventions' }, { name: 'devflow-git-safety', description: 'Git operations & safety' }, - { name: 'devflow-security-patterns', description: 'Security vulnerability patterns' }, { name: 'devflow-implementation-patterns', description: 'CRUD, API, events, config' }, { name: 'devflow-codebase-navigation', description: 'Exploration & pattern discovery' }, // Tier 2: Specialized Skills (user-facing, auto-activate) @@ -108,9 +107,21 @@ const DEVFLOW_SKILLS: CommandDefinition[] = [ { name: 'devflow-debug', description: 'Systematic debugging (auto)' }, { name: 'devflow-input-validation', description: 'Boundary validation' }, { name: 'devflow-worktree', description: 'Parallel development isolation' }, + { name: 'devflow-self-review', description: '9-pillar self-review framework' }, // Tier 3: Domain-Specific Skills { name: 'devflow-typescript', description: 'TypeScript patterns & idioms' }, { name: 'devflow-react', description: 'React components & hooks' }, + // Review Pattern Skills (used by Reviewer agent) + { name: 'devflow-architecture-patterns', description: 'Architecture & design patterns' }, + { name: 'devflow-complexity-patterns', description: 'Complexity & maintainability' }, + { name: 'devflow-consistency-patterns', description: 'Code consistency & style' }, + { name: 'devflow-database-patterns', description: 'Database design & queries' }, + { name: 'devflow-dependencies-patterns', description: 'Dependency management' }, + { name: 'devflow-documentation-patterns', description: 'Documentation quality' }, + { name: 'devflow-performance-patterns', description: 'Performance optimization' }, + { name: 'devflow-regression-patterns', description: 'Regression detection' }, + { name: 'devflow-security-patterns', description: 'Security vulnerabilities' }, + { name: 'devflow-tests-patterns', description: 'Test quality & coverage' }, ]; /** From 04c735abe86e97d9bfafd4bc0d15f7b088d429c1 Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Sun, 11 Jan 2026 21:51:10 +0000 Subject: [PATCH 3/3] fix(cli): add missing pattern skills to uninstall cleanup list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The uninstall command's skill cleanup list was missing 10 pattern skills added in Architecture v3. This would leave orphaned skills after uninstalling DevFlow: - devflow-self-review - devflow-architecture-patterns - devflow-complexity-patterns - devflow-consistency-patterns - devflow-database-patterns - devflow-dependencies-patterns - devflow-documentation-patterns - devflow-performance-patterns - devflow-regression-patterns - devflow-security-patterns - devflow-tests-patterns šŸ¤– Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- src/cli/commands/uninstall.ts | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/cli/commands/uninstall.ts b/src/cli/commands/uninstall.ts index ffcf5a01..01cabc3c 100644 --- a/src/cli/commands/uninstall.ts +++ b/src/cli/commands/uninstall.ts @@ -153,7 +153,6 @@ export const uninstallCommand = new Command('uninstall') 'devflow-review-methodology', 'devflow-docs-framework', 'devflow-git-safety', - 'devflow-security-patterns', 'devflow-implementation-patterns', 'devflow-codebase-navigation', // Tier 2: Specialized Skills @@ -163,9 +162,21 @@ export const uninstallCommand = new Command('uninstall') 'devflow-debug', 'devflow-input-validation', 'devflow-worktree', + 'devflow-self-review', // Tier 3: Domain-Specific Skills 'devflow-typescript', 'devflow-react', + // Review Pattern Skills (used by Reviewer agent) + 'devflow-architecture-patterns', + 'devflow-complexity-patterns', + 'devflow-consistency-patterns', + 'devflow-database-patterns', + 'devflow-dependencies-patterns', + 'devflow-documentation-patterns', + 'devflow-performance-patterns', + 'devflow-regression-patterns', + 'devflow-security-patterns', + 'devflow-tests-patterns', // Deprecated (for cleanup of old installs) 'devflow-pattern-check', 'devflow-error-handling'