mirror of
https://github.com/rustfs/rustfs.git
synced 2026-08-19 02:56:18 +00:00
chore(agents): add Rust code quality rules and skill (#3144)
* docs: update security advisory lessons * chore(agents): add Rust code quality rules and skill Add rules derived from full-project code review (48 findings across 7 dimensions) to prevent recurring issues in agent-generated code. AGENTS.md changes: - crates/AGENTS.md: error type design, concurrency, recursion safety, type casting, test quality rules - root AGENTS.md: serde safety, naming conventions - crates/ecstore/AGENTS.md: allocation discipline, lock ordering, recursion safety, dead code policy - crates/notify/AGENTS.md: lock ordering for runtime_view/facade Skill changes: - code-change-verification: add Rust-specific checks (unwrap, as cast, clone, lock order, recursion, error types, test assertions) - security-advisory-lessons: add serde deserialization safety pattern - NEW rust-code-quality: automated scan + manual review checklist
This commit is contained in:
@@ -14,6 +14,41 @@ Applies to all paths under `crates/`.
|
||||
- Keep integration tests under each crate's `tests/` directory.
|
||||
- Add regression tests for bug fixes and behavior changes.
|
||||
|
||||
## Error Type Design
|
||||
|
||||
- Public API functions must return a typed error enum (preferably `thiserror`-derived), never `Result<_, String>`.
|
||||
- Do not use `Box<dyn Error>` or `Box<dyn Error + Send + Sync>` in public trait methods or struct methods. Define a concrete error type with specific variants.
|
||||
- When implementing `std::error::Error`, always override `fn source()` if you store an inner error. Breaking the error chain makes debugging impossible.
|
||||
- Internal helpers that return `Result<_, String>` and are immediately wrapped via `.map_err(Error::other)` should return the actual error type directly.
|
||||
|
||||
## Concurrency
|
||||
|
||||
- Document lock acquisition order when a module uses multiple locks. Never acquire the same set of locks in different orders across code paths.
|
||||
- Never hold a `tokio::sync::RwLock`/`Mutex` write guard across `.await` points unless the critical section is unavoidably async and the hold time is bounded.
|
||||
- Prefer `compare_exchange` loops over load-then-store for concurrent counters (peak values, adaptive heuristics).
|
||||
- When resetting multi-field atomic statistics, use a version/sequence counter or accept that concurrent readers may see partial snapshots; document the tradeoff.
|
||||
- `std::sync::Mutex` is acceptable in async context only when held for a brief, non-`await`-containing critical section. If in doubt, use `tokio::sync::Mutex`.
|
||||
|
||||
## Recursion Safety
|
||||
|
||||
- Recursive tree/graph traversals must have a depth limit (e.g., `max_depth` counter) or use an iterative approach with an explicit `Vec` stack.
|
||||
- This applies to cache trees, directory walks, and any user-influenced hierarchy.
|
||||
- A corrupted or malicious input must not be able to overflow the thread stack.
|
||||
|
||||
## Type Casting
|
||||
|
||||
- Never use `as` for numeric conversions that may truncate or overflow. Use `try_into()` with explicit error handling, or clamp with `value.max(0) as usize` when the domain is bounded.
|
||||
- `f64 as usize` saturates but is fragile; clamp to `[0, usize::MAX as f64]` first.
|
||||
- Treat every `as` cast in a PR review as a potential bug; require justification.
|
||||
|
||||
## Testing
|
||||
|
||||
- Keep unit tests close to the module they test.
|
||||
- Keep integration tests under each crate's `tests/` directory.
|
||||
- Add regression tests for bug fixes and behavior changes.
|
||||
- Every test function must contain at least one `assert!`/`assert_eq!`/`assert_matches!`. A test that only calls code without asserting is not a test.
|
||||
- In tests, prefer `.expect("context: what was being tested")` over bare `.unwrap()`. A test failure should tell you which operation failed and with what input.
|
||||
|
||||
## Async and Performance
|
||||
|
||||
- Keep async paths non-blocking.
|
||||
|
||||
@@ -14,6 +14,30 @@ Applies to `crates/ecstore/`.
|
||||
- Keep network and disk operations async-friendly; avoid introducing unnecessary blocking.
|
||||
- Benchmark-sensitive changes should include measurable rationale.
|
||||
|
||||
## Allocation Discipline in Hot Paths
|
||||
|
||||
- Do not implement `Clone` on structs with >5 heap-allocated fields without considering `Arc` for heavy fields.
|
||||
- Before cloning a struct in a loop or per-request path, check if a reference or `Cow` would suffice.
|
||||
- When a struct contains a large buffer (e.g., erasure coding block), wrap it in `Arc` and pass by reference rather than cloning.
|
||||
- Use `&str` and `Cow<str>` instead of `String` for temporary computations (header parsing, signature building, path manipulation).
|
||||
- Use `HashMap::with_capacity()` / `Vec::with_capacity()` when the size is known or estimable.
|
||||
|
||||
## Lock Ordering
|
||||
|
||||
- When a function acquires multiple locks, document the acquisition order in a comment.
|
||||
- Never acquire the same set of locks in different orders across code paths — this is a deadlock.
|
||||
- Prefer `compare_exchange` loops over load-then-store for concurrent counters.
|
||||
|
||||
## Recursion Safety
|
||||
|
||||
- Recursive tree traversals (cache trees, directory walks) must have a depth limit or use iterative traversal with an explicit `Vec` stack.
|
||||
- `flatten()`, `delete_recursive()`, `copy_with_children()`, `total_children_rec()`, `mark()` — all must handle deep or corrupted trees safely.
|
||||
|
||||
## Dead Code
|
||||
|
||||
- Do not add `#![allow(dead_code)]` at the crate root. If code is unused, remove it or gate it behind a feature flag.
|
||||
- Each `#[allow(dead_code)]` annotation must have a comment explaining why the code is kept.
|
||||
|
||||
## Cross-Module Coordination
|
||||
|
||||
- Validate behavior impacts on:
|
||||
|
||||
@@ -28,6 +28,13 @@ shared plugin/runtime primitives from `rustfs-targets`.
|
||||
- `stream.rs` is a compatibility shim; new replay/runtime work should prefer
|
||||
shared helpers in `rustfs-targets::runtime`.
|
||||
|
||||
## Concurrency
|
||||
|
||||
- `runtime_view.rs` acquires locks in order: `stream_cancellers` → `target_list`.
|
||||
- `runtime_facade.rs` acquires locks in order: `target_list` → `replay_workers`.
|
||||
- These orders must not be reversed in new code. When adding a function that needs both `target_list` and `stream_cancellers`, acquire `stream_cancellers` first (matching `runtime_view.rs` order).
|
||||
- Do not hold write guards across `.await` points unless the hold time is bounded and the operation is unavoidably async.
|
||||
|
||||
## Change Style
|
||||
|
||||
- Preserve best-effort dispatch semantics and observability signals unless the
|
||||
|
||||
Reference in New Issue
Block a user