-
Notifications
You must be signed in to change notification settings - Fork 48
Add AI code review customization files [Rebase & FF] #1460
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
makubacki
merged 3 commits into
OpenDevicePartnership:main
from
makubacki:add_ai_review_files
Apr 17, 2026
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| --- | ||
| name: 'Patina Code Reviewer' | ||
| description: 'Patina code reviewer - reviews code against project conventions using read-only tools' | ||
| tools: | ||
| - search | ||
| - read | ||
| handoffs: | ||
| - label: Implement Fixes | ||
| agent: agent | ||
| prompt: > | ||
| Based on the review findings above, implement the suggested fixes. | ||
| send: false | ||
| --- | ||
| # Patina Code Reviewer | ||
|
|
||
| You are a code reviewer for the Patina project. Review code changes against the | ||
| project conventions defined in [copilot-instructions.md](../copilot-instructions.md) | ||
| and provide constructive, specific feedback. | ||
|
|
||
| When reviewing, also consult the deeper documentation as needed: | ||
|
|
||
| - `docs/src/dev/` for development principles | ||
| - `docs/src/component/` for component conventions | ||
| - `docs/src/dxe_core/` for core subsystem patterns | ||
|
|
||
| ## Review Process | ||
|
|
||
| 1. Read the code under review carefully. | ||
| 2. Check each convention category systematically: module organization, safety, | ||
| error handling, component model, testing, documentation, UEFI semantics. | ||
| 3. For each finding, cite the specific convention and suggest a concrete fix. | ||
|
|
||
| ## Review Style | ||
|
|
||
| - Be specific - cite file paths and line numbers. | ||
| - Be constructive - explain why the convention exists. | ||
| - Be concise - one finding per issue. | ||
| - Prioritize: safety and correctness first, then convention violations, then style. | ||
| - If the code follows all conventions, say so briefly. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,194 @@ | ||
| # Patina Project Instructions | ||
|
|
||
| Patina is a Rust-based UEFI firmware project. It currently replaces the traditional C-based EDK II | ||
| DXE Core with a Rust implementation that introduces a component-based architecture with dependency | ||
| injection in addition to a general-purpose UEFI Rust SDK. | ||
|
|
||
| ## Project Structure | ||
|
|
||
| The workspace is organized into four top-level areas: | ||
|
|
||
| - `components/` - Feature components (ACPI, Advanced Logger, MM, Performance, SMBIOS, etc.) | ||
| - `core/` - Shared core internals (debugger, collections, CPU, dependency expressions, stack traces) | ||
| - `patina_dxe_core/` - Main DXE Core library that ties everything together | ||
| - `sdk/` - Public SDK (`patina` crate for Boot/Runtime Services, component infrastructure, `patina_ffs` for firmware | ||
| file system, `patina_macro` for proc-macros) | ||
|
|
||
| Crate dependency rules are strict. Components depend on `sdk/` only - never on each other or on `core/`. The SDK | ||
| depends only on generic external crates. Core crates may depend on each other and on the SDK. See | ||
| [code_organization.md](docs/src/dev/code_organization.md) for the full dependency matrix. | ||
|
|
||
| ## Build and Test Commands | ||
|
|
||
| All commands go through `cargo make`. Never run raw `cargo` commands. | ||
|
|
||
| - `cargo make check` - Type-check all code | ||
| - `cargo make fmt` - Format code (run after every edit) | ||
| - `cargo make clippy` - Lint with clippy | ||
| - `cargo make test` - Run all unit tests | ||
| - `cargo make all` - Full PR readiness (fmt-check, deny, cspell, clippy, build, test, coverage, doc) | ||
| - `cargo make build-x64` / `cargo make build-aarch64` - Build for UEFI targets | ||
| - `cargo make coverage` - Generate test coverage reports | ||
| - `cargo make doc` - Build documentation | ||
|
|
||
| See [Makefile.toml](Makefile.toml) for the complete command list. | ||
|
|
||
| ## Module Organization | ||
|
|
||
| - Never use `mod.rs` files. Use named module files (e.g., `my_module.rs`) instead. | ||
| - No public definitions directly in `lib.rs` - only public module declarations. | ||
|
makubacki marked this conversation as resolved.
|
||
| - Break implementations into logical files with clean namespaces. | ||
| - Crate naming: `patina_` prefix for public crates, `patina_internal_` for internal | ||
| crates, `_macro` suffix for proc-macro crates. | ||
|
|
||
| ```rust | ||
| // WRONG: using mod.rs | ||
| // src/memory/mod.rs | ||
|
|
||
| // CORRECT: using named module file | ||
| // src/memory.rs | ||
| ``` | ||
|
|
||
| See [component requirements](docs/src/component/requirements.md) for crate layout standards. | ||
|
|
||
| ## Safety Conventions | ||
|
|
||
| - Prefer `zerocopy` for binary data and memory layouts over manual unsafe pointer/slice | ||
| operations. `zerocopy` provides safe, zero-cost abstractions for interpreting byte | ||
| buffers as typed data. | ||
| - Minimize `unsafe` code. Constrain it within safe abstraction wrappers. | ||
| - Document preconditions, postconditions, and invariants for every `unsafe` block. | ||
| - Prefer fallible constructors returning `Result` over constructors that silently | ||
| handle invalid input or panic. | ||
| - Mark functions `unsafe` only when the caller must uphold a contract the function | ||
| itself cannot verify internally. If the function validates all inputs before the | ||
| unsafe operation, it can remain safe. | ||
|
|
||
| See [unsafe.md](docs/src/dev/principles/unsafe.md) for the full safety philosophy | ||
| (software safety vs. hardware safety distinctions). | ||
|
|
||
| ## Error Handling | ||
|
|
||
| - Prefer `Result` return types. Avoid panics in production code. | ||
| - At UEFI ABI boundaries (`extern "efiapi"` functions), use `efi::Status`. | ||
| - Internally, use domain-specific Rust error types with `Debug`, `Display`, and `Error` | ||
| trait implementations. | ||
| - Implement `From<>` conversions at error type boundaries to create clean error chains. | ||
| - Use `expect("descriptive message")` over bare `unwrap()`. Reserve `unwrap()` for | ||
| test code only. | ||
|
|
||
| See [error-handling.md](docs/src/dev/principles/error-handling.md) for error | ||
| propagation patterns and examples. | ||
|
|
||
| ## Component Model | ||
|
|
||
| Patina uses dependency injection through component entry point function signatures. | ||
| A component only executes when all its declared dependencies are available. | ||
|
|
||
| - Define components with the `#[component]` attribute macro on an `impl` block. | ||
| The impl must contain `fn entry_point(self, ...) -> Result<()>`. | ||
| - Register service implementations with `#[derive(IntoService)]` and `#[service(dyn Trait)]`. | ||
| - Param types: `Config<T>`, `ConfigMut<T>`, `Service<T>`, `Hob<T>`, `Commands`, | ||
| `Handle`, `StandardBootServices`, `StandardRuntimeServices`, `&Storage`/`&mut Storage`, | ||
| `Option<P>`, tuples. | ||
| - `ConfigMut<T>` components run first (config is unlocked); calling `lock()` makes the | ||
| value immutable and enables `Config<T>` components to execute. | ||
| - `Service<dyn Trait>` is preferred over `Service<ConcreteType>` for mockability. Use | ||
| a concrete wrapper struct only when generic method signatures are needed. | ||
| - Use the **stored dependencies** pattern: store all dependency references as fields in | ||
| the component struct. The entry point stores references; methods use them. | ||
|
|
||
| See [component interface](docs/src/component/interface.md) and | ||
| [getting started](docs/src/component/getting_started.md) for details and examples. | ||
|
|
||
| ## Mocking and Testing | ||
|
|
||
| - Target ≥80% code coverage. Patch coverage ≥80% is a required PR check. | ||
| - Write tests to cover critical logic and edge cases, not just to hit coverage numbers. | ||
| - Test naming: prefix with `test_<component_name>_` or `test_<service_name>_`. | ||
| - Prefer `mockall` for trait mocking in unit tests. Use `#[automock]` on traits to generate mocks. | ||
| - Use `pretty_assertions` for readable test diffs. | ||
|
|
||
| See [testing.md](docs/src/dev/testing.md) for the full testing strategy. | ||
|
|
||
| ## Trait Design | ||
|
|
||
| Traits serve as **abstraction points** - not code reuse mechanisms. Use crates for | ||
| code reuse (see [reuse.md](docs/src/dev/principles/reuse.md)). | ||
|
|
||
| - Define traits to represent swappable behavior (e.g., `BootServices`, `SerialIO`). | ||
| - Implementations can vary per platform without affecting consumers. | ||
| - Keep traits focused on a single responsibility (Interface Segregation). | ||
|
|
||
| See [abstractions.md](docs/src/dev/principles/abstractions.md) for the full trait | ||
| design philosophy. | ||
|
|
||
| ## Documentation Standards | ||
|
|
||
| - All public items must be documented. Set `#[deny(missing_docs)]` in component crates. | ||
| - Use `///` doc comments with these sections as appropriate: **Examples**, **Errors**, | ||
| **Safety**, **Panics**. | ||
| - Document **traits**, not their implementations. | ||
| - In most cases, do not add `# Arguments` or `# Returns` sections - the function signature | ||
| should be self-evident. In cases where there is unavoidable ambiguity, add these sections | ||
| to provide clarity. | ||
|
|
||
| See [documenting.md](docs/src/dev/documenting.md) for templates and style guides. | ||
|
|
||
| ## UEFI-Specific Guidelines | ||
|
|
||
| - Use `TplMutex` for synchronization in shared state. Do not use non-TPL-aware | ||
| primitives like `spin::Mutex`. Keep critical sections narrow. | ||
| - Do not use `TplMutex` for interior mutability on non-shared data. | ||
| - No memory allocation or deallocation after `ExitBootServices`. | ||
|
|
||
| See [synchronization.md](docs/src/dxe_core/synchronization.md), | ||
| [memory management](docs/src/dxe_core/memory_management.md), and | ||
| [platform requirements](docs/src/integrate/patina_dxe_core_requirements.md). | ||
|
|
||
| ## Hardware Access | ||
|
|
||
| ### Memory-Mapped I/O (MMIO) | ||
|
|
||
| Do not create Rust references (`&T` or `&mut T`) to MMIO space. The compiler may | ||
| insert spurious reads through references, which can clear interrupt status bits, pop | ||
| FIFO entries, or trigger other device side-effects. MMIO must be accessed exclusively | ||
| through raw pointers with volatile operations. | ||
|
|
||
| Patina uses the [`safe-mmio`](https://github.com/google/safe-mmio) crate for all MMIO | ||
| access. `safe-mmio` provides `UniqueMmioPointer<T>` which never creates a `&T` to device | ||
| memory. Its field wrapper types encode read side-effect semantics at the type level: | ||
|
|
||
| - `ReadPure<T>` / `ReadPureWrite<T>` — reads have no side-effects (shared access OK). | ||
| - `ReadOnly<T>` / `ReadWrite<T>` — reads have side-effects (require `&mut` access). | ||
| - `WriteOnly<T>` — write-only register. | ||
|
|
||
| See [mmio.md](docs/src/dev/hardware_access/mmio.md) for the full rationale, wrapper | ||
| reference table, and links to `safe-mmio` documentation. | ||
|
|
||
| ## Dependency Management | ||
|
|
||
| - All external dependencies are vetted via `cargo-deny` for license compatibility, | ||
| security advisories, and allowed sources. | ||
| - Use workspace-level dependency declarations in the root `Cargo.toml` for consistency. | ||
| - Evaluate new dependencies against the criteria in | ||
| [dependency-management.md](docs/src/dev/principles/dependency-management.md). | ||
|
|
||
| ## Formatting | ||
|
|
||
| Formatting is enforced by `rustfmt` via [rustfmt.toml](rustfmt.toml). Always run | ||
| `cargo make fmt` after editing code. | ||
|
|
||
| ## Common Anti-Patterns | ||
|
|
||
| Flag these patterns during review: | ||
|
|
||
| 1. Using `mod.rs` instead of named module files | ||
| 2. Raw slice/pointer manipulation where `zerocopy` would work | ||
| 3. Unnecessary `unsafe` without a safe abstraction wrapper | ||
| 4. `unwrap()` in production code (outside of tests) | ||
| 5. Cross-component dependencies (components must only depend on `sdk/`) | ||
| 6. Adding public type definitions directly in `lib.rs` | ||
| 7. Missing documentation on public items | ||
| 8. Using non-TPL-aware synchronization primitives for shared state | ||
| 9. Creating Rust references (`&T`/`&mut T`) to MMIO space instead of using `safe-mmio` | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| --- | ||
| name: 'Patina Component Conventions' | ||
| description: 'Coding conventions and patterns for Patina component crates' | ||
| applyTo: 'components/**' | ||
| --- | ||
| # Patina Component Conventions | ||
|
|
||
| These conventions apply to all crates under `components/`. For project-wide rules, | ||
| see [copilot-instructions.md](../copilot-instructions.md). | ||
|
|
||
| ## Component Architecture | ||
|
|
||
| Components use dependency injection through their entry point function signature. | ||
| The entry point declares what the component needs and the dispatcher provides it. | ||
|
|
||
| - Apply the `#[component]` attribute macro on an `impl` block for the component struct. | ||
| - The impl block must contain a method named `entry_point` that takes `self` and | ||
| returns `patina::error::Result<()>`. | ||
| - Parameters in the entry point define dependencies - the component will not execute | ||
| until all dependencies are satisfied. | ||
|
|
||
| ```rust | ||
| use patina::{ | ||
| component::{component, params::Config}, | ||
| error::Result, | ||
| }; | ||
|
|
||
| struct MyComponent; | ||
|
|
||
| #[component] | ||
| impl MyComponent { | ||
| fn entry_point(self, config: Config<MyConfig>) -> Result<()> { | ||
| // Component logic using injected dependencies | ||
| Ok(()) | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| Services are registered via `#[derive(IntoService)]` with a `#[service(dyn Trait)]` | ||
| attribute: | ||
|
|
||
| ```rust | ||
| use patina::component::service::IntoService; | ||
|
|
||
| #[derive(IntoService)] | ||
| #[service(dyn MyTrait)] | ||
| pub struct MyServiceImpl { | ||
| // ... | ||
| } | ||
| ``` | ||
|
|
||
| ## Stored Dependencies Pattern | ||
|
|
||
| Components must store all dependency references as fields in the component struct | ||
| rather than passing them through call chains. This avoids memory copies and enables | ||
| safe reuse in UEFI's memory-constrained environment. | ||
|
|
||
| - **Initialization phase:** Store all dependency references in struct fields. | ||
| - **Execution phase:** Use stored references via methods - no additional parameter | ||
| resolution needed. | ||
| - Use `Service<T>` for service references (these are `Clone` with static lifetimes). | ||
| - Use `Option<T>` for optional dependencies to handle unavailable services gracefully. | ||
|
|
||
| ## Parameter Semantics | ||
|
|
||
| See [component interface](../../docs/src/component/interface.md) for the full parameter | ||
| reference table, availability rules, and detailed descriptions of each parameter type. | ||
|
|
||
| ## Crate Layout | ||
|
|
||
| Follow the standardized layout from [component requirements](../../docs/src/component/requirements.md): | ||
|
|
||
| 1. No public definitions in `lib.rs` - only public module declarations. | ||
| 2. **Required:** `component` module containing the crate's component(s). | ||
| 3. **Optional modules** (as needed): | ||
| - `config` - Configuration types registered via `.with_config` | ||
| - `error` - Domain-specific error types for services | ||
| - `hob` - GUID HOB type definitions | ||
| - `service` - Service trait definitions and implementations | ||
|
|
||
| ## Component Dependencies | ||
|
|
||
| Components must only depend on crates in `sdk/` and generic external crates. Never | ||
| depend on other component crates or `core/` crates. This keeps components loosely | ||
| coupled and independently maintainable. | ||
|
|
||
| ## Testing Components | ||
|
|
||
| - Test names must be prefixed with `test_<component_name>_` (snake_case). | ||
| - Use `Config::mock(value)`, `Service::mock(impl)`, `Hob::mock(data)`, and | ||
| `Commands::mock()` for unit testing component entry points. | ||
| - Mock service traits with `#[automock]` from `mockall`. Use extension traits for | ||
| any utility methods that call base trait methods. | ||
|
|
||
| See [component interface](../../docs/src/component/interface.md) for the full | ||
| parameter specification and examples. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| --- | ||
| description: 'Review code changes against Patina project conventions and standards' | ||
| tools: | ||
| - search | ||
| - read | ||
|
makubacki marked this conversation as resolved.
|
||
| --- | ||
| # Patina Code Review | ||
|
|
||
| Review the provided code against Patina project conventions. For each issue found, | ||
| cite the specific convention being violated and suggest a concrete fix. | ||
|
|
||
| ## Review Checklist | ||
|
|
||
| Work through each category below. Report findings grouped by category. If a | ||
| category has no issues, skip it silently. Apply the conventions from | ||
| [copilot-instructions.md](../copilot-instructions.md). | ||
|
|
||
| ### 1. Module Organization | ||
|
|
||
| - No `mod.rs` files; no public definitions in `lib.rs`; correct crate naming. | ||
|
|
||
| ### 2. Safety | ||
|
|
||
| - Raw pointer/slice work where `zerocopy` would suffice. | ||
| - `unsafe` blocks missing documented invariants or missing safe wrappers. | ||
| - Functions marked `unsafe` that could validate inputs internally. | ||
|
|
||
| ### 3. Error Handling | ||
|
|
||
| - `unwrap()` in non-test code. | ||
|
makubacki marked this conversation as resolved.
|
||
| - Missing `Result` returns, missing `From<>` conversions at error boundaries. | ||
|
|
||
| ### 4. Component Model | ||
|
|
||
| - Correct use of dependency injection, stored dependencies pattern, and parameter | ||
| types (`Config`/`ConfigMut`/`Service<dyn Trait>`). | ||
| - Components depending on anything outside `sdk/`. | ||
|
|
||
| ### 5. Testing | ||
|
|
||
| - New logic missing tests; incorrect test naming. | ||
| - Default methods on `#[automock]` traits instead of extension traits. | ||
|
|
||
| ### 6. Documentation | ||
|
|
||
| - Public items missing doc comments. | ||
| - `unsafe` functions missing `# Safety`; fallible functions missing `# Errors`. | ||
|
|
||
| ### 7. UEFI-Specific | ||
|
|
||
| - Non-TPL-aware synchronization primitives. | ||
|
makubacki marked this conversation as resolved.
|
||
| - Possible allocation after `ExitBootServices`. | ||
|
|
||
| ## Output Format | ||
|
|
||
| For each finding, use this format: | ||
|
|
||
| **[Category] Issue title** | ||
| - **File:** `path/to/file.rs:line` | ||
| - **Convention:** Brief description of the rule being violated | ||
| - **Suggestion:** Concrete fix or code example | ||
|
|
||
| Conclude with a summary: total findings by category and an overall assessment. | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.