From ebd8c8c6fa262f52236b2832eb770999421fc485 Mon Sep 17 00:00:00 2001 From: Ronaldo Martins Date: Mon, 16 Mar 2026 17:34:25 -0300 Subject: [PATCH] feat(agents): add Rust language support (#523) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(agents): add Rust language support — reviewer, build resolver, patterns, and testing Add Rust-specific agents and skills following the established Go/Kotlin pattern: - agents/rust-reviewer.md: ownership, lifetimes, unsafe audit, clippy, error handling - agents/rust-build-resolver.md: cargo build errors, borrow checker, dependency resolution - skills/rust-patterns/SKILL.md: idiomatic Rust patterns and best practices - skills/rust-testing/SKILL.md: TDD, unit/integration/async/property-based testing * fix(agents): correct Rust examples for accuracy and consistency - unsafe fn: add inner unsafe {} block for Rust 2024 edition compliance - edition: update from 2021 to 2024 as current default - rstest: add missing fixture import - mockall: add missing predicate::eq import - concurrency: use sync_channel (bounded) and expect() over unwrap() to align with rust-reviewer's HIGH-priority review checks * fix(skills): correct compilation issues in Rust code examples - collect: add .copied() for &str iterator into String - tokio import: remove unused sleep, keep Duration - async test: add missing Duration import * fix(skills): move --no-fail-fast before test-binary args --no-fail-fast is a Cargo option, not a test binary flag. Placing it after -- forwards it to the test harness where it is unrecognized. * fix(agents): distinguish missing cargo-audit from real audit failures Check if cargo-audit is installed before running it, so actual vulnerability findings are not suppressed by the fallback message. * fix: address automated review findings across all Rust files - build-resolver: prefer scoped cargo update over full refresh - testing: add Cargo.toml bench config with harness = false for criterion - testing: condense TDD example to stay under 500-line limit - patterns: use expect() over unwrap() on JoinHandle for consistency - patterns: add explicit lifetime to unsafe FFI return reference - reviewer: replace misleading "string interpolation" with concrete alternatives * fix: align with CONTRIBUTING.md conventions - skills: rename "When to Activate" to "When to Use" per template - reviewer: add cargo check gate before starting review * fix(agents): guard cargo-audit and cargo-deny with availability checks Match the pattern used in rust-build-resolver to avoid command-not-found errors when optional tools are not installed. * fix: address second round of automated review findings - testing: split TDD example into separate code blocks to avoid duplicate fn definition in single block - build-resolver/reviewer: use if/then/else instead of && ... || chaining for cargo-audit/deny to avoid masking real failures - build-resolver: add MSRV caveat to edition upgrade guidance * feat: add Rust slash commands for build, review, and test Add commands/rust-build.md, commands/rust-review.md, and commands/rust-test.md to provide consistent user entrypoints matching the existing Go and Kotlin command patterns. * fix(commands): improve rust-build accuracy and tone - Restructure-first borrow fix example instead of clone-first - Realistic cargo test output format (per-test lines, not per-file) - Align "Parse Errors" step with actual resolver behavior - Prefer restructuring over cloning in common errors table * fix: address cubic-dev-ai review findings on commands - Gate review on all automated checks, not just cargo check - Use git diff HEAD~1 / git diff main...HEAD for PR file selection - Fix #[must_use] guidance: Result is already must_use by type - Remove error-masking fallback on cargo tree --duplicates * fix: address remaining review findings across all bots - Add rust-reviewer and rust-build-resolver to AGENTS.md registry - Update agent count from 16 to 18 - Mark parse_config doctest as no_run (body is todo!()) - Add "How It Works" section to both Rust skills - Replace cargo install with taiki-e/install-action in CI snippet - Trim tarpaulin section to stay under 500-line limit * fix(agents): align rust-reviewer invocation with command spec - Use git diff HEAD~1 / main...HEAD instead of bare git diff - Add cargo test as explicit step before review begins * fix(skills): address cubic review on patterns and testing - Remove Tokio-specific language from How It Works summary - Add cargo-llvm-cov install note in coverage section - Revert no_run on doctest examples (illustrative code, not compiled) * fix(skills): use expect on thread join for consistency Replace handle.join().unwrap() with .expect("worker thread panicked") to match the .expect("mutex poisoned") pattern used above. * fix(agents): gate review on all automated checks, not just cargo check Consolidate check/clippy/fmt/test into a single gate step that stops and reports if any fail, matching the command spec. * fix(skills): replace unwrap with expect in channel example Use .expect("receiver disconnected") on tx.send() for consistency with the .expect() convention used in all other concurrency examples. * fix: address final review round — OpenCode mirrors, counts, examples - Add .opencode/commands/rust-{build,review,test}.md mirrors - Add .opencode/prompts/agents/rust-{build-resolver,reviewer}.txt mirrors - Fix AGENTS.md count to 20 (add missing kotlin agents to table) - Fix review example: all checks pass (consistent with gate policy) - Replace should_panic doctest with is_err() (consistent with best practices) - Trim testing commands to stay at 500-line limit * fix: address cubic and greptile review on OpenCode files and agents - Fix crate::module import guidance (internal path, not Cargo.toml) - Add cargo fmt --check to verification steps - Fix TDD GREEN example to handle error path (validate(input)?) - Scope .context() guidance to anyhow/eyre application code - Update command count from 40 to 51 - Add tokio channel variants to unbounded channel warning - Preserve JoinError context in spawned task panic message * fix: stale command count, channel guidance, cargo tree fallback - Fix stale command count in Project Structure section (40→51) - Clarify unbounded channel rule: context-appropriate bounded alternatives - Remove dead cargo tree fallback (exits 0 even with no duplicates) - Sync OpenCode reviewer mirror with tokio channel coverage --- .opencode/commands/rust-build.md | 78 +++ .opencode/commands/rust-review.md | 65 +++ .opencode/commands/rust-test.md | 104 ++++ .../prompts/agents/rust-build-resolver.txt | 93 ++++ .opencode/prompts/agents/rust-reviewer.txt | 61 +++ AGENTS.md | 10 +- agents/rust-build-resolver.md | 148 ++++++ agents/rust-reviewer.md | 93 ++++ commands/rust-build.md | 187 +++++++ commands/rust-review.md | 142 +++++ commands/rust-test.md | 308 +++++++++++ skills/rust-patterns/SKILL.md | 500 ++++++++++++++++++ skills/rust-testing/SKILL.md | 500 ++++++++++++++++++ 13 files changed, 2286 insertions(+), 3 deletions(-) create mode 100644 .opencode/commands/rust-build.md create mode 100644 .opencode/commands/rust-review.md create mode 100644 .opencode/commands/rust-test.md create mode 100644 .opencode/prompts/agents/rust-build-resolver.txt create mode 100644 .opencode/prompts/agents/rust-reviewer.txt create mode 100644 agents/rust-build-resolver.md create mode 100644 agents/rust-reviewer.md create mode 100644 commands/rust-build.md create mode 100644 commands/rust-review.md create mode 100644 commands/rust-test.md create mode 100644 skills/rust-patterns/SKILL.md create mode 100644 skills/rust-testing/SKILL.md diff --git a/.opencode/commands/rust-build.md b/.opencode/commands/rust-build.md new file mode 100644 index 00000000..82a668df --- /dev/null +++ b/.opencode/commands/rust-build.md @@ -0,0 +1,78 @@ +--- +description: Fix Rust build errors and borrow checker issues +agent: rust-build-resolver +subtask: true +--- + +# Rust Build Command + +Fix Rust build, clippy, and dependency errors: $ARGUMENTS + +## Your Task + +1. **Run cargo check**: `cargo check 2>&1` +2. **Run cargo clippy**: `cargo clippy -- -D warnings 2>&1` +3. **Fix errors** one at a time +4. **Verify fixes** don't introduce new errors + +## Common Rust Errors + +### Borrow Checker +``` +cannot borrow `x` as mutable because it is also borrowed as immutable +``` +**Fix**: Restructure to end immutable borrow first; clone only if justified + +### Type Mismatch +``` +mismatched types: expected `T`, found `U` +``` +**Fix**: Add `.into()`, `as`, or explicit type conversion + +### Missing Import +``` +unresolved import `crate::module` +``` +**Fix**: Fix the `use` path or declare the module (add Cargo.toml deps only for external crates) + +### Lifetime Errors +``` +does not live long enough +``` +**Fix**: Use owned type or add lifetime annotation + +### Trait Not Implemented +``` +the trait `X` is not implemented for `Y` +``` +**Fix**: Add `#[derive(Trait)]` or implement manually + +## Fix Order + +1. **Build errors** - Code must compile +2. **Clippy warnings** - Fix suspicious constructs +3. **Formatting** - `cargo fmt` compliance + +## Build Commands + +```bash +cargo check 2>&1 +cargo clippy -- -D warnings 2>&1 +cargo fmt --check 2>&1 +cargo tree --duplicates +cargo test +``` + +## Verification + +After fixes: +```bash +cargo check # Should succeed +cargo clippy -- -D warnings # No warnings allowed +cargo fmt --check # Formatting should pass +cargo test # Tests should pass +``` + +--- + +**IMPORTANT**: Fix errors only. No refactoring, no improvements. Get the build green with minimal changes. diff --git a/.opencode/commands/rust-review.md b/.opencode/commands/rust-review.md new file mode 100644 index 00000000..ad9c8ece --- /dev/null +++ b/.opencode/commands/rust-review.md @@ -0,0 +1,65 @@ +--- +description: Rust code review for ownership, safety, and idiomatic patterns +agent: rust-reviewer +subtask: true +--- + +# Rust Review Command + +Review Rust code for idiomatic patterns and best practices: $ARGUMENTS + +## Your Task + +1. **Analyze Rust code** for idioms and patterns +2. **Check ownership** - borrowing, lifetimes, unnecessary clones +3. **Review error handling** - proper `?` propagation, no unwrap in production +4. **Verify safety** - unsafe usage, injection, secrets + +## Review Checklist + +### Safety (CRITICAL) +- [ ] No unchecked `unwrap()`/`expect()` in production paths +- [ ] `unsafe` blocks have `// SAFETY:` comments +- [ ] No SQL/command injection +- [ ] No hardcoded secrets + +### Ownership (HIGH) +- [ ] No unnecessary `.clone()` to satisfy borrow checker +- [ ] `&str` preferred over `String` in function parameters +- [ ] `&[T]` preferred over `Vec` in function parameters +- [ ] No excessive lifetime annotations where elision works + +### Error Handling (HIGH) +- [ ] Errors propagated with `?`; use `.context()` in `anyhow`/`eyre` application code +- [ ] No silenced errors (`let _ = result;`) +- [ ] `thiserror` for library errors, `anyhow` for applications + +### Concurrency (HIGH) +- [ ] No blocking in async context +- [ ] Bounded channels preferred +- [ ] `Mutex` poisoning handled +- [ ] `Send`/`Sync` bounds correct + +### Code Quality (MEDIUM) +- [ ] Functions under 50 lines +- [ ] No deep nesting (>4 levels) +- [ ] Exhaustive matching on business enums +- [ ] Clippy warnings addressed + +## Report Format + +### CRITICAL Issues +- [file:line] Issue description + Suggestion: How to fix + +### HIGH Issues +- [file:line] Issue description + Suggestion: How to fix + +### MEDIUM Issues +- [file:line] Issue description + Suggestion: How to fix + +--- + +**TIP**: Run `cargo clippy -- -D warnings` and `cargo fmt --check` for automated checks. diff --git a/.opencode/commands/rust-test.md b/.opencode/commands/rust-test.md new file mode 100644 index 00000000..a1f2dbd2 --- /dev/null +++ b/.opencode/commands/rust-test.md @@ -0,0 +1,104 @@ +--- +description: Rust TDD workflow with unit and property tests +agent: tdd-guide +subtask: true +--- + +# Rust Test Command + +Implement using Rust TDD methodology: $ARGUMENTS + +## Your Task + +Apply test-driven development with Rust idioms: + +1. **Define types** - Structs, enums, traits +2. **Write tests** - Unit tests in `#[cfg(test)]` modules +3. **Implement minimal code** - Pass the tests +4. **Check coverage** - Target 80%+ + +## TDD Cycle for Rust + +### Step 1: Define Interface +```rust +pub struct Input { + // fields +} + +pub fn process(input: &Input) -> Result { + todo!() +} +``` + +### Step 2: Write Tests +```rust +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn valid_input_succeeds() { + let input = Input { /* ... */ }; + let result = process(&input); + assert!(result.is_ok()); + } + + #[test] + fn invalid_input_returns_error() { + let input = Input { /* ... */ }; + let result = process(&input); + assert!(result.is_err()); + } +} +``` + +### Step 3: Run Tests (RED) +```bash +cargo test +``` + +### Step 4: Implement (GREEN) +```rust +pub fn process(input: &Input) -> Result { + // Minimal implementation that handles both paths + validate(input)?; + Ok(Output { /* ... */ }) +} +``` + +### Step 5: Check Coverage +```bash +cargo llvm-cov +cargo llvm-cov --fail-under-lines 80 +``` + +## Rust Testing Commands + +```bash +cargo test # Run all tests +cargo test -- --nocapture # Show println output +cargo test test_name # Run specific test +cargo test --no-fail-fast # Don't stop on first failure +cargo test --lib # Unit tests only +cargo test --test integration # Integration tests only +cargo test --doc # Doc tests only +cargo bench # Run benchmarks +``` + +## Test File Organization + +``` +src/ +├── lib.rs # Library root +├── service.rs # Implementation +└── service/ + └── tests.rs # Or inline #[cfg(test)] mod tests {} +tests/ +└── integration.rs # Integration tests +benches/ +└── benchmark.rs # Criterion benchmarks +``` + +--- + +**TIP**: Use `rstest` for parameterized tests and `proptest` for property-based testing. diff --git a/.opencode/prompts/agents/rust-build-resolver.txt b/.opencode/prompts/agents/rust-build-resolver.txt new file mode 100644 index 00000000..cca828d1 --- /dev/null +++ b/.opencode/prompts/agents/rust-build-resolver.txt @@ -0,0 +1,93 @@ +# Rust Build Error Resolver + +You are an expert Rust build error resolution specialist. Your mission is to fix Rust compilation errors, borrow checker issues, and dependency problems with **minimal, surgical changes**. + +## Core Responsibilities + +1. Diagnose `cargo build` / `cargo check` errors +2. Fix borrow checker and lifetime errors +3. Resolve trait implementation mismatches +4. Handle Cargo dependency and feature issues +5. Fix `cargo clippy` warnings + +## Diagnostic Commands + +Run these in order: + +```bash +cargo check 2>&1 +cargo clippy -- -D warnings 2>&1 +cargo fmt --check 2>&1 +cargo tree --duplicates +if command -v cargo-audit >/dev/null; then cargo audit; else echo "cargo-audit not installed"; fi +``` + +## Resolution Workflow + +```text +1. cargo check -> Parse error message and error code +2. Read affected file -> Understand ownership and lifetime context +3. Apply minimal fix -> Only what's needed +4. cargo check -> Verify fix +5. cargo clippy -> Check for warnings +6. cargo fmt --check -> Verify formatting +7. cargo test -> Ensure nothing broke +``` + +## Common Fix Patterns + +| Error | Cause | Fix | +|-------|-------|-----| +| `cannot borrow as mutable` | Immutable borrow active | Restructure to end immutable borrow first, or use `Cell`/`RefCell` | +| `does not live long enough` | Value dropped while still borrowed | Extend lifetime scope, use owned type, or add lifetime annotation | +| `cannot move out of` | Moving from behind a reference | Use `.clone()`, `.to_owned()`, or restructure to take ownership | +| `mismatched types` | Wrong type or missing conversion | Add `.into()`, `as`, or explicit type conversion | +| `trait X is not implemented for Y` | Missing impl or derive | Add `#[derive(Trait)]` or implement trait manually | +| `unresolved import` | Missing dependency or wrong path | Add to Cargo.toml or fix `use` path | +| `unused variable` / `unused import` | Dead code | Remove or prefix with `_` | + +## Borrow Checker Troubleshooting + +```rust +// Problem: Cannot borrow as mutable because also borrowed as immutable +// Fix: Restructure to end immutable borrow before mutable borrow +let value = map.get("key").cloned(); +if value.is_none() { + map.insert("key".into(), default_value); +} + +// Problem: Value does not live long enough +// Fix: Move ownership instead of borrowing +fn get_name() -> String { + let name = compute_name(); + name // Not &name (dangling reference) +} +``` + +## Key Principles + +- **Surgical fixes only** — don't refactor, just fix the error +- **Never** add `#[allow(unused)]` without explicit approval +- **Never** use `unsafe` to work around borrow checker errors +- **Never** add `.unwrap()` to silence type errors — propagate with `?` +- **Always** run `cargo check` after every fix attempt +- Fix root cause over suppressing symptoms + +## Stop Conditions + +Stop and report if: +- Same error persists after 3 fix attempts +- Fix introduces more errors than it resolves +- Error requires architectural changes beyond scope +- Borrow checker error requires redesigning data ownership model + +## Output Format + +```text +[FIXED] src/handler/user.rs:42 +Error: E0502 — cannot borrow `map` as mutable because it is also borrowed as immutable +Fix: Cloned value from immutable borrow before mutable insert +Remaining errors: 3 +``` + +Final: `Build Status: SUCCESS/FAILED | Errors Fixed: N | Files Modified: list` diff --git a/.opencode/prompts/agents/rust-reviewer.txt b/.opencode/prompts/agents/rust-reviewer.txt new file mode 100644 index 00000000..cab02f80 --- /dev/null +++ b/.opencode/prompts/agents/rust-reviewer.txt @@ -0,0 +1,61 @@ +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. Begin review + +## Security Checks (CRITICAL) + +- **SQL Injection**: String interpolation in queries + ```rust + // Bad + format!("SELECT * FROM users WHERE id = {}", user_id) + // Good: use parameterized queries via sqlx, diesel, etc. + sqlx::query("SELECT * FROM users WHERE id = $1").bind(user_id) + ``` + +- **Command Injection**: Unvalidated input in `std::process::Command` + ```rust + // Bad + Command::new("sh").arg("-c").arg(format!("echo {}", user_input)) + // Good + Command::new("echo").arg(user_input) + ``` + +- **Unsafe without justification**: Missing `// SAFETY:` comment +- **Hardcoded secrets**: API keys, passwords, tokens in source +- **Use-after-free via raw pointers**: Unsafe pointer manipulation + +## Error Handling (CRITICAL) + +- **Silenced errors**: `let _ = result;` on `#[must_use]` types +- **Missing error context**: `return Err(e)` without `.context()` or `.map_err()` +- **Panic in production**: `panic!()`, `todo!()`, `unreachable!()` in production paths +- **`Box` in libraries**: Use `thiserror` for typed errors + +## Ownership and Lifetimes (HIGH) + +- **Unnecessary cloning**: `.clone()` to satisfy borrow checker without understanding root cause +- **String instead of &str**: Taking `String` when `&str` suffices +- **Vec instead of slice**: Taking `Vec` when `&[T]` suffices + +## Concurrency (HIGH) + +- **Blocking in async**: `std::thread::sleep`, `std::fs` in async context +- **Unbounded channels**: `mpsc::channel()`/`tokio::sync::mpsc::unbounded_channel()` need justification — prefer bounded channels +- **`Mutex` poisoning ignored**: Not handling `PoisonError` +- **Missing `Send`/`Sync` bounds**: Types shared across threads + +## Code Quality (HIGH) + +- **Large functions**: Over 50 lines +- **Wildcard match on business enums**: `_ =>` hiding new variants +- **Dead code**: Unused functions, imports, variables + +## Approval Criteria + +- **Approve**: No CRITICAL or HIGH issues +- **Warning**: MEDIUM issues only +- **Block**: CRITICAL or HIGH issues found diff --git a/AGENTS.md b/AGENTS.md index 8e9e9fd1..56e7bb45 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1,6 +1,6 @@ # Everything Claude Code (ECC) — Agent Instructions -This is a **production-ready AI coding plugin** providing 16 specialized agents, 65+ skills, 40 commands, and automated hook workflows for software development. +This is a **production-ready AI coding plugin** providing 20 specialized agents, 65+ skills, 51 commands, and automated hook workflows for software development. ## Core Principles @@ -25,11 +25,15 @@ This is a **production-ready AI coding plugin** providing 16 specialized agents, | doc-updater | Documentation and codemaps | Updating docs | | go-reviewer | Go code review | Go projects | | go-build-resolver | Go build errors | Go build failures | +| kotlin-reviewer | Kotlin code review | Kotlin/Android/KMP projects | +| kotlin-build-resolver | Kotlin/Gradle build errors | Kotlin build failures | | database-reviewer | PostgreSQL/Supabase specialist | Schema design, query optimization | | python-reviewer | Python code review | Python projects | | chief-of-staff | Communication triage and drafts | Multi-channel email, Slack, LINE, Messenger | | loop-operator | Autonomous loop execution | Run loops safely, monitor stalls, intervene | | harness-optimizer | Harness config tuning | Reliability, cost, throughput | +| rust-reviewer | Rust code review | Rust projects | +| rust-build-resolver | Rust build errors | Rust build failures | ## Agent Orchestration @@ -128,9 +132,9 @@ Troubleshoot failures: check test isolation → verify mocks → fix implementat ## Project Structure ``` -agents/ — 13 specialized subagents +agents/ — 20 specialized subagents skills/ — 65+ workflow skills and domain knowledge -commands/ — 40 slash commands +commands/ — 51 slash commands hooks/ — Trigger-based automations rules/ — Always-follow guidelines (common + per-language) scripts/ — Cross-platform Node.js utilities diff --git a/agents/rust-build-resolver.md b/agents/rust-build-resolver.md new file mode 100644 index 00000000..4fdc961d --- /dev/null +++ b/agents/rust-build-resolver.md @@ -0,0 +1,148 @@ +--- +name: rust-build-resolver +description: Rust build, compilation, and dependency error resolution specialist. Fixes cargo build errors, borrow checker issues, and Cargo.toml problems with minimal changes. Use when Rust builds fail. +tools: ["Read", "Write", "Edit", "Bash", "Grep", "Glob"] +model: sonnet +--- + +# Rust Build Error Resolver + +You are an expert Rust build error resolution specialist. Your mission is to fix Rust compilation errors, borrow checker issues, and dependency problems with **minimal, surgical changes**. + +## Core Responsibilities + +1. Diagnose `cargo build` / `cargo check` errors +2. Fix borrow checker and lifetime errors +3. Resolve trait implementation mismatches +4. Handle Cargo dependency and feature issues +5. Fix `cargo clippy` warnings + +## Diagnostic Commands + +Run these in order: + +```bash +cargo check 2>&1 +cargo clippy -- -D warnings 2>&1 +cargo fmt --check 2>&1 +cargo tree --duplicates 2>&1 +if command -v cargo-audit >/dev/null; then cargo audit; else echo "cargo-audit not installed"; fi +``` + +## Resolution Workflow + +```text +1. cargo check -> Parse error message and error code +2. Read affected file -> Understand ownership and lifetime context +3. Apply minimal fix -> Only what's needed +4. cargo check -> Verify fix +5. cargo clippy -> Check for warnings +6. cargo test -> Ensure nothing broke +``` + +## Common Fix Patterns + +| Error | Cause | Fix | +|-------|-------|-----| +| `cannot borrow as mutable` | Immutable borrow active | Restructure to end immutable borrow first, or use `Cell`/`RefCell` | +| `does not live long enough` | Value dropped while still borrowed | Extend lifetime scope, use owned type, or add lifetime annotation | +| `cannot move out of` | Moving from behind a reference | Use `.clone()`, `.to_owned()`, or restructure to take ownership | +| `mismatched types` | Wrong type or missing conversion | Add `.into()`, `as`, or explicit type conversion | +| `trait X is not implemented for Y` | Missing impl or derive | Add `#[derive(Trait)]` or implement trait manually | +| `unresolved import` | Missing dependency or wrong path | Add to Cargo.toml or fix `use` path | +| `unused variable` / `unused import` | Dead code | Remove or prefix with `_` | +| `expected X, found Y` | Type mismatch in return/argument | Fix return type or add conversion | +| `cannot find macro` | Missing `#[macro_use]` or feature | Add dependency feature or import macro | +| `multiple applicable items` | Ambiguous trait method | Use fully qualified syntax: `::method()` | +| `lifetime may not live long enough` | Lifetime bound too short | Add lifetime bound or use `'static` where appropriate | +| `async fn is not Send` | Non-Send type held across `.await` | Restructure to drop non-Send values before `.await` | +| `the trait bound is not satisfied` | Missing generic constraint | Add trait bound to generic parameter | +| `no method named X` | Missing trait import | Add `use Trait;` import | + +## Borrow Checker Troubleshooting + +```rust +// Problem: Cannot borrow as mutable because also borrowed as immutable +// Fix: Restructure to end immutable borrow before mutable borrow +let value = map.get("key").cloned(); // Clone ends the immutable borrow +if value.is_none() { + map.insert("key".into(), default_value); +} + +// Problem: Value does not live long enough +// Fix: Move ownership instead of borrowing +fn get_name() -> String { // Return owned String + let name = compute_name(); + name // Not &name (dangling reference) +} + +// Problem: Cannot move out of index +// Fix: Use swap_remove, clone, or take +let item = vec.swap_remove(index); // Takes ownership +// Or: let item = vec[index].clone(); +``` + +## Cargo.toml Troubleshooting + +```bash +# Check dependency tree for conflicts +cargo tree -d # Show duplicate dependencies +cargo tree -i some_crate # Invert — who depends on this? + +# Feature resolution +cargo tree -f "{p} {f}" # Show features enabled per crate +cargo check --features "feat1,feat2" # Test specific feature combination + +# Workspace issues +cargo check --workspace # Check all workspace members +cargo check -p specific_crate # Check single crate in workspace + +# Lock file issues +cargo update -p specific_crate # Update one dependency (preferred) +cargo update # Full refresh (last resort — broad changes) +``` + +## Edition and MSRV Issues + +```bash +# Check edition in Cargo.toml (2024 is the current default for new projects) +grep "edition" Cargo.toml + +# Check minimum supported Rust version +rustc --version +grep "rust-version" Cargo.toml + +# Common fix: update edition for new syntax (check rust-version first!) +# In Cargo.toml: edition = "2024" # Requires rustc 1.85+ +``` + +## Key Principles + +- **Surgical fixes only** — don't refactor, just fix the error +- **Never** add `#[allow(unused)]` without explicit approval +- **Never** use `unsafe` to work around borrow checker errors +- **Never** add `.unwrap()` to silence type errors — propagate with `?` +- **Always** run `cargo check` after every fix attempt +- Fix root cause over suppressing symptoms +- Prefer the simplest fix that preserves the original intent + +## Stop Conditions + +Stop and report if: +- Same error persists after 3 fix attempts +- Fix introduces more errors than it resolves +- Error requires architectural changes beyond scope +- Borrow checker error requires redesigning data ownership model + +## Output Format + +```text +[FIXED] src/handler/user.rs:42 +Error: E0502 — cannot borrow `map` as mutable because it is also borrowed as immutable +Fix: Cloned value from immutable borrow before mutable insert +Remaining errors: 3 +``` + +Final: `Build Status: SUCCESS/FAILED | Errors Fixed: N | Files Modified: list` + +For detailed Rust error patterns and code examples, see `skill: rust-patterns`. diff --git a/agents/rust-reviewer.md b/agents/rust-reviewer.md new file mode 100644 index 00000000..85f1840a --- /dev/null +++ b/agents/rust-reviewer.md @@ -0,0 +1,93 @@ +--- +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. 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` 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` suffices +- **Vec instead of slice**: Taking `Vec` 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`. diff --git a/commands/rust-build.md b/commands/rust-build.md new file mode 100644 index 00000000..5bdb9e37 --- /dev/null +++ b/commands/rust-build.md @@ -0,0 +1,187 @@ +--- +description: Fix Rust build errors, borrow checker issues, and dependency problems incrementally. Invokes the rust-build-resolver agent for minimal, surgical fixes. +--- + +# Rust Build and Fix + +This command invokes the **rust-build-resolver** agent to incrementally fix Rust build errors with minimal changes. + +## What This Command Does + +1. **Run Diagnostics**: Execute `cargo check`, `cargo clippy`, `cargo fmt --check` +2. **Parse Errors**: Identify error codes and affected files +3. **Fix Incrementally**: One error at a time +4. **Verify Each Fix**: Re-run `cargo check` after each change +5. **Report Summary**: Show what was fixed and what remains + +## When to Use + +Use `/rust-build` when: +- `cargo build` or `cargo check` fails with errors +- `cargo clippy` reports warnings +- Borrow checker or lifetime errors block compilation +- Cargo dependency resolution fails +- After pulling changes that break the build + +## Diagnostic Commands Run + +```bash +# Primary build check +cargo check 2>&1 + +# Lints and suggestions +cargo clippy -- -D warnings 2>&1 + +# Formatting check +cargo fmt --check 2>&1 + +# Dependency issues +cargo tree --duplicates + +# Security audit (if available) +if command -v cargo-audit >/dev/null; then cargo audit; else echo "cargo-audit not installed"; fi +``` + +## Example Session + +````text +User: /rust-build + +Agent: +# Rust Build Resolution + +## Initial Diagnostics + +```bash +$ cargo check +error[E0502]: cannot borrow `map` as mutable because it is also borrowed as immutable + --> src/service/cache.rs:25:5 +error[E0308]: mismatched types + --> src/handler/api.rs:42:12 +error[E0425]: cannot find value `db` in this scope + --> src/handler/api.rs:58:9 +``` + +Errors found: 3 + +## Fix 1: Borrow Checker Error + +File: src/service/cache.rs:25 +Error: E0502 — cannot borrow `map` as mutable because also borrowed as immutable +Cause: Immutable borrow still active during mutable insert + +```rust +// Changed: restructured to end immutable borrow before mutable access +if !map.contains_key("key") { + map.insert("key".into(), default); +} +``` + +```bash +$ cargo check +# 2 errors remaining +``` + +## Fix 2: Type Mismatch + +File: src/handler/api.rs:42 +Error: E0308 — mismatched types: expected `usize`, found `&str` + +```rust +// Changed +let count = params.get("count"); +// To +let count: usize = params.get("count") + .and_then(|s| s.parse().ok()) + .unwrap_or(0); +``` + +```bash +$ cargo check +# 1 error remaining +``` + +## Fix 3: Unresolved Name + +File: src/handler/api.rs:58 +Error: E0425 — cannot find value `db` in this scope +Cause: Missing import + +```rust +// Added +use crate::db; +``` + +```bash +$ cargo check +# Build successful! +``` + +## Final Verification + +```bash +$ cargo clippy -- -D warnings +# No warnings + +$ cargo test +running 6 tests +test service::cache::tests::test_insert ... ok +test service::cache::tests::test_get ... ok +test handler::api::tests::test_parse_count ... ok +test handler::api::tests::test_missing_count ... ok +test handler::api::tests::test_db_import ... ok +test handler::api::tests::test_response ... ok + +test result: ok. 6 passed; 0 failed; 0 ignored +``` + +## Summary + +| Metric | Count | +|--------|-------| +| Build errors fixed | 3 | +| Clippy warnings fixed | 0 | +| Files modified | 2 | +| Remaining issues | 0 | + +Build Status: SUCCESS +```` + +## Common Errors Fixed + +| Error | Typical Fix | +|-------|-------------| +| `cannot borrow as mutable` | Restructure to end immutable borrow first; clone only if justified | +| `does not live long enough` | Use owned type or add lifetime annotation | +| `cannot move out of` | Restructure to take ownership; clone only as last resort | +| `mismatched types` | Add `.into()`, `as`, or explicit conversion | +| `trait X not implemented` | Add `#[derive(Trait)]` or implement manually | +| `unresolved import` | Add to Cargo.toml or fix `use` path | +| `cannot find value` | Add import or fix path | + +## Fix Strategy + +1. **Build errors first** - Code must compile +2. **Clippy warnings second** - Fix suspicious constructs +3. **Formatting third** - `cargo fmt` compliance +4. **One fix at a time** - Verify each change +5. **Minimal changes** - Don't refactor, just fix + +## Stop Conditions + +The agent will stop and report if: +- Same error persists after 3 attempts +- Fix introduces more errors +- Requires architectural changes +- Borrow checker error requires redesigning data ownership + +## Related Commands + +- `/rust-test` - Run tests after build succeeds +- `/rust-review` - Review code quality +- `/verify` - Full verification loop + +## Related + +- Agent: `agents/rust-build-resolver.md` +- Skill: `skills/rust-patterns/` diff --git a/commands/rust-review.md b/commands/rust-review.md new file mode 100644 index 00000000..5d91d7d9 --- /dev/null +++ b/commands/rust-review.md @@ -0,0 +1,142 @@ +--- +description: Comprehensive Rust code review for ownership, lifetimes, error handling, unsafe usage, and idiomatic patterns. Invokes the rust-reviewer agent. +--- + +# Rust Code Review + +This command invokes the **rust-reviewer** agent for comprehensive Rust-specific code review. + +## What This Command Does + +1. **Verify Automated Checks**: Run `cargo check`, `cargo clippy -- -D warnings`, `cargo fmt --check`, and `cargo test` — stop if any fail +2. **Identify Rust Changes**: Find modified `.rs` files via `git diff HEAD~1` (or `git diff main...HEAD` for PRs) +3. **Run Security Audit**: Execute `cargo audit` if available +4. **Security Scan**: Check for unsafe usage, command injection, hardcoded secrets +5. **Ownership Review**: Analyze unnecessary clones, lifetime issues, borrowing patterns +6. **Generate Report**: Categorize issues by severity + +## When to Use + +Use `/rust-review` when: +- After writing or modifying Rust code +- Before committing Rust changes +- Reviewing pull requests with Rust code +- Onboarding to a new Rust codebase +- Learning idiomatic Rust patterns + +## Review Categories + +### CRITICAL (Must Fix) +- Unchecked `unwrap()`/`expect()` in production code paths +- `unsafe` without `// SAFETY:` comment documenting invariants +- SQL injection via string interpolation in queries +- Command injection via unvalidated input in `std::process::Command` +- Hardcoded credentials +- Use-after-free via raw pointers + +### HIGH (Should Fix) +- Unnecessary `.clone()` to satisfy borrow checker +- `String` parameter where `&str` or `impl AsRef` suffices +- Blocking in async context (`std::thread::sleep`, `std::fs`) +- Missing `Send`/`Sync` bounds on shared types +- Wildcard `_ =>` match on business-critical enums +- Large functions (>50 lines) + +### MEDIUM (Consider) +- Unnecessary allocation in hot paths +- Missing `with_capacity` when size is known +- Suppressed clippy warnings without justification +- Public API without `///` documentation +- Consider `#[must_use]` on non-`must_use` return types where ignoring values is likely a bug + +## Automated Checks Run + +```bash +# Build gate (must pass before review) +cargo check + +# Lints and suggestions +cargo clippy -- -D warnings + +# Formatting +cargo fmt --check + +# Tests +cargo test + +# Security audit (if available) +if command -v cargo-audit >/dev/null; then cargo audit; else echo "cargo-audit not installed"; fi +``` + +## Example Usage + +````text +User: /rust-review + +Agent: +# Rust Code Review Report + +## Files Reviewed +- src/service/user.rs (modified) +- src/handler/api.rs (modified) + +## Static Analysis Results +- Build: Successful +- Clippy: No warnings +- Formatting: Passed +- Tests: All passing + +## Issues Found + +[CRITICAL] Unchecked unwrap in Production Path +File: src/service/user.rs:28 +Issue: Using `.unwrap()` on database query result +```rust +let user = db.find_by_id(id).unwrap(); // Panics on missing user +``` +Fix: Propagate error with context +```rust +let user = db.find_by_id(id) + .context("failed to fetch user")?; +``` + +[HIGH] Unnecessary Clone +File: src/handler/api.rs:45 +Issue: Cloning String to satisfy borrow checker +```rust +let name = user.name.clone(); +process(&user, &name); +``` +Fix: Restructure to avoid clone +```rust +let result = process_name(&user.name); +use_user(&user, result); +``` + +## Summary +- CRITICAL: 1 +- HIGH: 1 +- MEDIUM: 0 + +Recommendation: Block merge until CRITICAL issue is fixed +```` + +## Approval Criteria + +| Status | Condition | +|--------|-----------| +| Approve | No CRITICAL or HIGH issues | +| Warning | Only MEDIUM issues (merge with caution) | +| Block | CRITICAL or HIGH issues found | + +## Integration with Other Commands + +- Use `/rust-test` first to ensure tests pass +- Use `/rust-build` if build errors occur +- Use `/rust-review` before committing +- Use `/code-review` for non-Rust-specific concerns + +## Related + +- Agent: `agents/rust-reviewer.md` +- Skills: `skills/rust-patterns/`, `skills/rust-testing/` diff --git a/commands/rust-test.md b/commands/rust-test.md new file mode 100644 index 00000000..8a238479 --- /dev/null +++ b/commands/rust-test.md @@ -0,0 +1,308 @@ +--- +description: Enforce TDD workflow for Rust. Write tests first, then implement. Verify 80%+ coverage with cargo-llvm-cov. +--- + +# Rust TDD Command + +This command enforces test-driven development methodology for Rust code using `#[test]`, rstest, proptest, and mockall. + +## What This Command Does + +1. **Define Types/Traits**: Scaffold function signatures with `todo!()` +2. **Write Tests**: Create comprehensive test module (RED) +3. **Run Tests**: Verify tests fail for the right reason +4. **Implement Code**: Write minimal code to pass (GREEN) +5. **Refactor**: Improve while keeping tests green +6. **Check Coverage**: Ensure 80%+ coverage with cargo-llvm-cov + +## When to Use + +Use `/rust-test` when: +- Implementing new Rust functions, methods, or traits +- Adding test coverage to existing Rust code +- Fixing bugs (write failing test first) +- Building critical business logic +- Learning TDD workflow in Rust + +## TDD Cycle + +``` +RED -> Write failing test first +GREEN -> Implement minimal code to pass +REFACTOR -> Improve code, tests stay green +REPEAT -> Next test case +``` + +## Example Session + +````text +User: /rust-test I need a function to validate user registration + +Agent: +# TDD Session: Registration Validator + +## Step 1: Define Interface + +```rust +// src/validator.rs +pub struct RegistrationRequest { + pub name: String, + pub email: String, + pub password: String, +} + +pub enum ValidationResult { + Valid, + Invalid(Vec), +} + +pub fn validate_registration(request: &RegistrationRequest) -> ValidationResult { + todo!() +} +``` + +## Step 2: Write Tests (RED) + +```rust +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn valid_registration_returns_valid() { + let request = RegistrationRequest { + name: "Alice".into(), + email: "alice@example.com".into(), + password: "SecureP@ss1".into(), + }; + assert!(matches!(validate_registration(&request), ValidationResult::Valid)); + } + + #[test] + fn blank_name_returns_invalid() { + let request = RegistrationRequest { + name: "".into(), + email: "alice@example.com".into(), + password: "SecureP@ss1".into(), + }; + match validate_registration(&request) { + ValidationResult::Invalid(errors) => { + assert!(errors.contains(&"Name is required".to_string())); + } + ValidationResult::Valid => panic!("expected Invalid"), + } + } + + #[test] + fn invalid_email_returns_invalid() { + let request = RegistrationRequest { + name: "Alice".into(), + email: "not-an-email".into(), + password: "SecureP@ss1".into(), + }; + assert!(matches!( + validate_registration(&request), + ValidationResult::Invalid(_) + )); + } + + #[test] + fn short_password_returns_invalid() { + let request = RegistrationRequest { + name: "Alice".into(), + email: "alice@example.com".into(), + password: "short".into(), + }; + assert!(matches!( + validate_registration(&request), + ValidationResult::Invalid(_) + )); + } +} +``` + +## Step 3: Run Tests — Verify FAIL + +```bash +$ cargo test +running 4 tests +test tests::valid_registration_returns_valid ... FAILED (not yet implemented) +test tests::blank_name_returns_invalid ... FAILED +test tests::invalid_email_returns_invalid ... FAILED +test tests::short_password_returns_invalid ... FAILED +``` + +Tests fail as expected (todo! panic). + +## Step 4: Implement Minimal Code (GREEN) + +```rust +pub fn validate_registration(request: &RegistrationRequest) -> ValidationResult { + let mut errors = Vec::new(); + + if request.name.trim().is_empty() { + errors.push("Name is required".into()); + } + if !request.email.contains('@') { + errors.push("Invalid email format".into()); + } + if request.password.len() < 8 { + errors.push("Password must be at least 8 characters".into()); + } + + if errors.is_empty() { + ValidationResult::Valid + } else { + ValidationResult::Invalid(errors) + } +} +``` + +## Step 5: Run Tests — Verify PASS + +```bash +$ cargo test +running 4 tests +test tests::valid_registration_returns_valid ... ok +test tests::blank_name_returns_invalid ... ok +test tests::invalid_email_returns_invalid ... ok +test tests::short_password_returns_invalid ... ok + +test result: ok. 4 passed; 0 failed +``` + +All tests passing! + +## Step 6: Check Coverage + +```bash +$ cargo llvm-cov +Coverage: 100.0% of lines +``` + +Coverage: 100% + +## TDD Complete! +```` + +## Test Patterns + +### Unit Tests + +```rust +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn adds_two_numbers() { + assert_eq!(add(2, 3), 5); + } + + #[test] + fn handles_error() -> Result<(), Box> { + let result = parse_config(r#"port = 8080"#)?; + assert_eq!(result.port, 8080); + Ok(()) + } +} +``` + +### Parameterized Tests with rstest + +```rust +use rstest::{rstest, fixture}; + +#[rstest] +#[case("hello", 5)] +#[case("", 0)] +#[case("rust", 4)] +fn test_string_length(#[case] input: &str, #[case] expected: usize) { + assert_eq!(input.len(), expected); +} +``` + +### Async Tests + +```rust +#[tokio::test] +async fn fetches_data_successfully() { + let client = TestClient::new().await; + let result = client.get("/data").await; + assert!(result.is_ok()); +} +``` + +### Property-Based Tests + +```rust +use proptest::prelude::*; + +proptest! { + #[test] + fn encode_decode_roundtrip(input in ".*") { + let encoded = encode(&input); + let decoded = decode(&encoded).unwrap(); + assert_eq!(input, decoded); + } +} +``` + +## Coverage Commands + +```bash +# Summary report +cargo llvm-cov + +# HTML report +cargo llvm-cov --html + +# Fail if below threshold +cargo llvm-cov --fail-under-lines 80 + +# Run specific test +cargo test test_name + +# Run with output +cargo test -- --nocapture + +# Run without stopping on first failure +cargo test --no-fail-fast +``` + +## Coverage Targets + +| Code Type | Target | +|-----------|--------| +| Critical business logic | 100% | +| Public API | 90%+ | +| General code | 80%+ | +| Generated / FFI bindings | Exclude | + +## TDD Best Practices + +**DO:** +- Write test FIRST, before any implementation +- Run tests after each change +- Use `assert_eq!` over `assert!` for better error messages +- Use `?` in tests that return `Result` for cleaner output +- Test behavior, not implementation +- Include edge cases (empty, boundary, error paths) + +**DON'T:** +- Write implementation before tests +- Skip the RED phase +- Use `#[should_panic]` when `Result::is_err()` works +- Use `sleep()` in tests — use channels or `tokio::time::pause()` +- Mock everything — prefer integration tests when feasible + +## Related Commands + +- `/rust-build` - Fix build errors +- `/rust-review` - Review code after implementation +- `/verify` - Run full verification loop + +## Related + +- Skill: `skills/rust-testing/` +- Skill: `skills/rust-patterns/` diff --git a/skills/rust-patterns/SKILL.md b/skills/rust-patterns/SKILL.md new file mode 100644 index 00000000..a9427da1 --- /dev/null +++ b/skills/rust-patterns/SKILL.md @@ -0,0 +1,500 @@ +--- +name: rust-patterns +description: Idiomatic Rust patterns, ownership, error handling, traits, concurrency, and best practices for building safe, performant applications. +origin: ECC +--- + +# Rust Development Patterns + +Idiomatic Rust patterns and best practices for building safe, performant, and maintainable applications. + +## When to Use + +- Writing new Rust code +- Reviewing Rust code +- Refactoring existing Rust code +- Designing crate structure and module layout + +## How It Works + +This skill enforces idiomatic Rust conventions across six key areas: ownership and borrowing to prevent data races at compile time, `Result`/`?` error propagation with `thiserror` for libraries and `anyhow` for applications, enums and exhaustive pattern matching to make illegal states unrepresentable, traits and generics for zero-cost abstraction, safe concurrency via `Arc>`, channels, and async/await, and minimal `pub` surfaces organized by domain. + +## Core Principles + +### 1. Ownership and Borrowing + +Rust's ownership system prevents data races and memory bugs at compile time. + +```rust +// Good: Pass references when you don't need ownership +fn process(data: &[u8]) -> usize { + data.len() +} + +// Good: Take ownership only when you need to store or consume +fn store(data: Vec) -> Record { + Record { payload: data } +} + +// Bad: Cloning unnecessarily to avoid borrow checker +fn process_bad(data: &Vec) -> usize { + let cloned = data.clone(); // Wasteful — just borrow + cloned.len() +} +``` + + +### Use `Cow` for Flexible Ownership + +```rust +use std::borrow::Cow; + +fn normalize(input: &str) -> Cow<'_, str> { + if input.contains(' ') { + Cow::Owned(input.replace(' ', "_")) + } else { + Cow::Borrowed(input) // Zero-cost when no mutation needed + } +} +``` + +## Error Handling + +### Use `Result` and `?` — Never `unwrap()` in Production + +```rust +// Good: Propagate errors with context +use anyhow::{Context, Result}; + +fn load_config(path: &str) -> Result { + let content = std::fs::read_to_string(path) + .with_context(|| format!("failed to read config from {path}"))?; + let config: Config = toml::from_str(&content) + .with_context(|| format!("failed to parse config from {path}"))?; + Ok(config) +} + +// Bad: Panics on error +fn load_config_bad(path: &str) -> Config { + let content = std::fs::read_to_string(path).unwrap(); // Panics! + toml::from_str(&content).unwrap() +} +``` + +### Library Errors with `thiserror`, Application Errors with `anyhow` + +```rust +// Library code: structured, typed errors +use thiserror::Error; + +#[derive(Debug, Error)] +pub enum StorageError { + #[error("record not found: {id}")] + NotFound { id: String }, + #[error("connection failed")] + Connection(#[from] std::io::Error), + #[error("invalid data: {0}")] + InvalidData(String), +} + +// Application code: flexible error handling +use anyhow::{bail, Result}; + +fn run() -> Result<()> { + let config = load_config("app.toml")?; + if config.workers == 0 { + bail!("worker count must be > 0"); + } + Ok(()) +} +``` + +### `Option` Combinators Over Nested Matching + +```rust +// Good: Combinator chain +fn find_user_email(users: &[User], id: u64) -> Option { + users.iter() + .find(|u| u.id == id) + .map(|u| u.email.clone()) +} + +// Bad: Deeply nested matching +fn find_user_email_bad(users: &[User], id: u64) -> Option { + match users.iter().find(|u| u.id == id) { + Some(user) => match &user.email { + email => Some(email.clone()), + }, + None => None, + } +} +``` + +## Enums and Pattern Matching + +### Model States as Enums + +```rust +// Good: Impossible states are unrepresentable +enum ConnectionState { + Disconnected, + Connecting { attempt: u32 }, + Connected { session_id: String }, + Failed { reason: String, retries: u32 }, +} + +fn handle(state: &ConnectionState) { + match state { + ConnectionState::Disconnected => connect(), + ConnectionState::Connecting { attempt } if *attempt > 3 => abort(), + ConnectionState::Connecting { .. } => wait(), + ConnectionState::Connected { session_id } => use_session(session_id), + ConnectionState::Failed { retries, .. } if *retries < 5 => retry(), + ConnectionState::Failed { reason, .. } => log_failure(reason), + } +} +``` + +### Exhaustive Matching — No Catch-All for Business Logic + +```rust +// Good: Handle every variant explicitly +match command { + Command::Start => start_service(), + Command::Stop => stop_service(), + Command::Restart => restart_service(), + // Adding a new variant forces handling here +} + +// Bad: Wildcard hides new variants +match command { + Command::Start => start_service(), + _ => {} // Silently ignores Stop, Restart, and future variants +} +``` + +## Traits and Generics + +### Accept Generics, Return Concrete Types + +```rust +// Good: Generic input, concrete output +fn read_all(reader: &mut impl Read) -> std::io::Result> { + let mut buf = Vec::new(); + reader.read_to_end(&mut buf)?; + Ok(buf) +} + +// Good: Trait bounds for multiple constraints +fn process(item: T) -> String { + format!("processed: {item}") +} +``` + +### Trait Objects for Dynamic Dispatch + +```rust +// Use when you need heterogeneous collections or plugin systems +trait Handler: Send + Sync { + fn handle(&self, request: &Request) -> Response; +} + +struct Router { + handlers: Vec>, +} + +// Use generics when you need performance (monomorphization) +fn fast_process(handler: &H, request: &Request) -> Response { + handler.handle(request) +} +``` + +### Newtype Pattern for Type Safety + +```rust +// Good: Distinct types prevent mixing up arguments +struct UserId(u64); +struct OrderId(u64); + +fn get_order(user: UserId, order: OrderId) -> Result { + // Can't accidentally swap user and order IDs + todo!() +} + +// Bad: Easy to swap arguments +fn get_order_bad(user_id: u64, order_id: u64) -> Result { + todo!() +} +``` + +## Structs and Data Modeling + +### Builder Pattern for Complex Construction + +```rust +struct ServerConfig { + host: String, + port: u16, + max_connections: usize, +} + +impl ServerConfig { + fn builder(host: impl Into, port: u16) -> ServerConfigBuilder { + ServerConfigBuilder { host: host.into(), port, max_connections: 100 } + } +} + +struct ServerConfigBuilder { host: String, port: u16, max_connections: usize } + +impl ServerConfigBuilder { + fn max_connections(mut self, n: usize) -> Self { self.max_connections = n; self } + fn build(self) -> ServerConfig { + ServerConfig { host: self.host, port: self.port, max_connections: self.max_connections } + } +} + +// Usage: ServerConfig::builder("localhost", 8080).max_connections(200).build() +``` + +## Iterators and Closures + +### Prefer Iterator Chains Over Manual Loops + +```rust +// Good: Declarative, lazy, composable +let active_emails: Vec = users.iter() + .filter(|u| u.is_active) + .map(|u| u.email.clone()) + .collect(); + +// Bad: Imperative accumulation +let mut active_emails = Vec::new(); +for user in &users { + if user.is_active { + active_emails.push(user.email.clone()); + } +} +``` + +### Use `collect()` with Type Annotation + +```rust +// Collect into different types +let names: Vec<_> = items.iter().map(|i| &i.name).collect(); +let lookup: HashMap<_, _> = items.iter().map(|i| (i.id, i)).collect(); +let combined: String = parts.iter().copied().collect(); + +// Collect Results — short-circuits on first error +let parsed: Result, _> = strings.iter().map(|s| s.parse()).collect(); +``` + +## Concurrency + +### `Arc>` for Shared Mutable State + +```rust +use std::sync::{Arc, Mutex}; + +let counter = Arc::new(Mutex::new(0)); +let handles: Vec<_> = (0..10).map(|_| { + let counter = Arc::clone(&counter); + std::thread::spawn(move || { + let mut num = counter.lock().expect("mutex poisoned"); + *num += 1; + }) +}).collect(); + +for handle in handles { + handle.join().expect("worker thread panicked"); +} +``` + +### Channels for Message Passing + +```rust +use std::sync::mpsc; + +let (tx, rx) = mpsc::sync_channel(16); // Bounded channel with backpressure + +for i in 0..5 { + let tx = tx.clone(); + std::thread::spawn(move || { + tx.send(format!("message {i}")).expect("receiver disconnected"); + }); +} +drop(tx); // Close sender so rx iterator terminates + +for msg in rx { + println!("{msg}"); +} +``` + +### Async with Tokio + +```rust +use tokio::time::Duration; + +async fn fetch_with_timeout(url: &str) -> Result { + let response = tokio::time::timeout( + Duration::from_secs(5), + reqwest::get(url), + ) + .await + .context("request timed out")? + .context("request failed")?; + + response.text().await.context("failed to read body") +} + +// Spawn concurrent tasks +async fn fetch_all(urls: Vec) -> Vec> { + let handles: Vec<_> = urls.into_iter() + .map(|url| tokio::spawn(async move { + fetch_with_timeout(&url).await + })) + .collect(); + + let mut results = Vec::with_capacity(handles.len()); + for handle in handles { + results.push(handle.await.unwrap_or_else(|e| panic!("spawned task panicked: {e}"))); + } + results +} +``` + +## Unsafe Code + +### When Unsafe Is Acceptable + +```rust +// Acceptable: FFI boundary with documented invariants (Rust 2024+) +/// # Safety +/// `ptr` must be a valid, aligned pointer to an initialized `Widget`. +unsafe fn widget_from_raw<'a>(ptr: *const Widget) -> &'a Widget { + // SAFETY: caller guarantees ptr is valid and aligned + unsafe { &*ptr } +} + +// Acceptable: Performance-critical path with proof of correctness +// SAFETY: index is always < len due to the loop bound +unsafe { slice.get_unchecked(index) } +``` + +### When Unsafe Is NOT Acceptable + +```rust +// Bad: Using unsafe to bypass borrow checker +// Bad: Using unsafe for convenience +// Bad: Using unsafe without a Safety comment +// Bad: Transmuting between unrelated types +``` + +## Module System and Crate Structure + +### Organize by Domain, Not by Type + +```text +my_app/ +├── src/ +│ ├── main.rs +│ ├── lib.rs +│ ├── auth/ # Domain module +│ │ ├── mod.rs +│ │ ├── token.rs +│ │ └── middleware.rs +│ ├── orders/ # Domain module +│ │ ├── mod.rs +│ │ ├── model.rs +│ │ └── service.rs +│ └── db/ # Infrastructure +│ ├── mod.rs +│ └── pool.rs +├── tests/ # Integration tests +├── benches/ # Benchmarks +└── Cargo.toml +``` + +### Visibility — Expose Minimally + +```rust +// Good: pub(crate) for internal sharing +pub(crate) fn validate_input(input: &str) -> bool { + !input.is_empty() +} + +// Good: Re-export public API from lib.rs +pub mod auth; +pub use auth::AuthMiddleware; + +// Bad: Making everything pub +pub fn internal_helper() {} // Should be pub(crate) or private +``` + +## Tooling Integration + +### Essential Commands + +```bash +# Build and check +cargo build +cargo check # Fast type checking without codegen +cargo clippy # Lints and suggestions +cargo fmt # Format code + +# Testing +cargo test +cargo test -- --nocapture # Show println output +cargo test --lib # Unit tests only +cargo test --test integration # Integration tests only + +# Dependencies +cargo audit # Security audit +cargo tree # Dependency tree +cargo update # Update dependencies + +# Performance +cargo bench # Run benchmarks +``` + +## Quick Reference: Rust Idioms + +| Idiom | Description | +|-------|-------------| +| Borrow, don't clone | Pass `&T` instead of cloning unless ownership is needed | +| Make illegal states unrepresentable | Use enums to model valid states only | +| `?` over `unwrap()` | Propagate errors, never panic in library/production code | +| Parse, don't validate | Convert unstructured data to typed structs at the boundary | +| Newtype for type safety | Wrap primitives in newtypes to prevent argument swaps | +| Prefer iterators over loops | Declarative chains are clearer and often faster | +| `#[must_use]` on Results | Ensure callers handle return values | +| `Cow` for flexible ownership | Avoid allocations when borrowing suffices | +| Exhaustive matching | No wildcard `_` for business-critical enums | +| Minimal `pub` surface | Use `pub(crate)` for internal APIs | + +## Anti-Patterns to Avoid + +```rust +// Bad: .unwrap() in production code +let value = map.get("key").unwrap(); + +// Bad: .clone() to satisfy borrow checker without understanding why +let data = expensive_data.clone(); +process(&original, &data); + +// Bad: Using String when &str suffices +fn greet(name: String) { /* should be &str */ } + +// Bad: Box in libraries (use thiserror instead) +fn parse(input: &str) -> Result> { todo!() } + +// Bad: Ignoring must_use warnings +let _ = validate(input); // Silently discarding a Result + +// Bad: Blocking in async context +async fn bad_async() { + std::thread::sleep(Duration::from_secs(1)); // Blocks the executor! + // Use: tokio::time::sleep(Duration::from_secs(1)).await; +} +``` + +**Remember**: If it compiles, it's probably correct — but only if you avoid `unwrap()`, minimize `unsafe`, and let the type system work for you. diff --git a/skills/rust-testing/SKILL.md b/skills/rust-testing/SKILL.md new file mode 100644 index 00000000..21b484a1 --- /dev/null +++ b/skills/rust-testing/SKILL.md @@ -0,0 +1,500 @@ +--- +name: rust-testing +description: Rust testing patterns including unit tests, integration tests, async testing, property-based testing, mocking, and coverage. Follows TDD methodology. +origin: ECC +--- + +# Rust Testing Patterns + +Comprehensive Rust testing patterns for writing reliable, maintainable tests following TDD methodology. + +## When to Use + +- Writing new Rust functions, methods, or traits +- Adding test coverage to existing code +- Creating benchmarks for performance-critical code +- Implementing property-based tests for input validation +- Following TDD workflow in Rust projects + +## How It Works + +1. **Identify target code** — Find the function, trait, or module to test +2. **Write a test** — Use `#[test]` in a `#[cfg(test)]` module, rstest for parameterized tests, or proptest for property-based tests +3. **Mock dependencies** — Use mockall to isolate the unit under test +4. **Run tests (RED)** — Verify the test fails with the expected error +5. **Implement (GREEN)** — Write minimal code to pass +6. **Refactor** — Improve while keeping tests green +7. **Check coverage** — Use cargo-llvm-cov, target 80%+ + +## TDD Workflow for Rust + +### The RED-GREEN-REFACTOR Cycle + +``` +RED → Write a failing test first +GREEN → Write minimal code to pass the test +REFACTOR → Improve code while keeping tests green +REPEAT → Continue with next requirement +``` + +### Step-by-Step TDD in Rust + +```rust +// RED: Write test first, use todo!() as placeholder +pub fn add(a: i32, b: i32) -> i32 { todo!() } + +#[cfg(test)] +mod tests { + use super::*; + #[test] + fn test_add() { assert_eq!(add(2, 3), 5); } +} +// cargo test → panics at 'not yet implemented' +``` + +```rust +// GREEN: Replace todo!() with minimal implementation +pub fn add(a: i32, b: i32) -> i32 { a + b } +// cargo test → PASS, then REFACTOR while keeping tests green +``` + +## Unit Tests + +### Module-Level Test Organization + +```rust +// src/user.rs +pub struct User { + pub name: String, + pub email: String, +} + +impl User { + pub fn new(name: impl Into, email: impl Into) -> Result { + let email = email.into(); + if !email.contains('@') { + return Err(format!("invalid email: {email}")); + } + Ok(Self { name: name.into(), email }) + } + + pub fn display_name(&self) -> &str { + &self.name + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn creates_user_with_valid_email() { + let user = User::new("Alice", "alice@example.com").unwrap(); + assert_eq!(user.display_name(), "Alice"); + assert_eq!(user.email, "alice@example.com"); + } + + #[test] + fn rejects_invalid_email() { + let result = User::new("Bob", "not-an-email"); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("invalid email")); + } +} +``` + +### Assertion Macros + +```rust +assert_eq!(2 + 2, 4); // Equality +assert_ne!(2 + 2, 5); // Inequality +assert!(vec![1, 2, 3].contains(&2)); // Boolean +assert_eq!(value, 42, "expected 42 but got {value}"); // Custom message +assert!((0.1_f64 + 0.2 - 0.3).abs() < f64::EPSILON); // Float comparison +``` + +## Error and Panic Testing + +### Testing `Result` Returns + +```rust +#[test] +fn parse_returns_error_for_invalid_input() { + let result = parse_config("}{invalid"); + assert!(result.is_err()); + + // Assert specific error variant + let err = result.unwrap_err(); + assert!(matches!(err, ConfigError::ParseError(_))); +} + +#[test] +fn parse_succeeds_for_valid_input() -> Result<(), Box> { + let config = parse_config(r#"{"port": 8080}"#)?; + assert_eq!(config.port, 8080); + Ok(()) // Test fails if any ? returns Err +} +``` + +### Testing Panics + +```rust +#[test] +#[should_panic] +fn panics_on_empty_input() { + process(&[]); +} + +#[test] +#[should_panic(expected = "index out of bounds")] +fn panics_with_specific_message() { + let v: Vec = vec![]; + let _ = v[0]; +} +``` + +## Integration Tests + +### File Structure + +```text +my_crate/ +├── src/ +│ └── lib.rs +├── tests/ # Integration tests +│ ├── api_test.rs # Each file is a separate test binary +│ ├── db_test.rs +│ └── common/ # Shared test utilities +│ └── mod.rs +``` + +### Writing Integration Tests + +```rust +// tests/api_test.rs +use my_crate::{App, Config}; + +#[test] +fn full_request_lifecycle() { + let config = Config::test_default(); + let app = App::new(config); + + let response = app.handle_request("/health"); + assert_eq!(response.status, 200); + assert_eq!(response.body, "OK"); +} +``` + +## Async Tests + +### With Tokio + +```rust +#[tokio::test] +async fn fetches_data_successfully() { + let client = TestClient::new().await; + let result = client.get("/data").await; + assert!(result.is_ok()); + assert_eq!(result.unwrap().items.len(), 3); +} + +#[tokio::test] +async fn handles_timeout() { + use std::time::Duration; + let result = tokio::time::timeout( + Duration::from_millis(100), + slow_operation(), + ).await; + + assert!(result.is_err(), "should have timed out"); +} +``` + +## Test Organization Patterns + +### Parameterized Tests with `rstest` + +```rust +use rstest::{rstest, fixture}; + +#[rstest] +#[case("hello", 5)] +#[case("", 0)] +#[case("rust", 4)] +fn test_string_length(#[case] input: &str, #[case] expected: usize) { + assert_eq!(input.len(), expected); +} + +// Fixtures +#[fixture] +fn test_db() -> TestDb { + TestDb::new_in_memory() +} + +#[rstest] +fn test_insert(test_db: TestDb) { + test_db.insert("key", "value"); + assert_eq!(test_db.get("key"), Some("value".into())); +} +``` + +### Test Helpers + +```rust +#[cfg(test)] +mod tests { + use super::*; + + /// Creates a test user with sensible defaults. + fn make_user(name: &str) -> User { + User::new(name, &format!("{name}@test.com")).unwrap() + } + + #[test] + fn user_display() { + let user = make_user("alice"); + assert_eq!(user.display_name(), "alice"); + } +} +``` + +## Property-Based Testing with `proptest` + +### Basic Property Tests + +```rust +use proptest::prelude::*; + +proptest! { + #[test] + fn encode_decode_roundtrip(input in ".*") { + let encoded = encode(&input); + let decoded = decode(&encoded).unwrap(); + assert_eq!(input, decoded); + } + + #[test] + fn sort_preserves_length(mut vec in prop::collection::vec(any::(), 0..100)) { + let original_len = vec.len(); + vec.sort(); + assert_eq!(vec.len(), original_len); + } + + #[test] + fn sort_produces_ordered_output(mut vec in prop::collection::vec(any::(), 0..100)) { + vec.sort(); + for window in vec.windows(2) { + assert!(window[0] <= window[1]); + } + } +} +``` + +### Custom Strategies + +```rust +use proptest::prelude::*; + +fn valid_email() -> impl Strategy { + ("[a-z]{1,10}", "[a-z]{1,5}") + .prop_map(|(user, domain)| format!("{user}@{domain}.com")) +} + +proptest! { + #[test] + fn accepts_valid_emails(email in valid_email()) { + assert!(User::new("Test", &email).is_ok()); + } +} +``` + +## Mocking with `mockall` + +### Trait-Based Mocking + +```rust +use mockall::{automock, predicate::eq}; + +#[automock] +trait UserRepository { + fn find_by_id(&self, id: u64) -> Option; + fn save(&self, user: &User) -> Result<(), StorageError>; +} + +#[test] +fn service_returns_user_when_found() { + let mut mock = MockUserRepository::new(); + mock.expect_find_by_id() + .with(eq(42)) + .times(1) + .returning(|_| Some(User { id: 42, name: "Alice".into() })); + + let service = UserService::new(Box::new(mock)); + let user = service.get_user(42).unwrap(); + assert_eq!(user.name, "Alice"); +} + +#[test] +fn service_returns_none_when_not_found() { + let mut mock = MockUserRepository::new(); + mock.expect_find_by_id() + .returning(|_| None); + + let service = UserService::new(Box::new(mock)); + assert!(service.get_user(99).is_none()); +} +``` + +## Doc Tests + +### Executable Documentation + +```rust +/// Adds two numbers together. +/// +/// # Examples +/// +/// ``` +/// use my_crate::add; +/// +/// assert_eq!(add(2, 3), 5); +/// assert_eq!(add(-1, 1), 0); +/// ``` +pub fn add(a: i32, b: i32) -> i32 { + a + b +} + +/// Parses a config string. +/// +/// # Errors +/// +/// Returns `Err` if the input is not valid TOML. +/// +/// ```no_run +/// use my_crate::parse_config; +/// +/// let config = parse_config(r#"port = 8080"#).unwrap(); +/// assert_eq!(config.port, 8080); +/// ``` +/// +/// ```no_run +/// use my_crate::parse_config; +/// +/// assert!(parse_config("}{invalid").is_err()); +/// ``` +pub fn parse_config(input: &str) -> Result { + todo!() +} +``` + +## Benchmarking with Criterion + +```toml +# Cargo.toml +[dev-dependencies] +criterion = { version = "0.5", features = ["html_reports"] } + +[[bench]] +name = "benchmark" +harness = false +``` + +```rust +// benches/benchmark.rs +use criterion::{black_box, criterion_group, criterion_main, Criterion}; + +fn fibonacci(n: u64) -> u64 { + match n { + 0 | 1 => n, + _ => fibonacci(n - 1) + fibonacci(n - 2), + } +} + +fn bench_fibonacci(c: &mut Criterion) { + c.bench_function("fib 20", |b| b.iter(|| fibonacci(black_box(20)))); +} + +criterion_group!(benches, bench_fibonacci); +criterion_main!(benches); +``` + +## Test Coverage + +### Running Coverage + +```bash +# Install: cargo install cargo-llvm-cov (or use taiki-e/install-action in CI) +cargo llvm-cov # Summary +cargo llvm-cov --html # HTML report +cargo llvm-cov --lcov > lcov.info # LCOV format for CI +cargo llvm-cov --fail-under-lines 80 # Fail if below threshold +``` + +### Coverage Targets + +| Code Type | Target | +|-----------|--------| +| Critical business logic | 100% | +| Public API | 90%+ | +| General code | 80%+ | +| Generated / FFI bindings | Exclude | + +## Testing Commands + +```bash +cargo test # Run all tests +cargo test -- --nocapture # Show println output +cargo test test_name # Run tests matching pattern +cargo test --lib # Unit tests only +cargo test --test api_test # Integration tests only +cargo test --doc # Doc tests only +cargo test --no-fail-fast # Don't stop on first failure +cargo test -- --ignored # Run ignored tests +``` + +## Best Practices + +**DO:** +- Write tests FIRST (TDD) +- Use `#[cfg(test)]` modules for unit tests +- Test behavior, not implementation +- Use descriptive test names that explain the scenario +- Prefer `assert_eq!` over `assert!` for better error messages +- Use `?` in tests that return `Result` for cleaner error output +- Keep tests independent — no shared mutable state + +**DON'T:** +- Use `#[should_panic]` when you can test `Result::is_err()` instead +- Mock everything — prefer integration tests when feasible +- Ignore flaky tests — fix or quarantine them +- Use `sleep()` in tests — use channels, barriers, or `tokio::time::pause()` +- Skip error path testing + +## CI Integration + +```yaml +# GitHub Actions +test: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: dtolnay/rust-toolchain@stable + with: + components: clippy, rustfmt + + - name: Check formatting + run: cargo fmt --check + + - name: Clippy + run: cargo clippy -- -D warnings + + - name: Run tests + run: cargo test + + - uses: taiki-e/install-action@cargo-llvm-cov + + - name: Coverage + run: cargo llvm-cov --fail-under-lines 80 +``` + +**Remember**: Tests are documentation. They show how your code is meant to be used. Write them clearly and keep them up to date.