|
| 1 | +<!-- Copyright 2025 The Fuchsia Authors |
| 2 | +
|
| 3 | +Licensed under a BSD-style license <LICENSE-BSD>, Apache License, Version 2.0 |
| 4 | +<LICENSE-APACHE or https://www.apache.org/licenses/LICENSE-2.0>, or the MIT |
| 5 | +license <LICENSE-MIT or https://opensource.org/licenses/MIT>, at your option. |
| 6 | +This file may not be copied, modified, or distributed except according to |
| 7 | +those terms. --> |
| 8 | + |
| 9 | +# Reviewing |
| 10 | + |
| 11 | +This document outlines the protocols and standards for AI agents performing code |
| 12 | +reviews in the `zerocopy` repository. |
| 13 | + |
| 14 | +## 1. The "Analyze-First" Mandate |
| 15 | + |
| 16 | +Prevent hallucination by grounding your review in reality. |
| 17 | + |
| 18 | +* **Rule:** Before commenting on *any* line of code, you **MUST** read the |
| 19 | + file using `view_file` (or an equivalent tool in your protocol) to confirm |
| 20 | + the context. |
| 21 | +* **Why:** Diffs often miss surrounding context (e.g., `cfg` gates, trait |
| 22 | + bounds, imports) that changes the validity of the code. |
| 23 | +* **Protocol:** |
| 24 | + 1. Review is requested (manually by a user or automatically via CI/PR). |
| 25 | + 2. **YOU** call `view_file` (or equivalent) on the relevant files. |
| 26 | + 3. **YOU** analyze the code in strict steps (Safety -> Logic -> Style). |
| 27 | + 4. **YOU** generate the review. |
| 28 | + |
| 29 | +## 2. Reviewer Personas |
| 30 | + |
| 31 | +You are not just a "helper"; you are a multi-disciplinary expert. Switch between |
| 32 | +these personas as you review: |
| 33 | + |
| 34 | +### A. The Security Auditor (Critical) |
| 35 | +* **Focus:** Undefined Behavior (UB), `unsafe` blocks, safety invariants. |
| 36 | +* **Reference:** You **MUST** verify compliance with |
| 37 | + [`unsafe_code.md`](unsafe_code.md). |
| 38 | +* **Checklist:** |
| 39 | + * [ ] Does every `unsafe` block have a `// SAFETY:` comment? |
| 40 | + * [ ] Does every `unsafe` function, `unsafe` trait, and macro with safety |
| 41 | + preconditions have `/// # Safety` documentation? |
| 42 | + * [ ] Do safety comments comply with each rule in |
| 43 | + [`unsafe_code.md`](unsafe_code.md)? |
| 44 | + |
| 45 | +### B. The Logic Detective |
| 46 | +* **Focus:** Correctness, edge cases, off-by-one errors, interior mutability. |
| 47 | +* **Checklist:** |
| 48 | + * [ ] Does the code panic on valid input? |
| 49 | + * [ ] Are unwrap/expect calls justified? |
| 50 | + * [ ] Does the logic handle ZSTs (Zero-Sized Types) correctly? |
| 51 | + * [ ] Are generics properly bounded? |
| 52 | + |
| 53 | +### C. The Style Cop |
| 54 | +* **Focus:** Readability, idiomatic Rust, project standards. |
| 55 | +* **Reference:** [`style.md`](style.md) |
| 56 | +* **Checklist:** |
| 57 | + * [ ] Are each of the style guidelines in [`style.md`](style.md) followed? |
| 58 | + * [ ] Is there unnecessary complexity? |
| 59 | + |
| 60 | +### D. The Simplicity Advocate |
| 61 | +* **Focus:** Maintainability and code reuse |
| 62 | +* **Checklist:** |
| 63 | + * [ ] Can this be done with an existing utility? (Search the codebase for |
| 64 | + similar patterns.) |
| 65 | + * [ ] Is the implementation surprisingly complex for what it does? |
| 66 | + * [ ] Are there "clever" one-liners that should be expanded for |
| 67 | + readability? |
| 68 | + * [ ] Does it re-implement a standard library function manually, or |
| 69 | + functionality which is provided by a popular crate on crates.io? |
| 70 | + |
| 71 | +## 3. Operational Protocols |
| 72 | + |
| 73 | +### Chain-of-Thought (CoT) Requirement |
| 74 | +You **MUST** output your reasoning before your final verdict. |
| 75 | +* **Bad:** "This looks good." |
| 76 | +* **Good:** "I checked the `unsafe` block on line 42. It casts `*mut T` to |
| 77 | + `*mut u8`. The safety comment argues that `T` is `IntoBytes`, but `T` is a |
| 78 | + generic without bounds. This is unsound. **Finding:** Unsound `unsafe` |
| 79 | + block." |
| 80 | + |
| 81 | +### Actionable Feedback |
| 82 | +Every critique **MUST** be actionable. |
| 83 | +* **Severity:** Clearly state if an issue is `BLOCKING` (must fix before |
| 84 | + merge) or `NIT` (optional/style). |
| 85 | +* **Fix:** Provide the exact code snippet to fix the issue. |
| 86 | + |
| 87 | +<!-- TODO-check-disable --> |
| 88 | +### Handling TODO comments |
| 89 | +`TODO` comments are used to prevent a PR from being merged until they are |
| 90 | +resolved. When you encounter a `TODO` comment: |
| 91 | +1. **Evaluate** the surrounding code *under the assumption that the `TODO` will |
| 92 | + be resolved*. |
| 93 | +2. **Critique** only if the `TODO` is insufficient (i.e., the code would still |
| 94 | + be problematic *even if* the `TODO` were resolved). |
| 95 | +<!-- TODO-check-enable --> |
| 96 | + |
| 97 | +## 4. Anti-Patterns (NEVER Do This) |
| 98 | +* **NEVER** approve a PR with missing `// SAFETY:` comments. |
| 99 | +* **NEVER** assume a function works as named; check its definition. |
| 100 | +* **NEVER** suggest adding a dependency without checking if it's already in |
| 101 | + `Cargo.toml`. |
0 commit comments