From d1730c5f6a33f6fc3e8cb0faf409f119eea5cf7c Mon Sep 17 00:00:00 2001 From: lcian <17258265+lcian@users.noreply.github.com> Date: Wed, 18 Feb 2026 15:28:18 +0100 Subject: [PATCH] feat(skills): Add rust-development skill for Rust PR review Add a skill that reviews Rust code changes against Sentry's Rust conventions. Covers error handling, iterator patterns, async code, field visibility, import organization, naming, documentation, and versioning. Designed to be invoked when validating or finishing a PR. Based on https://develop.sentry.dev/engineering-practices/rust.md Co-Authored-By: Claude Opus 4.6 --- .claude/settings.json | 3 +- README.md | 1 + .../skills/claude-settings-audit/SKILL.md | 3 +- .../skills/rust-development/SKILL.md | 183 ++++++++++++++++++ 4 files changed, 188 insertions(+), 2 deletions(-) create mode 100644 plugins/sentry-skills/skills/rust-development/SKILL.md diff --git a/.claude/settings.json b/.claude/settings.json index d119f9a..b2de21b 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -55,7 +55,8 @@ "Skill(sentry-skills:django-perf-review)", "Skill(sentry-skills:code-simplifier)", "Skill(sentry-skills:skill-creator)", - "Skill(sentry-skills:skill-scanner)" + "Skill(sentry-skills:skill-scanner)", + "Skill(sentry-skills:rust-development)" ], "deny": [] }, diff --git a/README.md b/README.md index c31429a..c982210 100644 --- a/README.md +++ b/README.md @@ -49,6 +49,7 @@ Works with Claude Code, Cursor, Cline, GitHub Copilot, and other compatible agen | [code-simplifier](plugins/sentry-skills/skills/code-simplifier/SKILL.md) | Simplifies and refines code for clarity, consistency, and maintainability | | [skill-creator](plugins/sentry-skills/skills/skill-creator/SKILL.md) | Create new agent skills for this repository | | [skill-scanner](plugins/sentry-skills/skills/skill-scanner/SKILL.md) | Scan agent skills for security issues before adoption | +| [rust-development](plugins/sentry-skills/skills/rust-development/SKILL.md) | Review Rust code for Sentry conventions and best practices | ## Available Subagents diff --git a/plugins/sentry-skills/skills/claude-settings-audit/SKILL.md b/plugins/sentry-skills/skills/claude-settings-audit/SKILL.md index 85ee75f..ac42102 100644 --- a/plugins/sentry-skills/skills/claude-settings-audit/SKILL.md +++ b/plugins/sentry-skills/skills/claude-settings-audit/SKILL.md @@ -155,7 +155,8 @@ If this is a Sentry project (or sentry-skills plugin is installed), include: "Skill(sentry-skills:django-perf-review)", "Skill(sentry-skills:code-simplifier)", "Skill(sentry-skills:skill-creator)", - "Skill(sentry-skills:skill-scanner)" + "Skill(sentry-skills:skill-scanner)", + "Skill(sentry-skills:rust-development)" ] ``` diff --git a/plugins/sentry-skills/skills/rust-development/SKILL.md b/plugins/sentry-skills/skills/rust-development/SKILL.md new file mode 100644 index 0000000..a0f2624 --- /dev/null +++ b/plugins/sentry-skills/skills/rust-development/SKILL.md @@ -0,0 +1,183 @@ +--- +name: rust-development +description: Review Rust code for Sentry conventions and best practices. Use when finishing a Rust PR, validating Rust changes, "review Rust code", "check Rust conventions", or ensuring Rust code follows Sentry style guidelines. Covers error handling, naming, iterators, async patterns, imports, documentation, and versioning. +allowed-tools: Read, Grep, Glob, Bash, Task +--- + + + +# Rust Development Review + +Review Rust code changes against Sentry's Rust conventions. Focus on the diff — only flag issues in changed code. + +## Step 1: Gather Context + +1. Run `git diff main...HEAD -- '*.rs' 'Cargo.toml'` to identify changed Rust files +2. If no Rust files changed, stop and report "No Rust files in this PR" +3. Read the changed files to understand the full context around each diff hunk + +## Step 2: Review Error Handling + +These patterns cause panics in production. Flag every occurrence in changed code. + +| Pattern | Issue | Fix | +|---------|-------|-----| +| `.unwrap()` | Panics on None/Err | Use `?`, `.unwrap_or()`, `.unwrap_or_default()`, or match | +| `array[index]` on slices | Panics on out-of-bounds | Use `.get(index)` | +| `a - b` on unsigned integers | Panics on overflow in debug, wraps in release | Use `checked_sub()` or `saturating_sub()` | +| Unchecked arithmetic on user input | Overflow/underflow | Use `checked_add()`, `checked_mul()`, etc. | + +**Exception:** `.unwrap()` is acceptable in tests and when immediately preceded by a guard that guarantees `Some`/`Ok` (e.g., `.is_some()` check). + +## Step 3: Review Iterator Patterns + +Check iterator usage in public interfaces. + +**Public API iterators:** +- Use an explicit named iterator type, not `impl Iterator` (which prevents naming the return type) +- Name custom iterator types with `Iter` suffix (e.g., `EventIter`) +- Implement `FusedIterator` by default +- Implement `ExactSizeIterator` when size is known upfront +- Consider `DoubleEndedIterator` for reverse iteration support + +**For complex cases:** wrap a boxed iterator in a newtype struct rather than returning `Box`. + +Only flag iterator issues in **public** functions and types. Internal iterators using `impl Iterator` are fine. + +## Step 4: Review Async Code + +- Prefer native `async fn` in traits (Rust 1.75+) over the `async-trait` crate +- Returned futures must be `Send` when used in multi-threaded runtimes — flag `async fn` in traits that don't add a `Send` bound when the crate uses `tokio::spawn` or similar multi-threaded executors +- The `async-trait` crate is acceptable as a fallback for compatibility with older Rust versions + +## Step 5: Review Field Visibility + +Default to private struct fields. Only flag `pub` fields when they don't fit these exceptions: + +| Acceptable `pub` fields | Reason | +|--------------------------|--------| +| Newtype wrappers (`pub struct Foo(pub Bar)`) | Direct inner-type access is the point | +| Stable schema/protocol types | Part of a defined wire format | + +For all other structs, provide accessor methods instead of `pub` fields: +- `fn foo(&self) -> &T` — getter +- `fn foo_mut(&mut self) -> &mut T` — mutable access +- `fn set_foo(&mut self, val: T)` — setter + +## Step 6: Review Import Organization + +Imports must be organized in three groups separated by blank lines: + +```rust +// 1. Standard library +use std::collections::HashMap; +use std::fmt; + +// 2. External and workspace dependencies +use serde::Deserialize; +use tokio::sync::Mutex; + +// 3. Crate-local modules +use crate::config::Config; +use super::utils; +``` + +Flag imports that mix these groups without blank line separators. + +## Step 7: Review Naming Conventions + +| Convention | Correct | Incorrect | +|-----------|---------|-----------| +| Getters | `fn foo(&self)` | `fn get_foo(&self)` | +| Borrowed conversion | `fn as_foo(&self) -> &Foo` | `fn to_foo(&self) -> &Foo` | +| Owned conversion | `fn to_foo(&self) -> Foo` | `fn as_foo(&self) -> Foo` (if cloning) | +| Consuming conversion | `fn into_foo(self) -> Foo` | `fn to_foo(self) -> Foo` | +| Constructors | `fn new(...) -> Self` | `fn create(...)`, `fn build(...)` | +| Iterator types | `FooIter` | `FooIterator` | +| Fallible constructors | `fn new(...) -> Result` or `fn try_new(...)` | — | + +Also check for redundant prefixes — e.g., `Event::event_type` should be `Event::kind` or `Event::ty`. + +## Step 8: Review Documentation + +Check that **public** items have doc comments following these conventions: + +- Start with a single-line summary in third person: `/// Returns the event timestamp.` +- Use American English spelling +- Cross-reference related types with `[`backtick links`]` +- Include doctests for public utility functions and SDK methods +- Unit test names should be descriptive: `tests::parse_empty_input` not `tests::test1` + +Only flag missing docs on new public items introduced in the PR. Do not flag pre-existing undocumented items. + +## Step 9: Review Declaration Order + +Within each file, the expected order is: + +1. Module-level doc comment +2. Imports (organized per Step 6) +3. Re-exports (`pub use`) +4. Module declarations (`mod`) +5. Constants and statics +6. Error types +7. Main types (structs, enums) and their impls +8. Free functions +9. Tests (`#[cfg(test)]`) + +Within a type's implementation: + +1. Struct/enum definition +2. Inherent `impl` blocks +3. Standard library trait impls (`Display`, `From`, etc.) +4. Custom/external trait impls + +Flag significant ordering violations in new code. Don't flag minor reordering in existing files. + +## Step 10: Review Versioning (if Cargo.toml changed) + +If `Cargo.toml` version was bumped, verify it follows these rules: + +| Crate Version | Breaking change | New feature | Bug fix | +|--------------|-----------------|-------------|---------| +| **1.0+** (post-1.0) | Major bump | Minor bump | Patch bump | +| **0.x** (pre-1.0) | Minor bump | Patch bump | Patch bump | + +The pre-1.0 convention aligns with Cargo's caret requirement behavior where `0.x.y` treats minor as the compatibility version. + +## Output Format + +```markdown +## Rust Review: [Summary] + +### Findings + +#### [RUST-001] Category (Severity) +**Location:** `file.rs:42` +**Issue:** Description of what's wrong +**Fix:** +\```rust +// suggested fix +\``` + +### Summary +- X issues found (Y high, Z medium) +``` + +**Severity levels:** +- **High** — will panic in production, violates public API contract, or breaks semver +- **Medium** — convention violation, naming issue, or missing documentation +- **Low** — minor style issue, ordering preference + +If no issues found: "No Rust convention issues found in the changed files." + +## What NOT to Flag + +- Pre-existing code not modified in this PR +- Test files (unless public test utilities) +- `.unwrap()` in tests +- Generated code or build scripts +- One-time setup concerns (lint configuration, Clippy settings, IDE setup) +- Formatting issues (rely on `rustfmt` for this)