@@ -0,0 +1,94 @@
+---
+name: rust-reviewer
+description: Expert Rust code reviewer specializing in ownership, lifetimes, error handling, unsafe usage, and idiomatic patterns. Use for all Rust code changes. MUST BE USED for Rust projects.
+tools: ["Read", "Grep", "Glob", "Bash"]
+model: sonnet
+---
+
+You are a senior Rust code reviewer ensuring high standards of safety, idiomatic patterns, and performance.
+
+When invoked:
+1. Run `cargo check`, `cargo clippy -- -D warnings`, `cargo fmt --check`, and `cargo test` — if any fail, stop and report
+2. Run `git diff HEAD~1 -- '*.rs'` (or `git diff main...HEAD -- '*.rs'` for PR review) to see recent Rust file changes
+3. Focus on modified `.rs` files
+4. If the project has CI or merge requirements, note that review assumes a green CI and resolved merge conflicts where applicable; call out if the diff suggests otherwise.
+5. Begin review
+
+## Review Priorities
+
+### CRITICAL — Safety
+
+- **Unchecked `unwrap()`/`expect()`**: In production code paths — use `?` or handle explicitly
+- **Unsafe without justification**: Missing `// SAFETY:` comment documenting invariants
+- **SQL injection**: String interpolation in queries — use parameterized queries
+- **Command injection**: Unvalidated input in `std::process::Command`
+- **Path traversal**: User-controlled paths without canonicalization and prefix check
+- **Hardcoded secrets**: API keys, passwords, tokens in source
+- **Insecure deserialization**: Deserializing untrusted data without size/depth limits
+- **Use-after-free via raw pointers**: Unsafe pointer manipulation without lifetime guarantees
+
+### CRITICAL — Error Handling
+
+- **Silenced errors**: Using `let _ = result;` on `#[must_use]` types
+- **Missing error context**: `return Err(e)` without `.context()` or `.map_err()`
+- **Panic for recoverable errors**: `panic!()`, `todo!()`, `unreachable!()` in production paths
+- **`Box<dyn Error>` in libraries**: Use `thiserror` for typed errors instead
+
+### HIGH — Ownership and Lifetimes
+
+- **Unnecessary cloning**: `.clone()` to satisfy borrow checker without understanding the root cause
+- **String instead of &str**: Taking `String` when `&str` or `impl AsRef<str>` suffices
+- **Vec instead of slice**: Taking `Vec<T>` when `&[T]` suffices
+- **Missing `Cow`**: Allocating when `Cow<'_, str>` would avoid it
+- **Lifetime over-annotation**: Explicit lifetimes where elision rules apply
+
+### HIGH — Concurrency
+
+- **Blocking in async**: `std::thread::sleep`, `std::fs` in async context — use tokio equivalents
+- **Unbounded channels**: `mpsc::channel()`/`tokio::sync::mpsc::unbounded_channel()` need justification — prefer bounded channels (`tokio::sync::mpsc::channel(n)` in async, `sync_channel(n)` in sync)
+- **`Mutex` poisoning ignored**: Not handling `PoisonError` from `.lock()`
+- **Missing `Send`/`Sync` bounds**: Types shared across threads without proper bounds
+- **Deadlock patterns**: Nested lock acquisition without consistent ordering
+
+### HIGH — Code Quality
+
+- **Large functions**: Over 50 lines
+- **Deep nesting**: More than 4 levels
+- **Wildcard match on business enums**: `_ =>` hiding new variants
+- **Non-exhaustive matching**: Catch-all where explicit handling is needed
+- **Dead code**: Unused functions, imports, or variables
+
+### MEDIUM — Performance
+
+- **Unnecessary allocation**: `to_string()` / `to_owned()` in hot paths
+- **Repeated allocation in loops**: String or Vec creation inside loops
+- **Missing `with_capacity`**: `Vec::new()` when size is known — use `Vec::with_capacity(n)`
+- **Excessive cloning in iterators**: `.cloned()` / `.clone()` when borrowing suffices
+- **N+1 queries**: Database queries in loops
+
+### MEDIUM — Best Practices
+
+- **Clippy warnings unaddressed**: Suppressed with `#[allow]` without justification
+- **Missing `#[must_use]`**: On non-`must_use` return types where ignoring values is likely a bug
+- **Derive order**: Should follow `Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize`
+- **Public API without docs**: `pub` items missing `///` documentation
+- **`format!` for simple concatenation**: Use `push_str`, `concat!`, or `+` for simple cases
+
+## Diagnostic Commands
+
+```bash
+cargo clippy -- -D warnings
+cargo fmt --check
+cargo test
+if command -v cargo-audit >/dev/null; then cargo audit; else echo "cargo-audit not installed"; fi
+if command -v cargo-deny >/dev/null; then cargo deny check; else echo "cargo-deny not installed"; fi
+cargo build --release 2>&1 | head -50
+```
+
+## Approval Criteria
+
+- **Approve**: No CRITICAL or HIGH issues
+- **Warning**: MEDIUM issues only
+- **Block**: CRITICAL or HIGH issues found
+
+For detailed Rust code examples and anti-patterns, see `skill: rust-patterns`.
@@ -0,0 +1,70 @@
+---
+paths:
+  - "src/**"
+---
+
+# Architecture
+
+## Core Components
+
+**CLI Entry Point** (`src/main.rs`)
+- Reads a single PHP file path from `argv[1]`
+- Loads file contents into memory
+- Iterates over all registered rules from `all_rules()`, chaining each rule's output into the next
+- If final content differs from original, writes the file back
+- Prints timing report per rule and peak memory usage
+
+**Library Root** (`src/lib.rs`)
+- Trivial re-export of public modules (`reporter`, `rules`)
+
+**Rule System** (`src/rules/mod.rs`)
+- Defines the rule contract: `type RuleFn = fn(&str) -> Option<String>`
+  - Input: PHP source code as `&str`
+  - Output: `None` if no changes needed, `Some(modified_source)` if changed
+  - Rules are pure functions — no side effects, no state
+- `pub fn all_rules() -> Vec<(&'static str, RuleFn)>` — the rule registry
+  - Single place to register new rules
+  - Integration tests automatically discover and run all registered rules
+
+**Observability** (`src/reporter.rs`)
+- Measures wall-clock time per rule (via `std::time::Instant`)
+- Reports peak memory usage via `libc::getrusage()` (macOS/Linux only; returns 0 elsewhere)
+- Formats and prints timing line to stdout: `[INFO] Refactoring took X.XXms (peak memory: X.XMB) | rule1: Xms, rule2: Xms, ...`
+
+**PHP AST Parsing**
+- Uses `mago-syntax` (PHP ecosystem) to parse source into an AST
+- Allocates AST into `bumpalo` arena for zero-copy traversal
+- Rules walk the AST, collect byte offsets (from spans), then apply text edits back-to-front (to preserve offset validity)
+
+---
+
+## Current Rules
+
+### `src/rules/quality/add_final_keyword.rs`
+
+**What it does**: Adds `final` keyword to non-abstract, non-final PHP classes (prevents accidental subclassing)
+
+**Behavior**:
+- Parses PHP source into AST
+- Walks all class declarations and classes within namespaces
+- For each class: if it has no `final` or `abstract` modifiers, inserts `final ` before the `class` keyword
+- If class is `readonly`, inserts before `readonly` to produce `final readonly class Foo {}`
+- Applies insertions back-to-front to avoid offset shifting
+- Returns `None` if no changes; returns `Some(modified_source)` if changed
+
+**Skips**:
+- Abstract classes
+- Classes that already have `final`
+- Interfaces and traits (no `final` modifier)
+- Enums (PHP enums cannot be marked `final`)
+
+---
+
+## Quality Issues (High Priority)
+
+See `/code-quality-issues` skill for the full H1–H3 blockers, M1–M7 medium-priority, and L1–L6 low-priority list.
+
+Key high-priority items to address:
+- **H1**: `reporter.rs` unsafe block missing `// SAFETY:` comment
+- **H2**: `.unwrap()` in test fixtures lose error context
+- **H3**: Unnecessary `String` clone in `main.rs`
@@ -0,0 +1,172 @@
+---
+paths:
+  - "src/rules/**"
+---
+
+# PHP AST Implementation Patterns
+
+## Overview
+
+Rules transform PHP code by:
+1. Parsing source into an AST using `mago-syntax`
+2. Allocating the AST into a `bumpalo` arena (zero-copy, single-free)
+3. Walking the AST to collect byte offsets (from spans)
+4. Applying text edits back-to-front (to preserve offset validity)
+5. Returning `Some(modified_source)` or `None`
+
+---
+
+## Parsing & Allocation
+
+```rust
+use bumpalo::Bump;
+use mago_syntax::parser::Parser;
+
+pub fn apply(source: &str) -> Option<String> {
+    // Create arena for zero-copy AST allocation
+    let arena = Bump::new();
+
+    // Parse into AST
+    let mut parser = Parser::new(&arena, source.as_bytes());
+    let result = parser.parse();
+
+    // Handle parse errors gracefully — return None, don't panic
+    let ast = result.ok()?;
+
+    // ... rest of rule logic
+}
+```
+
+---
+
+## Walking the AST
+
+`mago-syntax` provides AST node types like:
+- `Statement` (enum of statement variants)
+- `Sequence<Statement>` (collection of statements)
+- `Class` (class declaration with modifiers, name, members)
+- `Namespace` (namespace wrapper around statements)
+
+Example: find all top-level and namespaced classes:
+
+```rust
+for statement in &ast.statements {
+    match statement {
+        Statement::Class(class) => {
+            // Process top-level class
+        }
+        Statement::Namespace(ns) => {
+            for inner_stmt in &ns.statements {
+                if let Statement::Class(class) = inner_stmt {
+                    // Process namespaced class
+                }
+            }
+        }
+        _ => {}
+    }
+}
+```
+
+---
+
+## Collecting Byte Offsets
+
+AST nodes have `.span` (a byte offset range). Collect all spans that need edits:
+
+```rust
+let mut edits: Vec<(usize, &'static str)> = vec![];
+
+// For each class that needs a `final` keyword
+for class in classes_needing_final {
+    let position = class.span.start; // byte offset
+    edits.push((position, "final "));
+}
+```
+
+**Important**: Spans are byte offsets, not character offsets. UTF-8 matters.
+
+---
+
+## Applying Text Edits Back-to-Front
+
+To avoid offset shifting, apply edits in reverse order (from end of file to start):
+
+```rust
+// Sort edits by position (descending)
+edits.sort_by(|a, b| b.0.cmp(&a.0));
+
+let mut result = source.to_string();
+
+for (pos, text) in edits {
+    result.insert_str(pos, text);
+}
+
+if result == source {
+    None  // No changes
+} else {
+    Some(result)
+}
+```
+
+**Why back-to-front?** Inserting at position 10 shifts all positions after 10 forward. By working backward, earlier positions remain valid.
+
+---
+
+## Common Patterns
+
+### Checking Modifiers
+
+```rust
+if class.modifiers.contains_final() {
+    // Already has `final`
+    return None;
+}
+
+if class.modifiers.contains_abstract() {
+    // Skip abstract classes
+    return None;
+}
+```
+
+### Handling `readonly` Classes
+
+If inserting before the `class` keyword, check for `readonly`:
+
+```rust
+let insert_before = if class.modifiers.contains_readonly() {
+    class.readonly_position
+} else {
+    class.class_position
+};
+
+edits.push((insert_before, "final "));
+```
+
+### Safe Byte Offset Handling
+
+Document UTF-8 assumptions. If casting `usize`, use `usize::try_from()`:
+
+```rust
+let pos: usize = usize::try_from(span.start)
+    .expect("span offset should fit in usize");
+```
+
+---
+
+## Error Handling
+
+- **Never panic on malformed input.** The rule runs on user code.
+- **Return `None` if AST walk fails or is incomplete.** Better to skip than crash.
+- **Log suspicious assumptions with comments.** Example: "assumes mago-syntax always sets span.start < span.end"
+
+---
+
+## Testing Your Rule
+
+See `/testing-patterns` for fixture format.
+
+Key test cases:
+- One positive fixture (shows transformation)
+- One no-op fixture per skip condition (abstract, final, readonly, interface, trait, enum)
+- Edge case: mixing `readonly` with other modifiers
+- Idempotency: running rule twice should be idempotent
@@ -0,0 +1,53 @@
+---
+paths:
+  - "src/rules/**"
+---
+
+# Rule Contract
+
+## Function Signature
+
+Every rule is a pure function:
+
+```rust
+type RuleFn = fn(&str) -> Option<String>
+```
+
+**Input**: PHP source code as `&str`
+**Output**:
+- `None` if no changes needed
+- `Some(modified_source)` if the rule transformed the code
+
+## Constraints
+
+### Pure Function
+- No global state
+- No side effects
+- Deterministic output — same input always produces same output
+- No panic on malformed input — either transform or return `None`
+
+### Return Semantics
+- Return `None` = "no changes needed, skip to next rule"
+- Return `Some(String)` = "I changed the code, pass this to next rule"
+- Never return an unchanged copy of the input; return `None` instead
+
+### Idempotency (Critical)
+**Applying a rule twice to the same file must produce the same output as applying it once.**
+
+This is essential for rule chaining. If rule A produces output, then rule B runs on it, then rule A runs again, the result must be identical to the first time rule A ran.
+
+Test this explicitly: if you write a fixture file, verify that running your rule twice produces the same result.
+
+### Registration
+
+Rules are registered in `src/rules/mod.rs` via the `all_rules()` function:
+
+```rust
+pub fn all_rules() -> Vec<(&'static str, RuleFn)> {
+    vec![
+        ("category/rule_name", category::rule_name::apply),
+    ]
+}
+```
+
+The key (first element) is used in timing reports and test discovery. Use forward slashes and snake_case.