|
1 | 1 | # GitHub Copilot Instructions for Contrast AI SmartFix |
2 | 2 |
|
3 | | -This document provides guidelines for GitHub Copilot when working on the Contrast AI SmartFix project. |
4 | | - |
5 | | -## Core Development Principles |
6 | | - |
7 | | -### 1. Code Quality & Linting |
8 | | -- **All code changes MUST pass linting** - Use flake8 with the project's configuration (`.flake8`) |
9 | | -- Follow PEP 8 style guidelines strictly |
10 | | -- Maximum line length: 180 characters (as configured in `.flake8`) |
11 | | -- Use type hints for all function parameters and return values |
12 | | -- Ensure proper docstrings for all functions, classes, and modules |
13 | | - |
14 | | -### 2. Testing Requirements |
15 | | -- **New code MUST have unit tests** - No exceptions for new functionality |
16 | | -- **Existing code modifications MUST include tests** - When editing existing code, write comprehensive unit tests |
17 | | -- Use pytest as the testing framework |
18 | | -- Maintain minimum 80% code coverage for new code |
19 | | -- Tests should follow the Arrange-Act-Assert pattern |
20 | | -- Mock external dependencies appropriately (GitHub API, Contrast API, file system operations) |
21 | | - |
22 | | -### 3. Object-Oriented Design & Domain-Driven Design |
23 | | -- **New code MUST follow Object-Oriented principles** - Encapsulation, inheritance, polymorphism |
24 | | -- **Refactor existing procedural code to OOP** - When editing old code, take the opportunity to make it Object-Oriented |
25 | | -- Apply Domain-Driven Design (DDD) principles to identify and model domain objects: |
26 | | - |
27 | | -#### Domain Objects to Consider: |
28 | | -- **Vulnerability** - Represents security vulnerabilities with properties like severity, rule, UUID |
29 | | -- **Remediation** - Encapsulates the process of fixing a vulnerability |
30 | | -- **BuildResult** - Represents build execution results and status |
31 | | -- **GitRepository** - Manages git operations and repository state |
32 | | -- **PullRequest** - Represents GitHub pull request with metadata and operations |
33 | | -- **Agent** - Abstracts AI agent behavior (FixAgent, QAAgent) |
34 | | -- **TelemetryEvent** - Structured telemetry data collection |
35 | | -- **ConfigurationContext** - Environment and configuration management |
36 | | - |
37 | | -#### Design Patterns to Use: |
38 | | -- **Strategy Pattern** - For different coding agents (SmartFix vs External) |
39 | | -- **Factory Pattern** - For creating agents, configurations, and API clients |
40 | | -- **Repository Pattern** - For data access (GitHub, Contrast API) |
41 | | -- **Command Pattern** - For git operations and build commands |
42 | | -- **Observer Pattern** - For telemetry and logging events |
43 | | - |
44 | | -### 4. Error Handling & Resilience |
45 | | -- Use custom exception classes that inherit from appropriate base exceptions |
46 | | -- Implement proper logging at appropriate levels (DEBUG, INFO, WARNING, ERROR) |
47 | | -- Handle external API failures gracefully with retries where appropriate |
48 | | -- Validate inputs at boundaries (API responses, user inputs, configuration) |
49 | | - |
50 | | -### 5. Async/Await Best Practices |
51 | | -- Use async/await consistently for I/O operations |
52 | | -- Properly handle asyncio event loops and cleanup |
53 | | -- Use context managers for resource management |
54 | | -- Handle cancellation and timeout scenarios |
55 | | - |
56 | | -## File Organization & Architecture |
57 | | - |
58 | | -### 6. Module Structure |
59 | | -- Keep modules focused on single responsibilities |
60 | | -- Use dependency injection instead of global state where possible |
61 | | -- Separate domain logic from infrastructure concerns |
62 | | -- Group related functionality into cohesive modules |
63 | | - |
64 | | -### 7. Configuration Management |
65 | | -- Centralize configuration in the `Config` class |
66 | | -- Use environment variables with sensible defaults |
67 | | -- Validate configuration at startup |
68 | | -- Support testing configurations |
69 | | - |
70 | | -## Security & Performance |
71 | | - |
72 | | -### 8. Security Considerations |
73 | | -- Never log sensitive data (API keys, tokens) |
74 | | -- Sanitize user inputs and API responses |
75 | | -- Use secure defaults for all configurations |
76 | | -- Validate file paths to prevent directory traversal |
77 | | - |
78 | | -### 9. Performance Guidelines |
79 | | -- Use efficient data structures and algorithms |
80 | | -- Implement proper caching where beneficial |
81 | | -- Avoid blocking operations in async contexts |
82 | | -- Monitor memory usage for large operations |
83 | | - |
84 | | -## Documentation & Maintenance |
85 | | - |
86 | | -### 10. Code Documentation |
87 | | -- Write clear, concise docstrings for all public methods |
88 | | -- Include usage examples in docstrings for complex functions |
89 | | -- Document complex business logic with inline comments |
90 | | -- Keep README.md updated with architectural changes |
91 | | - |
92 | | -### 11. Git & Version Control |
93 | | -- Use descriptive commit messages following conventional commits |
94 | | -- Keep commits atomic and focused |
95 | | -- Write meaningful branch names |
96 | | -- Update version numbers appropriately |
97 | | - |
98 | | -## Testing Patterns |
99 | | - |
100 | | -### 12. Test Structure |
101 | | -```python |
102 | | -class TestClassName: |
103 | | - """Test class for ClassName functionality.""" |
104 | | - |
105 | | - def setup_method(self): |
106 | | - """Set up test fixtures before each test method.""" |
107 | | - pass |
108 | | - |
109 | | - def test_method_name_should_expected_behavior(self): |
110 | | - """Test that method_name produces expected behavior under specific conditions.""" |
111 | | - # Arrange |
112 | | - # Act |
113 | | - # Assert |
114 | | - pass |
115 | | -``` |
116 | | - |
117 | | -### 13. Mock Usage |
118 | | -- Mock external dependencies (APIs, file system, network) |
119 | | -- Use `unittest.mock.patch` appropriately |
120 | | -- Verify mock calls when testing side effects |
121 | | -- Use `pytest.fixture` for reusable test data |
122 | | - |
123 | | -## Legacy Code Refactoring |
124 | | - |
125 | | -### 14. When Editing Existing Code |
126 | | -- **Identify domain objects** - Look for data and behavior that belong together |
127 | | -- **Extract classes** - Convert function clusters into cohesive classes |
128 | | -- **Introduce interfaces** - Abstract complex dependencies |
129 | | -- **Add tests first** - Write tests for existing behavior before refactoring |
130 | | -- **Refactor incrementally** - Make small, safe changes |
131 | | - |
132 | | -### 15. Gradual Modernization |
133 | | -- Replace global variables with dependency injection |
134 | | -- Convert procedural code to class-based approaches |
135 | | -- Introduce type hints to existing functions |
136 | | -- Add comprehensive error handling |
137 | | - |
138 | | -## Example Refactoring Approach |
139 | | - |
140 | | -**Before (Procedural):** |
141 | | -```python |
142 | | -def process_vulnerability(vuln_data, repo_path, config): |
143 | | - # 50+ lines of mixed concerns |
144 | | - pass |
145 | | -``` |
146 | | - |
147 | | -**After (Object-Oriented):** |
148 | | -```python |
149 | | -class VulnerabilityProcessor: |
150 | | - def __init__(self, repository: GitRepository, agent_factory: AgentFactory): |
151 | | - self._repository = repository |
152 | | - self._agent_factory = agent_factory |
153 | | - |
154 | | - def process(self, vulnerability: Vulnerability) -> RemediationResult: |
155 | | - agent = self._agent_factory.create_fix_agent() |
156 | | - return agent.remediate(vulnerability, self._repository) |
157 | | -``` |
158 | | - |
159 | | -## Continuous Improvement |
160 | | - |
161 | | -- Regularly review and update these guidelines |
162 | | -- Incorporate learnings from code reviews |
163 | | -- Stay updated with Python and testing best practices |
164 | | -- Monitor code quality metrics and improve accordingly |
165 | | - |
166 | | ---- |
167 | | - |
168 | | -**Remember:** These guidelines ensure maintainable, testable, and robust code that follows industry best practices and domain-driven design principles. |
| 3 | +## FUNDAMENTAL RULE: Test-First Simplicity |
| 4 | + |
| 5 | +**EVERY change follows this pattern:** |
| 6 | +1. **Write tests FIRST** - Before writing any production code |
| 7 | +2. **Question every requirement** - Is this actually needed? (*Exception: When executing planned refactoring tasks from ai-dev/tasks.md*) |
| 8 | +3. **Delete unnecessary parts** - Remove complexity, don't add it |
| 9 | +4. **Make tests pass** - Simplest implementation that works |
| 10 | +5. **Look around and refactor downstream** - Fix related code that uses your changes |
| 11 | + |
| 12 | +## Working with Planned vs. Unplanned Work |
| 13 | + |
| 14 | +### Planned Tasks (ai-dev/tasks.md): |
| 15 | +- Follow the task plan - requirements pre-analyzed |
| 16 | +- Test each task's deliverables before implementing |
| 17 | +- Keep implementations simple even if architecture is complex |
| 18 | +- Question implementation details, not the overall plan |
| 19 | + |
| 20 | +### Unplanned Work: |
| 21 | +- Question everything - apply full YAGNI principles |
| 22 | +- Start minimal - build only what's needed right now |
| 23 | +- Delete unused code immediately |
| 24 | + |
| 25 | +## Core Workflow |
| 26 | + |
| 27 | +### Testing (DO THIS FIRST) |
| 28 | +- Write tests before production code - no exceptions |
| 29 | +- Run `./test/run_tests.sh` after every change |
| 30 | +- All tests must pass before moving on |
| 31 | +- Delete tests for deleted features |
| 32 | + |
| 33 | +### Complete Refactoring (Not Partial) |
| 34 | +- When you touch code, look around - what else uses this? |
| 35 | +- Follow the dependency chain - update ALL callers |
| 36 | +- Clean up as you go - remove dead imports, unused variables |
| 37 | + |
| 38 | +### Anti-Over-Engineering |
| 39 | +- YAGNI - don't build for imaginary future requirements |
| 40 | +- Question every class/method - can this be a simple function? |
| 41 | +- Delete unused code immediately |
| 42 | +- Favor simple functions over complex class hierarchies |
| 43 | + |
| 44 | +### Red Flags - Stop and Simplify |
| 45 | +- Classes with 1 method → use a function |
| 46 | +- Enums/constants nobody uses → delete them |
| 47 | +- More than 3 levels of abstraction → flatten it |
| 48 | +- Configuration for imaginary features → remove it |
| 49 | + |
| 50 | +## Code Standards |
| 51 | +- All changes must pass flake8 linting |
| 52 | +- Type hints for public interfaces |
| 53 | +- Max line length: 180 characters |
| 54 | +- Fix whitespace: `sed -i '' 's/[[:space:]]*$//' path/to/file.py` |
| 55 | + |
| 56 | +## File Management - CLEAN UP INTERMEDIATE FILES |
| 57 | +- **Never leave duplicate files** - If you create `file_clean.py`, `file_backup.py`, `file_new.py` while editing, DELETE the extras |
| 58 | +- **Check for duplicates before finishing** - Run `ls -la` to spot files with similar names |
| 59 | +- **One source of truth** - Each logical unit should have exactly one file |
| 60 | +- **Remove failed attempts** - If you mess up editing and start over, delete the corrupted version |
| 61 | +- **Common duplicate patterns to avoid:** |
| 62 | + - `test_something.py` and `test_something_clean.py` |
| 63 | + - `module.py` and `module_backup.py` |
| 64 | + - `config.py` and `config_new.py` |
| 65 | + |
| 66 | +## When Refactoring Existing Code |
| 67 | +1. **Write tests for current behavior first** - Capture existing functionality |
| 68 | +2. **Check for planned migration path** - Is there a tasks.md plan for this area? |
| 69 | +3. **For planned refactoring**: Follow the task sequence and dependencies |
| 70 | +4. **For unplanned refactoring**: Question what can be deleted - Unused code, over-complex abstractions |
| 71 | +5. **Refactor incrementally** - Small, safe changes with tests between each |
| 72 | +6. **Follow the chain** - Update ALL code that depends on your changes |
| 73 | +7. **Clean up afterwards** - Remove dead imports, fix related issues |
| 74 | + |
| 75 | +## Additional Red Flags - Stop and Simplify (Even in Planned Work) |
| 76 | +- **Complex class hierarchies** - Can the implementation be simpler? |
| 77 | +- **More than 3 levels of abstraction** - Flatten the implementation |
| 78 | +- **Circular dependencies** - Break them up during implementation |
| 79 | +- **Classes with 1 method** - Could this be a function instead? |
| 80 | +- **Enums or constants nobody uses** - Delete them even if planned |
| 81 | +- **Configuration for imaginary future features** - Remove it |
| 82 | + |
| 83 | +## Balancing Planning vs. Simplicity |
| 84 | +- **Respect the architecture plan** - Don't change overall structure during task execution |
| 85 | +- **Keep implementations simple** - Complex architecture ≠ complex implementation |
| 86 | +- **Suggest alternatives** - If you see over-engineering opportunities during implementation |
| 87 | +- **Focus on YAGNI for implementation details** - Don't add features not required by the current task |
| 88 | +- **Test thoroughly** - Especially important when following complex architectural plans |
0 commit comments