From b5e565c0ce572ffd820f72c3a878e65f531f0739 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=AE=89=E6=AD=A3=E8=B6=85?= Date: Sun, 31 May 2026 22:19:36 +0800 Subject: [PATCH] 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 --- .../skills/code-change-verification/SKILL.md | 13 ++ .agents/skills/rust-code-quality/SKILL.md | 112 ++++++++++++++++++ .../rust-code-quality/references/checklist.md | 52 ++++++++ .../skills/security-advisory-lessons/SKILL.md | 3 + .../references/advisory-patterns.md | 16 ++- AGENTS.md | 12 ++ crates/AGENTS.md | 35 ++++++ crates/ecstore/AGENTS.md | 24 ++++ crates/notify/AGENTS.md | 7 ++ 9 files changed, 272 insertions(+), 2 deletions(-) create mode 100644 .agents/skills/rust-code-quality/SKILL.md create mode 100644 .agents/skills/rust-code-quality/references/checklist.md diff --git a/.agents/skills/code-change-verification/SKILL.md b/.agents/skills/code-change-verification/SKILL.md index b910e1716..0c2d1d716 100644 --- a/.agents/skills/code-change-verification/SKILL.md +++ b/.agents/skills/code-change-verification/SKILL.md @@ -41,6 +41,19 @@ Use this skill to review code changes consistently before merge, before release, - hidden coupling to shared helpers/constants/features - If a point is uncertain, mark it as an open question instead of guessing. +#### Rust-specific checks (apply to all Rust changes) + +- **unwrap/expect in production**: Search changed files for `.unwrap()` and `.expect(` outside test modules. Every `unwrap()` in production code must have a justification comment or be replaced with `?`. +- **Silent type truncation**: Search for `as u8/u16/u32/u64/usize/i8/i16/i32/i64/isize` casts. Every `as` cast must be justified; negative-to-unsigned and large-to-small are bugs by default. Use `try_into()` or explicit clamping. +- **Unnecessary cloning**: Check `.clone()` calls in loops, per-request paths, and on structs with >5 heap-allocated fields. Consider `Arc`, references, or `Cow`. +- **Lock ordering**: If the change acquires multiple locks, verify the order matches all other call sites. Document the order in a comment. +- **Locks across .await**: Flag any `tokio::sync::RwLock`/`Mutex` guard held across an `.await` point without bounded hold time. +- **Recursion depth**: If the change adds or modifies a recursive function, verify it has a depth limit or uses iterative traversal with an explicit stack. +- **Error types**: Flag `Result<_, String>`, `Box`, and missing `Error::source()` implementations in public APIs. +- **Test assertions**: Every test function must have at least one `assert!`. Flag tests that only call code without verifying results. +- **println/eprintln**: Search changed files for `println!`/`eprintln!` outside test modules. Production code must use `tracing` macros. +- **Serde safety**: Structs deserialized from untrusted input (S3 API, user config) should have `#[serde(deny_unknown_fields)]`. + ### 4) Findings-first output - Order findings by severity: - P0: critical failure, security breach, or data loss risk diff --git a/.agents/skills/rust-code-quality/SKILL.md b/.agents/skills/rust-code-quality/SKILL.md new file mode 100644 index 000000000..0147618b9 --- /dev/null +++ b/.agents/skills/rust-code-quality/SKILL.md @@ -0,0 +1,112 @@ +--- +name: rust-code-quality +description: Enforce Rust-specific code quality rules on every code change. Use before merge to catch unwrap abuse, silent truncation, unnecessary cloning, lock ordering violations, recursion risks, and error type anti-patterns. +--- + +# Rust Code Quality Gate + +Use this skill on every Rust code change to enforce quality rules that `cargo clippy` does not catch. + +## Quick Start + +1. Identify changed `.rs` files. +2. Run automated checks on changed files. +3. Run manual review checklist on the diff. +4. Report findings; block merge if P0/P1 issues exist. + +## Automated Checks + +Run these on every changed `.rs` file (excluding test modules): + +```bash +# 1. unwrap/expect in production code +rg -n '\.unwrap\(\)|\.expect\(' | grep -v '#\[cfg(test)\]' | grep -v 'test' | grep -v 'bench' + +# 2. Silent type truncation via `as` cast +rg -n ' as (u8|u16|u32|u64|usize|i8|i16|i32|i64|isize)\b' + +# 3. String as error type +rg -n 'Result<.*String>' | grep -v test + +# 4. Box in public APIs +rg -n 'Box | grep -v test + +# 5. println/eprintln in production +rg -n 'println!\|eprintln!' | grep -v test + +# 6. Ordering::Relaxed usage (verify each is intentional) +rg -n 'Ordering::Relaxed' +``` + +## Manual Review Checklist + +For every Rust code change, verify: + +### Error Handling +- [ ] No `unwrap()` or `expect()` in production code without justification comment +- [ ] No `Result<_, String>` in public API signatures +- [ ] No `Box` in public trait/struct methods +- [ ] `Error::source()` is overridden when inner error is stored +- [ ] Error messages are actionable (what failed, with what input) + +### Type Safety +- [ ] No silent `as` truncation (negative→unsigned, large→small) +- [ ] `try_into()` or explicit clamping used for numeric conversions +- [ ] No `f64 as usize` without prior clamping + +### Concurrency +- [ ] Lock acquisition order is documented when multiple locks are used +- [ ] No `tokio::sync` write guards held across `.await` without bounded hold time +- [ ] Concurrent counters use `compare_exchange` loops, not load-then-store +- [ ] `std::sync::Mutex` in async context is held only briefly, never across `.await` + +### Memory and Performance +- [ ] No `.clone()` on structs with >5 heap-allocated fields in hot paths +- [ ] `HashMap::with_capacity()` / `Vec::with_capacity()` used when size is known +- [ ] Large buffers wrapped in `Arc` rather than cloned +- [ ] Temporary string computations use `&str` or `Cow` instead of `String` + +### Recursion Safety +- [ ] Recursive functions have a depth limit or use iterative traversal +- [ ] Tree/cache traversals handle corrupted/cyclic input safely + +### Testing +- [ ] Every test function has at least one `assert!` +- [ ] Tests use `.expect("context")` not bare `.unwrap()` +- [ ] No `println!`/`eprintln!` in production code (use `tracing`) + +### Serde +- [ ] Structs from untrusted input have `#[serde(deny_unknown_fields)]` +- [ ] `#[serde(default)]` not used on security-critical fields without validation + +### Code Hygiene +- [ ] No `#![allow(dead_code)]` at crate root +- [ ] No camelCase statics or Hungarian notation +- [ ] New string literals don't duplicate existing constants + +## Severity Classification + +- **P0 (Block merge)**: `unwrap()` in request hot path, silent truncation on user input, lock ordering violation, recursion without depth limit +- **P1 (Must fix)**: `Result<_, String>` in public API, unnecessary clone in hot path, `Box` in trait method +- **P2 (Should fix)**: Missing `assert!` in test, `println!` in production, missing `with_capacity` +- **P3 (Nice to fix)**: Naming convention violation, missing doc comment, `as_ptr()` vs `Arc::ptr_eq` + +## Output Template + +``` +## Rust Code Quality Report + +### Automated Scan +- unwrap/expect in production: N found +- as casts: N found +- String errors: N found +- println/eprintln: N found + +### Findings +- [P1] `path:line` — description + - Fix: ... + - Validation: ... + +### Verdict +PASS / BLOCKED (list blocking findings) +``` diff --git a/.agents/skills/rust-code-quality/references/checklist.md b/.agents/skills/rust-code-quality/references/checklist.md new file mode 100644 index 000000000..878713317 --- /dev/null +++ b/.agents/skills/rust-code-quality/references/checklist.md @@ -0,0 +1,52 @@ +# Rust Code Quality Checklist + +Use this as a quick pre-merge checklist for every Rust code change. + +## Critical (P0 — block merge) + +| Check | Command | +|-------|---------| +| No `unwrap()` in request/storage hot path | `rg '\.unwrap\(\)' \| grep -v test` | +| No `as` truncation on user input | `rg ' as (u32\|usize\|i32)' ` | +| Lock order consistent across call sites | Manual: trace all lock acquisitions | +| Recursive functions have depth limit | Manual: check for `max_depth` or iterative pattern | +| No `panic!`/`unwrap_or_else(panic!)` in production | `rg 'panic!\|unwrap_or_else.*panic' \| grep -v test` | + +## High (P1 — must fix) + +| Check | Command | +|-------|---------| +| No `Result<_, String>` in public API | `rg 'Result<.*String>' \| grep -v test` | +| No `Box` in public trait | `rg 'Box \| grep -v test` | +| No unnecessary `.clone()` in hot path | Manual: check loops and per-request paths | +| `Error::source()` implemented when inner error stored | Manual: check `impl Error` | +| No `eprintln!`/`println!` in production | `rg 'println!\|eprintln!' \| grep -v test` | + +## Medium (P2 — should fix) + +| Check | Command | +|-------|---------| +| Tests have assertions | Manual: check for `assert` in test functions | +| `HashMap`/`Vec` use `with_capacity` when size known | Manual: check `::new()` in loops | +| No `#![allow(dead_code)]` at crate root | `rg 'allow.dead_code' \| grep 'lib.rs'` | +| Serde structs from untrusted input have `deny_unknown_fields` | Manual: check `#[derive(Deserialize)]` | + +## Low (P3 — nice to fix) + +| Check | Command | +|-------|---------| +| No camelCase statics | `rg 'static ref [a-z]' ` | +| `Arc::ptr_eq` instead of `as_ptr + ptr::eq` | `rg 'as_ptr\|ptr::eq' ` | +| Public functions have doc comments | `rg 'pub fn' \| grep -v '///'` | + +## Quick One-Liner + +```bash +# Run all automated checks on changed files +CHANGED=$(git diff --name-only HEAD~1 -- '*.rs' | grep -v test | grep -v bench) +echo "=== unwrap/expect ===" && rg -c '\.unwrap\(\)|\.expect\(' $CHANGED 2>/dev/null +echo "=== as casts ===" && rg -c ' as (u8|u16|u32|u64|usize|i8|i16|i32|i64|isize)\b' $CHANGED 2>/dev/null +echo "=== String errors ===" && rg -c 'Result<.*String>' $CHANGED 2>/dev/null +echo "=== println ===" && rg -c 'println!|eprintln!' $CHANGED 2>/dev/null +echo "=== Ordering::Relaxed ===" && rg -c 'Ordering::Relaxed' $CHANGED 2>/dev/null +``` diff --git a/.agents/skills/security-advisory-lessons/SKILL.md b/.agents/skills/security-advisory-lessons/SKILL.md index a3efd6c44..43cf4069c 100644 --- a/.agents/skills/security-advisory-lessons/SKILL.md +++ b/.agents/skills/security-advisory-lessons/SKILL.md @@ -70,6 +70,7 @@ For the full pattern map, read [advisory-patterns.md](references/advisory-patter - Never join untrusted bucket/object/RPC path strings onto filesystem roots without normalization and boundary checks. - Reject or safely handle `..`, absolute paths, URL-encoded traversal, platform separators, empty components, and paths that canonicalize outside the intended root. - Validate both S3 object-key paths and internode/RPC disk paths; storage helpers can bypass S3 authorization if they trust already-parsed paths. +- Archive auto-extract paths are object keys too. Validate tar/zip entry names before IAM checks and before storage writes, and prove cleaned paths cannot cross bucket or prefix boundaries. ### Secrets, default credentials, and crypto - Do not ship hard-coded shared tokens, HMAC secrets, private keys, or production test keys. @@ -86,6 +87,7 @@ For the full pattern map, read [advisory-patterns.md](references/advisory-patter - Treat all RPC payload bytes as attacker-controlled. Replace `unwrap`, `expect`, and panic-prone deserialization with typed errors. - Malformed request tests should cover empty bytes, truncated MessagePack/protobuf, invalid enum values, stale timestamps, and invalid signatures. - RPC authentication must be independently strong; do not depend on S3 admin credentials unless the fallback is explicit and safe. +- RPC signatures must bind the exact generated gRPC method path, timestamp, and request method. Service-prefix signatures must not authorize a different concrete NodeService call. ### Browser, CORS, and console surfaces - Do not reflect arbitrary `Origin` while also allowing credentials. Default CORS should be no CORS unless explicitly configured. @@ -115,5 +117,6 @@ Use these prompts while reviewing a diff: - Could a low-privileged authenticated user reach this path with the wrong action, parent, bucket, or source object? - Does a public/default/empty config change security behavior from fail-closed to fail-open? - Is any attacker-controlled value later used as a path, policy condition, credential identity, log field, URL, Origin, or response body? +- Is an archive entry, object key, or policy resource normalized differently between authorization and storage? - Is the same operation implemented in multiple paths, such as `CopyObject` vs `UploadPartCopy`, and do all paths enforce the same security contract? - Does the test prove the exploit form is denied, or only that the intended form still works? diff --git a/.agents/skills/security-advisory-lessons/references/advisory-patterns.md b/.agents/skills/security-advisory-lessons/references/advisory-patterns.md index 6cf4c2a87..a28c7d887 100644 --- a/.agents/skills/security-advisory-lessons/references/advisory-patterns.md +++ b/.agents/skills/security-advisory-lessons/references/advisory-patterns.md @@ -38,6 +38,7 @@ Update this file only when an advisory adds or changes a reusable lesson, affect - `GHSA-pq29-69jg-9mxc`: RPC `read_file_stream` joined untrusted paths under a volume directory without canonical boundary checks. Lesson: `PathBuf::join` plus length checks are not path security. - `GHSA-8r6f-hmq2-28rg`: object keys containing traversal sequences bypassed bucket/object authorization when mapped to filesystem paths. Lesson: reject traversal at object-key parsing and verify final storage paths remain under the expected bucket/key root. +- `GHSA-f4vq-9ffr-m8m3`: Snowball auto-extract accepted archive entries such as `../victim-bucket/object`, authorized the raw attacker-bucket path, then storage path cleaning crossed bucket boundaries. Lesson: archive entries become object keys and need traversal rejection plus consistent authz/storage normalization before writes. ### Secrets, defaults, and cryptographic misuse @@ -55,6 +56,7 @@ Update this file only when an advisory adds or changes a reusable lesson, affect - `GHSA-gw2x-q739-qhcr`: malformed gRPC `GetMetrics` payloads reached `unwrap()` on deserialization and caused remote DoS. Lesson: every network/RPC deserialization failure returns an error, not a panic. - `GHSA-h956-rh7x-ppgj` and `GHSA-r5qv-rc46-hv8q`: weak RPC auth increased reachability of otherwise internal handlers. Lesson: panic bugs become more severe when internode auth is weak or defaulted. +- `GHSA-c667-rgrv-99vj`: NodeService authentication signed the service prefix instead of the concrete generated method path, so valid metadata for one RPC could be replayed to another method during the timestamp window. Lesson: RPC HMAC payloads must bind exact gRPC method path, HTTP method surrogate, timestamp, and secret. ### Browser, CORS, and console isolation @@ -69,6 +71,13 @@ Update this file only when an advisory adds or changes a reusable lesson, affect - `GHSA-xrrf-67jm-3c2r`: SSE metadata reported encryption while reader composition bypassed `EncryptReader` and stored plaintext. Lesson: test actual bytes on disk and wrapper order, not only API metadata. +### Serde deserialization and input validation + +- No `#[serde(deny_unknown_fields)]` found across the entire codebase. Lesson: all structs deserialized from untrusted input (S3 API XML/JSON, lifecycle rules, bucket policies, replication configs) should have `#[serde(deny_unknown_fields)]` to reject malformed or adversarial payloads. +- `#[serde(default)]` on security-critical fields silently accepts missing values as zero/empty. Lesson: when a field has security implications (retention days, permissions, limits), validate the deserialized value explicitly rather than relying on defaults. +- Integer fields deserialized from user input and cast with `as` (e.g., `i32 as u32`) can wrap negative values to large positives. Lesson: validate ranges before casting; use `try_into()` or clamp. +- XML config typos (e.g., `"NoncurentDays"` instead of `"NoncurrentDays"`) are silently accepted when `deny_unknown_fields` is absent. Lesson: strict deserialization prevents silent misconfiguration that could cause data loss or unexpected retention behavior. + ## Useful Search Seeds Use these targeted searches when a diff touches security-sensitive code: @@ -76,10 +85,12 @@ Use these targeted searches when a diff touches security-sensitive code: ```bash rg -n "validate_admin_request|check_permissions|AdminAction::|deny_only|is_allowed" rustfs crates rg -n "UploadPartCopy|upload_part_copy|CompleteMultipart|PostObject|content-length-range|starts-with" rustfs crates -rg -n "PathBuf::join|canonicalize|\\.\\.|x-forwarded-for|x-real-ip|SourceIp" rustfs crates +rg -n "normalize_extract_entry_key|Snowball|auto-extract|PathBuf::join|canonicalize|\\.\\.|x-forwarded-for|x-real-ip|SourceIp" rustfs crates rg -n "DEFAULT_SECRET|DEFAULT_ACCESS|TEST_PRIVATE_KEY|rustfs rpc|RUSTFS_RPC_SECRET" rustfs crates +rg -n "TONIC_RPC_PREFIX|verify_rpc_signature|check_auth|NodeServiceServer|x-rustfs-signature" rustfs crates rg -n "debug!|trace!|info!|error!|\\?resp|\\?merged_config|session_token|secret_key" rustfs crates rg -n "HashReader|EncryptReader|SSE|server-side encryption|Access-Control-Allow-Credentials|Origin" rustfs crates +rg -n "deny_unknown_fields|serde.default|as u32|as usize|as i32" rustfs crates ``` ## Minimum Regression Test Expectations @@ -87,7 +98,8 @@ rg -n "HashReader|EncryptReader|SSE|server-side encryption|Access-Control-Allow- - Authz fixes: include unauthenticated, valid low-privilege, wrong-action, correct-action, owner, non-owner, and root/admin cases as applicable. - IAM fixes: include import/update/list service-account cases with attacker-controlled parent, claims, access key, secret key, and policy. - Copy/upload fixes: include cross-bucket, cross-user, source-denied, destination-denied, copy-source-condition, and multipart completion cases. -- Path fixes: include encoded traversal, absolute path, nested traversal, valid object keys that resemble traversal text but should be rejected, and canonical boundary checks. +- Path fixes: include encoded traversal, absolute path, nested traversal, archive entries with `..`, valid object keys that resemble traversal text but should be rejected, and canonical bucket/prefix boundary checks. - Logging fixes: assert redacted output for structs and response bodies that may contain credentials. +- RPC auth fixes: include captured metadata replay across two concrete methods, stale timestamps, wrong path, wrong method surrogate, wrong secret, and valid same-method calls. - Browser/CORS fixes: assert no credentials on reflected/default origins, correct behavior for explicit allowlists, and no same-origin script execution for previewed object content. - SSE fixes: inspect stored bytes and verify API metadata, read-back behavior, and on-disk ciphertext together. diff --git a/AGENTS.md b/AGENTS.md index 32346b400..bed363174 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -103,6 +103,18 @@ Do not open a PR with code changes when the required checks fail. - Use environment variables or vault tooling for sensitive configuration. - For localhost-sensitive tests, verify proxy settings to avoid traffic leakage. +## Serde Safety + +- Add `#[serde(deny_unknown_fields)]` to structs deserialized from untrusted input (S3 API XML/JSON, lifecycle rules, bucket policies, replication configs). +- When `deny_unknown_fields` is impractical (backward compatibility), at minimum log unknown fields at `warn` level. +- Never use `#[serde(default)]` on security-critical fields without explicit validation of the resulting value. + +## Naming Conventions + +- Follow Rust API Guidelines for naming: `SCREAMING_SNAKE_CASE` for statics and constants, `snake_case` for functions and variables, `PascalCase` for types. +- Do not use camelCase or Hungarian notation (e.g., `globalDeploymentIDPtr` → `GLOBAL_DEPLOYMENT_ID`). +- If existing code violates naming conventions, do not widen the violation in new code. Fix opportunistically when touching the surrounding area. + ## Scoped Guidance in This Repository - `.github/AGENTS.md` diff --git a/crates/AGENTS.md b/crates/AGENTS.md index aa02b5102..d068c4b78 100644 --- a/crates/AGENTS.md +++ b/crates/AGENTS.md @@ -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` or `Box` 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. diff --git a/crates/ecstore/AGENTS.md b/crates/ecstore/AGENTS.md index 06a577cb4..10101f55e 100644 --- a/crates/ecstore/AGENTS.md +++ b/crates/ecstore/AGENTS.md @@ -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` 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: diff --git a/crates/notify/AGENTS.md b/crates/notify/AGENTS.md index f253eb024..7a7acb738 100644 --- a/crates/notify/AGENTS.md +++ b/crates/notify/AGENTS.md @@ -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