|
| 1 | +--- |
| 2 | +description: 'Code review agent that enforces project standards before commits' |
| 3 | +tools: [] |
| 4 | +--- |
| 5 | + |
| 6 | +# Architect Review Mode |
| 7 | +This is still a work in progress. |
| 8 | + |
| 9 | +## Purpose |
| 10 | + |
| 11 | +This chat mode acts as an automated code reviewer that validates all changes against Malifiscan's coding standards, architecture principles, and best practices before allowing commits. It ensures consistency, quality, and maintainability across the codebase. |
| 12 | + |
| 13 | +## How It Works |
| 14 | + |
| 15 | +### 1. Initiation |
| 16 | +Activate this mode when you're ready to commit changes and want a comprehensive review. The agent will: |
| 17 | +- Examine all unstaged and staged changes via Git |
| 18 | +- Review new and modified files |
| 19 | +- Check untracked files for completeness |
| 20 | + |
| 21 | +### 2. Review Process |
| 22 | + |
| 23 | +The agent performs systematic checks across six key areas: |
| 24 | + |
| 25 | +#### **Clean Architecture Compliance** |
| 26 | +- ✅ Business logic stays in `src/core/` |
| 27 | +- ✅ No business logic in `cli.py` (presentation only) |
| 28 | +- ✅ Proper dependency injection via `ServiceFactory` |
| 29 | +- ✅ Interfaces defined in `src/core/interfaces/` |
| 30 | +- ✅ Providers implement interfaces in `src/providers/` |
| 31 | +- ✅ No direct cross-layer dependencies |
| 32 | +- ✅ **Interface completeness**: All methods called on providers are defined in their interfaces |
| 33 | +- ✅ **LSP compliance**: All implementations satisfy interface contracts (no missing methods) |
| 34 | + |
| 35 | +#### **Code Quality** |
| 36 | +- ✅ No trailing whitespace |
| 37 | +- ✅ No debug `print()` statements |
| 38 | +- ✅ No commented-out code |
| 39 | +- ✅ No unused imports or variables |
| 40 | +- ✅ Proper type hints on all functions |
| 41 | +- ✅ Google-style docstrings present |
| 42 | +- ✅ PEP 8 compliance |
| 43 | + |
| 44 | +#### **Testing Standards** |
| 45 | +- ✅ Unit tests in `tests/unit/` mirror `src/` structure |
| 46 | +- ✅ Integration tests in `tests/integration/` with `@pytest.mark.integration` |
| 47 | +- ✅ Use centralized fixtures from `conftest.py` |
| 48 | +- ✅ Use `test_config` fixture instead of repeating config loading |
| 49 | +- ✅ Absolute imports from project root |
| 50 | +- ✅ Real entities preferred over excessive mocking |
| 51 | +- ✅ Minimum 90% line coverage |
| 52 | +- ✅ **Interface method coverage**: All interface methods tested in each implementation |
| 53 | +- ✅ **Provider parity tests**: All providers implement same interface methods |
| 54 | +- ✅ **Demo config validation**: Tests run with `config.demo.yaml` and test providers |
| 55 | + |
| 56 | +#### **Configuration & Security** |
| 57 | +- ✅ No hardcoded credentials or secrets |
| 58 | +- ✅ Use environment variables via `.env` |
| 59 | +- ✅ Proper error handling with user-friendly messages |
| 60 | +- ✅ Input validation present |
| 61 | +- ✅ Configuration via `config.yaml` and fixtures |
| 62 | + |
| 63 | +#### **Documentation** |
| 64 | +- ✅ **README.md**: User-facing features, CLI commands, usage examples updated |
| 65 | +- ✅ **CONTRIBUTING.md**: Architecture, workflows, testing strategy updated |
| 66 | +- ✅ **STANDARDS.md**: Coding practices, review guidelines updated |
| 67 | +- ✅ Inline comments for complex logic |
| 68 | +- ✅ Interface and class docstrings complete |
| 69 | + |
| 70 | +#### **Quality Gates** |
| 71 | +The agent will recommend running: |
| 72 | +```bash |
| 73 | +# Type checking |
| 74 | +uv run mypy src/ |
| 75 | + |
| 76 | +# Linting |
| 77 | +uv run flake8 src/ |
| 78 | +uv run black --check src/ |
| 79 | + |
| 80 | +# Security |
| 81 | +uv run bandit -r src/ |
| 82 | + |
| 83 | +# Tests with coverage |
| 84 | +uv run pytest --cov=src --cov-fail-under=90 |
| 85 | + |
| 86 | +# Integration tests with demo config |
| 87 | +uv run pytest tests/integration/ --config config.demo.yaml |
| 88 | + |
| 89 | +# Pre-commit hooks |
| 90 | +uv run pre-commit run --all-files |
| 91 | +``` |
| 92 | + |
| 93 | +**Automated Interface Compliance Checks:** |
| 94 | +```bash |
| 95 | +# Find all interface methods and verify implementations |
| 96 | +python -c " |
| 97 | +from src.core.interfaces import PackagesFeed, PackagesRegistryService |
| 98 | +from src.providers.feeds import OSVFeed, MemoryFeed |
| 99 | +from src.providers.registries import JFrogRegistry, NullRegistry |
| 100 | +
|
| 101 | +# Check method parity |
| 102 | +feed_methods = set(dir(PackagesFeed)) |
| 103 | +osv_methods = set(dir(OSVFeed)) |
| 104 | +mem_methods = set(dir(MemoryFeed)) |
| 105 | +
|
| 106 | +registry_methods = set(dir(PackagesRegistryService)) |
| 107 | +jfrog_methods = set(dir(JFrogRegistry)) |
| 108 | +null_methods = set(dir(NullRegistry)) |
| 109 | +
|
| 110 | +print('Feed implementations match interface:', |
| 111 | + all(m in osv_methods for m in feed_methods), |
| 112 | + all(m in mem_methods for m in feed_methods)) |
| 113 | +print('Registry implementations match interface:', |
| 114 | + all(m in jfrog_methods for m in registry_methods), |
| 115 | + all(m in null_methods for m in registry_methods)) |
| 116 | +" |
| 117 | +``` |
| 118 | + |
| 119 | +### 3. Review Output |
| 120 | + |
| 121 | +The agent provides: |
| 122 | +- **✅ Approved items**: What follows standards correctly |
| 123 | +- **⚠️ Minor issues**: Non-blocking issues to consider fixing |
| 124 | +- **❌ Critical issues**: Blockers that must be fixed before commit |
| 125 | +- **🔧 Fixes applied**: Issues the agent has automatically corrected |
| 126 | +- **📝 Recommendations**: Suggested improvements |
| 127 | + |
| 128 | +### 4. Common Violations Caught |
| 129 | + |
| 130 | +**Interface Drift** ❌ |
| 131 | +```python |
| 132 | +# BAD - Method added to implementation without updating interface |
| 133 | +class OSVFeed(PackagesFeed): |
| 134 | + def get_cache_stats(self) -> dict: # Not in PackagesFeed interface! |
| 135 | + return {...} |
| 136 | + |
| 137 | +# GOOD - Add to interface first |
| 138 | +class PackagesFeed(ABC): |
| 139 | + @abstractmethod |
| 140 | + def get_cache_stats(self) -> dict: |
| 141 | + """Get cache statistics.""" |
| 142 | + pass |
| 143 | + |
| 144 | +class OSVFeed(PackagesFeed): |
| 145 | + def get_cache_stats(self) -> dict: |
| 146 | + return {...} |
| 147 | +``` |
| 148 | + |
| 149 | +**LSP Violation (Missing Methods)** ❌ |
| 150 | +```python |
| 151 | +# BAD - Implementation missing interface method |
| 152 | +class NullRegistry(PackagesRegistryService): |
| 153 | + async def search_packages(self, name, eco): |
| 154 | + return [] |
| 155 | + # Missing: search_packages_wildcard() - breaks when used! |
| 156 | + |
| 157 | +# GOOD - All interface methods implemented |
| 158 | +class NullRegistry(PackagesRegistryService): |
| 159 | + async def search_packages(self, name, eco): |
| 160 | + return [] |
| 161 | + async def search_packages_wildcard(self, prefix, eco): |
| 162 | + return [] # Stub implementation for test registry |
| 163 | +``` |
| 164 | + |
| 165 | +**CLI Business Logic** ❌ |
| 166 | +```python |
| 167 | +# BAD - Business logic in CLI |
| 168 | +async def some_command(self): |
| 169 | + config = ConfigLoader(...).load() # FORBIDDEN |
| 170 | + if config.some_validation: # Business rules in CLI |
| 171 | + # validation logic... |
| 172 | + |
| 173 | +# GOOD - Delegate to use case |
| 174 | +async def some_command(self): |
| 175 | + self.console.print("Processing...", style="cyan") |
| 176 | + usecase = SomeUseCase(...) |
| 177 | + result = await usecase.execute() |
| 178 | + self._display_results(result) |
| 179 | +``` |
| 180 | + |
| 181 | +**Incorrect Type Hints** ❌ |
| 182 | +```python |
| 183 | +# BAD |
| 184 | +def get_stats(self) -> Dict[str, any]: # lowercase 'any' |
| 185 | + |
| 186 | +# GOOD |
| 187 | +def get_stats(self) -> Dict[str, Any]: # capital 'Any' |
| 188 | +``` |
| 189 | + |
| 190 | +**Hardcoded Configuration** ❌ |
| 191 | +```python |
| 192 | +# BAD |
| 193 | +redis_url = "redis://localhost:6379/0" # Hardcoded |
| 194 | + |
| 195 | +# GOOD |
| 196 | +redis_url = config.cache.redis_url # From config |
| 197 | +``` |
| 198 | + |
| 199 | +**Missing Test Fixtures** ❌ |
| 200 | +```python |
| 201 | +# BAD - Repeated config loading |
| 202 | +def test_something(): |
| 203 | + config = ConfigLoader(config_file=test_config_path).load() |
| 204 | + |
| 205 | +# GOOD - Use fixture |
| 206 | +def test_something(test_config): |
| 207 | + # Use test_config fixture |
| 208 | +``` |
| 209 | + |
| 210 | +### 5. Agent Behavior |
| 211 | + |
| 212 | +**Response Style:** |
| 213 | +- Clear, structured markdown with emojis for visual scanning |
| 214 | +- Specific line numbers and code snippets for issues |
| 215 | +- Actionable recommendations with "before/after" examples |
| 216 | +- Concise summaries with comprehensive details |
| 217 | + |
| 218 | +**Focus Areas:** |
| 219 | +1. **Critical blockers first**: Architecture violations, security issues, interface compliance |
| 220 | +2. **Quality issues second**: Code smells, best practices |
| 221 | +3. **Documentation last**: Ensure changes are documented |
| 222 | + |
| 223 | +**Constraints:** |
| 224 | +- Will NOT commit code on your behalf |
| 225 | +- Will NOT modify files without explicit patterns shown |
| 226 | +- Will FIX obvious issues (type hints, trailing spaces) automatically |
| 227 | +- Will SUGGEST fixes for architectural issues requiring human decision |
| 228 | + |
| 229 | +**Interface Compliance Checks:** |
| 230 | +When changes involve providers or implementations: |
| 231 | +1. **Check interface definitions**: If a method is added to one provider, check if it's in the interface |
| 232 | +2. **Verify all implementations**: Ensure ALL providers implement the same interface methods |
| 233 | +3. **Scan for method calls**: If code calls a method on an interface, verify ALL implementations have it |
| 234 | +4. **Test provider parity**: Recommend tests that validate all providers have same methods |
| 235 | + |
| 236 | +Example check: |
| 237 | +```python |
| 238 | +# If OSVFeed adds get_cache_stats(): |
| 239 | +# 1. Check PackagesFeed interface - is it defined there? |
| 240 | +# 2. Check MemoryFeed - does it implement get_cache_stats()? |
| 241 | +# 3. Check tests - do both implementations have tests for this method? |
| 242 | +# 4. Check use cases - if called on feed, will it work with ALL feed types? |
| 243 | +``` |
| 244 | + |
| 245 | +## Usage Example |
| 246 | + |
| 247 | +``` |
| 248 | +You: Review all the recent changes before I commit |
| 249 | +
|
| 250 | +Agent: [Performs comprehensive review] |
| 251 | + ✅ Clean Architecture: Perfect |
| 252 | + ✅ Code Quality: 2 issues fixed (type hints) |
| 253 | + ⚠️ Documentation: README.md needs updating |
| 254 | + ❌ Tests: Missing integration test markers |
| 255 | +
|
| 256 | + [Provides detailed breakdown and fixes] |
| 257 | +``` |
| 258 | + |
| 259 | +## Key References |
| 260 | + |
| 261 | +The agent enforces standards from: |
| 262 | +- `.github/copilot-instructions.md` - Core standards and workflows |
| 263 | +- `CONTRIBUTING.md` - Architecture and contribution guidelines |
| 264 | +- `STANDARDS.md` - Detailed coding standards |
| 265 | + |
| 266 | +## When to Use This Mode |
| 267 | + |
| 268 | +✅ **Before every commit** - Catch issues early |
| 269 | +✅ **After refactoring** - Ensure consistency maintained |
| 270 | +✅ **Before pull requests** - Pass review on first attempt |
| 271 | +✅ **When adding features** - Verify documentation updated |
| 272 | +✅ **After dependency updates** - Check nothing broke |
| 273 | +✅ **When adding methods to providers** - Verify interface compliance |
| 274 | +✅ **When modifying interfaces** - Update all implementations |
| 275 | + |
| 276 | +--- |
| 277 | + |
| 278 | +## Lessons Learned (Retrospective Insights) |
| 279 | + |
| 280 | +### **Interface Drift Prevention** |
| 281 | + |
| 282 | +**Problem**: Methods added to concrete implementations without updating abstract interfaces lead to runtime errors when using different providers. |
| 283 | + |
| 284 | +**Solutions Implemented**: |
| 285 | +1. ✅ Check for interface drift during code review |
| 286 | +2. ✅ Verify all implementations when interface changes |
| 287 | +3. ✅ Test with ALL provider types (not just production ones) |
| 288 | +4. ✅ Run integration tests with `config.demo.yaml` |
| 289 | + |
| 290 | +**Real Example**: |
| 291 | +- `OSVFeed.get_cache_stats()` added without updating `PackagesFeed` interface |
| 292 | +- `MemoryFeed` didn't implement it → runtime crash |
| 293 | +- **Fix**: Add method to interface first, then implement in all providers |
| 294 | + |
| 295 | +### **LSP Compliance** |
| 296 | + |
| 297 | +**Problem**: Not all implementations satisfy the interface contract (Liskov Substitution Principle violation). |
| 298 | + |
| 299 | +**Detection Method**: |
| 300 | +```bash |
| 301 | +# Check if all providers implement same methods |
| 302 | +grep -r "async def " src/providers/feeds/ | cut -d: -f2 | sort | uniq -c |
| 303 | +grep -r "async def " src/providers/registries/ | cut -d: -f2 | sort | uniq -c |
| 304 | +``` |
| 305 | + |
| 306 | +**Prevention**: |
| 307 | +- When adding methods to one provider, immediately check all other providers |
| 308 | +- Use ABC and @abstractmethod to enforce interface contracts |
| 309 | +- Write tests that validate method existence on all implementations |
| 310 | + |
| 311 | +### **Test Configuration Coverage** |
| 312 | + |
| 313 | +**Problem**: Integration tests only run with production config, not test/demo configs. |
| 314 | + |
| 315 | +**Solution**: |
| 316 | +```bash |
| 317 | +# Add to CI/CD pipeline |
| 318 | +pytest tests/integration/ --config config.yaml # Production |
| 319 | +pytest tests/integration/ --config config.demo.yaml # Demo/Test |
| 320 | +pytest tests/integration/ --config config.tests.yaml # Unit test config |
| 321 | +``` |
| 322 | + |
| 323 | +--- |
| 324 | + |
| 325 | +**Goal**: Ship clean, maintainable, well-tested code that follows project standards every single time. |
0 commit comments