feat: Add comprehensive Copilot coding guidelines and code review#44
feat: Add comprehensive Copilot coding guidelines and code review#44rusty1968 wants to merge 5 commits intoOpenPRoT:mainfrom
Conversation
- Add detailed Copilot instructions with safety-first coding patterns - Define test code exceptions for ergonomic development while maintaining safety - Establish type-safe hardware register access requirements - Add comprehensive code review checklist and forbidden patterns reference - Create initial code review identifying 50+ violations in existing codebase Key guidelines: * Panic-free production code (no unwrap/expect/indexing) * Type-safe PAC-based hardware register access only * Named constants instead of magic numbers * Test code may use unwrap/indexing for ergonomics but not bypass hardware safety * Security-focused patterns for cryptographic operations The code review identifies critical violations in ECDSA/RSA modules using direct register access and systematic unsafe patterns throughout the codebase requiring immediate refactoring.
There was a problem hiding this comment.
Pull Request Overview
This PR establishes comprehensive Copilot coding guidelines for safety-critical embedded development, adding enforcement mechanisms for panic-free production code while maintaining test ergonomics. The guidelines emphasize type-safe hardware register access, proper error handling, and security-focused patterns for cryptographic operations.
Key changes:
- Implements panic-free production code enforcement via clippy lints with test exceptions
- Establishes type-safe PAC-based hardware register access requirements
- Creates comprehensive code review documentation identifying 50+ violations in existing codebase
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/lib.rs |
Adds conditional clippy lints to enforce panic-free patterns in production code only |
src/tests/mod.rs |
Allows ergonomic patterns (unwrap, indexing) in test code for developer productivity |
code-review.md |
Documents extensive violations of safety guidelines throughout existing codebase |
.github/workflows/copilot-instructions.md |
Defines complete coding guidelines with safety patterns and forbidden practices |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
code-review.md
Outdated
| // ❌ FORBIDDEN - Can panic on None | ||
| timing_config: self.timing_config.unwrap_or(TimingConfig { | ||
|
|
||
| // ✅ REQUIRED - Explicit handling | ||
| timing_config: self.timing_config.unwrap_or_else(|| TimingConfig::default()), |
There was a problem hiding this comment.
The 'forbidden' example shows unwrap_or() which actually cannot panic (it provides a default value). The 'required' alternative using unwrap_or_else() is not actually safer - both are panic-free. This example incorrectly categorizes safe code as forbidden.
code-review.md
Outdated
| self.buf.get_mut(idx).ok_or(CommonError::IndexOutOfBounds)? | ||
| ``` | ||
|
|
||
| #### `/src/tests/functional/i2c_test.rs` - Lines 67, 75, 77 |
There was a problem hiding this comment.
The 'required' solution still uses direct indexing (self.buffer[..data.len()]) after the bounds check, which contradicts the stated requirement to avoid direct indexing. The solution should use .get_mut() methods consistently.
code-review.md
Outdated
| 5. Use `#[cfg_attr(not(test), deny(clippy::unwrap_used, clippy::indexing_slicing))]` to prevent future violations (✅ **IMPLEMENTED**) | ||
|
|
||
| **Note**: The clippy lint enforcement has been implemented in `src/lib.rs` with conditional application to exclude test code: |
There was a problem hiding this comment.
The code block shows duplicated clippy lint configuration. The same lints are already shown as implemented in the actual src/lib.rs file changes, making this redundant documentation.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Added rationale for forbidden direct indexing after bounds checks, referencing copilot-instructions.md Provided unsafe block documentation template and clarified documentation requirements Clarified test code exceptions, emphasizing hardware and crypto safety rules Improved reviewer guidance with actionable references and examples
1 participant
Key guidelines:
The code review identifies critical violations in ECDSA/RSA modules using direct register access and systematic unsafe patterns throughout the codebase requiring immediate refactoring.