From 8ace340694af9e58d41e6652c1c6dda0e0ba8462 Mon Sep 17 00:00:00 2001 From: Zhengchao An Date: Fri, 17 Jul 2026 18:59:59 +0800 Subject: [PATCH] docs(agents): add anti-bloat guidelines and simplicity adversary role (#4975) Add Reuse Before You Write and Necessary Code Only sections to AGENTS.md, promote quality probes into a dedicated seventh adversarial-validation role (simplicity adversary), extend rust-code-quality checks, and dedupe code-change-verification's restated Rust checklist into a pointer. --- .../skills/adversarial-validation/SKILL.md | 20 ++++++--- .../skills/code-change-verification/SKILL.md | 11 +---- .agents/skills/rust-code-quality/SKILL.md | 20 ++++++--- AGENTS.md | 43 ++++++++++++------- 4 files changed, 58 insertions(+), 36 deletions(-) diff --git a/.agents/skills/adversarial-validation/SKILL.md b/.agents/skills/adversarial-validation/SKILL.md index f2745fbef..eb5c7f184 100644 --- a/.agents/skills/adversarial-validation/SKILL.md +++ b/.agents/skills/adversarial-validation/SKILL.md @@ -1,6 +1,6 @@ --- name: adversarial-validation -description: Execute the Adversarial Validation policy from the root AGENTS.md — run the six reviewer roles (correctness, security, concurrency/durability, compatibility, performance, test coverage) with RustFS-specific attack probes. Use on every behavior-affecting code change, bug fix, or design proposal before declaring it done. +description: Execute the Adversarial Validation policy from the root AGENTS.md — run the seven reviewer roles (correctness, simplicity, security, concurrency/durability, compatibility, performance, test coverage) with RustFS-specific attack probes. Use on every behavior-affecting code change, bug fix, or design proposal before declaring it done. --- # Adversarial Validation Playbooks @@ -53,14 +53,22 @@ shipped bug or rule that earns each probe its place. - Exercise the zero/empty end of every new size or count parameter: zero-length object PUT then GET (body must be empty, not error), part count 0, empty Vec of disks/entries into aggregation functions, and env/config values of 0 (must clamp or reject, never divide-by-zero or 'scan nothing and report zero usage'). Anywhere the diff computes a ratio, capacity, or progress percentage, plug in 0 and the max value. - Where: crates/ecstore aggregation and scanner paths; crates/object-capacity; config/env parsing in touched crates - Evidence: Commits 787cc77a7 'clamp zero capacity env values to safe defaults' (#4559) and 32b1094ec 'resolve a symlinked scan root instead of silently counting zero' (#4564) — zero-as-silent-wrong-answer is a recurring repo bug class. -- Smaller-diff attack: rewrite the diff's change mentally (or actually, in scratch) as the minimal in-place edit and compare. Flag as findings: a helper function with exactly one caller introduced by this diff; a file rewrite where a 3-line edit inside the existing control flow suffices; reshaped control flow in init/locking/metadata/quorum paths beyond what the fix requires; new string literals duplicating existing constants (grep the token first); #[path] module inclusion. If the smaller diff achieves identical behavior, report it with the concrete replacement. - - Where: Any diff; extra scrutiny for crates/ecstore, crates/lock, rustfs/src/storage where 'preserve the existing control-flow shape' is an explicit rule - - Evidence: AGENTS.md 'Change Style for Existing Logic' (one-off helper ban, preserve control-flow shape in distributed/locking/metadata paths, no #[path]) and 'Constant and String Usage'; Adversarial Validation section names the smaller-diff clause as a correctness-adversary finding. - For any diff touching multipart or object commit paths, order the operations on paper and attack the failure point between them: kill the process (or return Err) after the commit rename but before cleanup, and after cleanup but before commit. Verify the earlier-failure case leaves the object readable and the later-failure case leaves no half-visible object; part meta files must never be deleted before the commit is durable. - Where: crates/ecstore multipart commit/cleanup (set_disk/ops); rustfs/src/storage multipart handlers - Evidence: Commit c77c5f047 'defer multipart part.N.meta cleanup until after commit' (#4548) — cleanup-before-commit ordering already caused a real data-loss window; the #4221 durability work shows fsync/ordering bugs are endemic here. -Null report example: "Attacked quorum-1 error reduction, exact max-keys listing boundary, trailing-slash dir keys, nil-UUID tier versionId, mid-stream reconstruct error propagation, and a minimal-diff rewrite — no break found; diff is already the minimal in-place edit." +Null report example: "Attacked quorum-1 error reduction, exact max-keys listing boundary, trailing-slash dir keys, nil-UUID tier versionId, and mid-stream reconstruct error propagation — no break found." + +### Simplicity adversary + +- Smaller-diff attack: rewrite the diff's change mentally (or actually, in scratch) as the minimal in-place edit and compare. Flag as findings: a helper function with exactly one caller introduced by this diff; a file rewrite where a 3-line edit inside the existing control flow suffices; reshaped control flow in init/locking/metadata/quorum paths beyond what the fix requires; new string literals duplicating existing constants (grep the token first); #[path] module inclusion. If the smaller diff achieves identical behavior, report it with the concrete replacement. + - Where: Any diff; extra scrutiny for crates/ecstore, crates/lock, rustfs/src/storage where 'preserve the existing control-flow shape' is an explicit rule + - Evidence: AGENTS.md 'Change Style for Existing Logic' (one-off helper ban, preserve control-flow shape in distributed/locking/metadata paths, no #[path]) and 'Reuse Before You Write' (constants clause); the Adversarial Validation roles list charters the simplicity adversary with exactly this attack. +- Reuse-and-necessity attack: for each new helper the diff introduces, run `ls crates/utils/src crates/common/src` and `rg -i 'fn \w*'` over those dirs plus the touched crate (snake_case signatures — a full-text single-word grep drowns, a multi-word phrase returns nothing). A reimplementation of an existing workspace utility, or of plain std/tokio behavior no wrapper refines, is a finding — but so is forced reuse with mismatched semantics (normalization such as `clean` resolving `.`/`..` against raw S3 keys, error type, backoff, durability gating). For each new defensive branch, demand the nameable trigger and flag re-validation of what a validated upstream layer on the SAME path already guarantees — excluding the Cross-Cutting Domain Invariant patterns (nil/empty/absent UUID, dual metadata keys, unversioned-tier versionId) and re-checks before destructive actions, which are load-bearing even when redundant on the happy path. For each new test, flag near-duplicates pinning the same code path AND poison-value class as an existing test — boundary companions (n==max vs max+1, absent vs empty vs nil UUID, MetaObject vs MetaDeleteMarker) are never near-duplicates; the test-coverage skeptic playbook below mandates them. + - Where: Any diff adding helpers, branches on decoded/peer data, or tests; helper checks against crates/utils, crates/common, and the touched crate + - Evidence: AGENTS.md 'Reuse Before You Write' and 'Necessary Code Only'; GHSA-f4vq-9ffr-m8m3 (normalization-asymmetry traversal — why forced reuse of normalizing helpers on raw keys is itself an attack); docs/operations/tier-ilm-debugging.md nil-versionId incident (why boundary re-checks are load-bearing). + +Null report example: "Rewrote the diff as an in-place edit (no smaller equivalent exists), grepped both new helpers against crates/utils, crates/common, and the touched crate (no existing equivalent; call-site semantics checked), verified the two new defensive branches name concrete corrupt-input triggers, and checked the added tests against the existing suite (each pins a distinct poison-value class) — no break found." ### Security reviewer @@ -243,7 +251,7 @@ Null report example: "Attacked the new rename_data commit-section work, durabili - If the diff writes internal object metadata, run the dual-key mutation: delete the `x-minio-internal-` write (keeping only `x-rustfs-internal-`) and check whether any test fails. Because `get_bytes` prefers the RustFS key, every read-back test stays green while MinIO interop is silently broken — coverage must include an assertion that BOTH keys are present in the stored metadata map. - Where: crates/utils/src/http/metadata_compat.rs and all its callers in crates/ecstore and rustfs/src/storage - Evidence: CLAUDE.md domain convention: metadata must be written under both x-rustfs-internal- and x-minio-internal- keys for MinIO interop; get_bytes prefers the RustFS key, making the MinIO-key half of the invariant invisible to read-back tests. -- For changed quorum/version/UUID logic, name the tests covering the specific poison values: quorum−1 disks, nil UUID, absent vs empty vs nil-serialized UUID bytes, and remote-tier version_id of None/"" (unversioned tier bucket → no versionId sent). Mutation check: remove a `.filter(|u| !u.is_nil())` guard from the diff and confirm a test fails; if none does, the nil-UUID class is uncovered. +- For changed quorum/version/UUID logic, name the tests covering the specific poison values: quorum−1 disks, nil UUID, absent vs empty vs nil-serialized UUID bytes, remote-tier version_id of None/"" (unversioned tier bucket → no versionId sent), and the same metadata read on both MetaObject and MetaDeleteMarker version types. Mutation check: remove a `.filter(|u| !u.is_nil())` guard from the diff and confirm a test fails; if none does, the nil-UUID class is uncovered. - Where: crates/ecstore (tier recovery, heal, quorum paths), crates/filemeta, code reading UUIDs from xl.meta metadata - Evidence: Commit 726f3dc18 (#4552) fixed rejection of empty remote version_id in tier recovery. CLAUDE.md invariant: absent/empty/nil UUID all mean 'no value', not Uuid::nil(). docs/operations/tier-ilm-debugging.md: None/"" tier version means unversioned bucket. df9cbc4ed (#4427): unvalidated distribution values caused shuffle index panic — edge values reached production untested. - For any pagination/limit/truncation change, construct the exact-boundary test: result count == max (page exactly full), max+1, and a delimiter re-fold that lands precisely on the page boundary — assert both the item count AND the is_truncated/continuation marker. Off-by-one at the page boundary is a recurring shipped bug here. diff --git a/.agents/skills/code-change-verification/SKILL.md b/.agents/skills/code-change-verification/SKILL.md index 0c2d1d716..553822c82 100644 --- a/.agents/skills/code-change-verification/SKILL.md +++ b/.agents/skills/code-change-verification/SKILL.md @@ -43,16 +43,7 @@ Use this skill to review code changes consistently before merge, before release, #### 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)]`. +Run the full checklist in [rust-code-quality](../rust-code-quality/SKILL.md) — the canonical Rust review checklist for the unwrap/casting/cloning/locking/recursion/error-type/serde/test rules and the reuse-and-necessity checks (duplicated helpers, defensive branches without a nameable trigger, redundant error wrapping). Do not restate those rules here; carry its P0–P3 ratings over unchanged and use this skill's output format. ### 4) Findings-first output - Order findings by severity: diff --git a/.agents/skills/rust-code-quality/SKILL.md b/.agents/skills/rust-code-quality/SKILL.md index 0147618b9..f7df363ad 100644 --- a/.agents/skills/rust-code-quality/SKILL.md +++ b/.agents/skills/rust-code-quality/SKILL.md @@ -36,6 +36,9 @@ rg -n 'println!\|eprintln!' | grep -v test # 6. Ordering::Relaxed usage (verify each is intentional) rg -n 'Ordering::Relaxed' + +# 7. Default substituted for a possibly-required value (judge each: is the value optional by domain?) +rg -n 'unwrap_or_default\(\)|unwrap_or\(' ``` ## Manual Review Checklist @@ -55,8 +58,8 @@ For every Rust code change, verify: - [ ] 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 +- [ ] Lock acquisition order is documented when multiple locks are used, and matches every other call site taking any overlapping subset (ABBA check) +- [ ] No `tokio::sync` lock guard (read or write) held across `.await` without bounded hold time — long-lived read guards wedge writers (#4195) - [ ] Concurrent counters use `compare_exchange` loops, not load-then-store - [ ] `std::sync::Mutex` in async context is held only briefly, never across `.await` @@ -84,12 +87,19 @@ For every Rust code change, verify: - [ ] No camelCase statics or Hungarian notation - [ ] New string literals don't duplicate existing constants +### Reuse and Necessity +- [ ] No new helper duplicating an existing workspace utility (`crates/utils`, `crates/common`, the touched crate) or plain std/tokio behavior no wrapper refines; reused helpers match the call site's semantics (normalization, error type, backoff, durability gating) +- [ ] No branch without a nameable concrete trigger; no re-validation of what a validated upstream layer on the same path already guarantees (Cross-Cutting Domain Invariant patterns and pre-destructive-action re-checks are load-bearing — keep them) +- [ ] Error context attached once where actionable, not re-wrapped at every hop; no typed→generic error conversion below aggregation/quorum layers +- [ ] No comments narrating the next line, restating a signature, or describing the change itself (invariant comments — lock ordering, `SAFETY`, unwrap justification — are not narration) +- [ ] No near-duplicate test pinning the same code path and poison-value class as an existing test (boundary companions — n==max vs max+1, absent/empty/nil UUID — are never near-duplicates) + ## 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` +- **P1 (Must fix)**: `Result<_, String>` in public API, unnecessary clone in hot path, `Box` in trait method, `unwrap_or_default()` on a domain-required value (metadata, quorum, version id) +- **P2 (Should fix)**: Missing `assert!` in test, `println!` in production, missing `with_capacity`, new helper duplicating an existing workspace utility, defensive branch with no nameable trigger (corrupt or stale persisted/peer data is always a nameable trigger for boundary-crossing values), near-duplicate test, redundant error re-wrapping +- **P3 (Nice to fix)**: Naming convention violation, missing doc comment, `as_ptr()` vs `Arc::ptr_eq`, narrating comment ## Output Template diff --git a/AGENTS.md b/AGENTS.md index 6811e6d3b..fec30106a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -14,7 +14,7 @@ If repo-level instructions conflict, follow the nearest file and keep behavior a ## Execution Discipline -- Read the relevant existing code, tests, and local guidance before changing behavior. +- Read the relevant existing code, tests, and local guidance before changing behavior. For new helpers or test setup, that read includes `crates/utils`, `crates/common`, and the touched crate's own `test_util`/fixtures (see Reuse Before You Write). - State assumptions when they affect the implementation or verification path. - If a task has multiple plausible interpretations, list the options briefly and choose the narrowest reasonable path; ask when the ambiguity would make the change risky. - For multi-step work, keep the plan minimal and tied to verifiable outcomes. @@ -41,14 +41,27 @@ If repo-level instructions conflict, follow the nearest file and keep behavior a - Do not refactor existing code only to make it easier to unit test. - Keep fixes narrowly aligned with the requested behavior; avoid semantic-adjacent rewrites while touching sensitive paths. - Keep code elegant, concise, and direct. Prefer minimal, readable implementations over over-engineering and excessive abstraction. Use comments to clarify non-obvious intent and invariants, not to compensate for unclear code. +- Do not write comments that narrate what the next line does, restate a signature, or describe the change you just made — that commentary belongs in the PR description, not the code. Required invariant comments — lock ordering, `SAFETY`, unwrap justification, `#[allow(dead_code)]` rationale, `RUSTFS_COMPAT_TODO` — are never narration. - Mention unrelated issues when useful, but do not fix them as part of a narrow task. -## Constant and String Usage +## Reuse Before You Write -- Before introducing new string literals, search for existing constants/enums that already represent the same semantic value. -- Reuse existing constants for protocol labels, error identifiers, header keys, event names, metric names, command tags, and similar fixed tokens. -- If a new string is truly unique, define a local constant near related logic and avoid scattering the literal across multiple sites. -- When changing existing behavior, keep naming and format consistency by aligning with established project constants. +Search for an existing implementation before writing a new one; extend what exists instead of duplicating it: + +- **Helpers and utilities** (path/string handling, hashing, retry, env parsing, IO wrappers): check `ls crates/utils/src` first — file names map to operations (`retry.rs`, `envs.rs`, `hash.rs`, `path.rs`, `string.rs`, `io.rs`) — plus `crates/common` (shared structures/globals), then `rg -i 'fn \w*' crates/utils/src crates/common/src /src` for signatures. Helpers are snake_case: a full-text single-word grep over a large crate drowns you and a multi-word phrase returns nothing. Reimplementing an existing workspace helper — or hand-rolling what `std`, `tokio`, or an existing workspace dependency already provides — is a review finding, not a style preference. +- **Reuse requires matching semantics, not a matching name**: before adopting a helper, check its normalization (`clean` resolves `.`/`..` — never apply it to raw S3 object keys), error type, backoff/deadline behavior, and durability gating against the call site. When semantics differ, a new narrowly-named helper with a comment naming the rejected lookalike is the correct outcome. The inverse also holds: workspace wrappers exist because raw `std`/`tokio` semantics were insufficient (durability gates, retries) — prefer the wrapper over the raw call. +- **Constants and fixed tokens** (protocol labels, error identifiers, header keys, event names, metric names, command tags): search for existing constants/enums that already represent the same semantic value and reuse them. If a value is truly new, define one local constant near related logic; never scatter the literal across sites. When changing existing behavior, align naming and format with the established constants. +- **Test scaffolding**: reuse existing test utilities and fixtures (the touched crate's own `test_util` module and `tests/fixtures`, or `crates/test-utils`) instead of writing new setup code — run `rg -l '' /src /tests` before writing a test. A new test must pin a failure mode no existing test covers. Near-duplicate means same code path AND same poison-value class: this repo's boundary companions (n==max vs max+1, absent vs empty vs nil UUID bytes, MetaObject vs MetaDeleteMarker) are distinct by definition and must all be written. + +## Necessary Code Only + +Net-new code — files, types, branches, comments — is cost to justify, not progress: + +- Validate at the trust boundary — untrusted client input, bytes read from disk, RPC payloads, config (see Serde Safety and Cross-Cutting Domain Invariants) — then trust the type: do not re-check what the type system or a validated upstream layer already guarantees, and cite the establishing check (`file:line`) when the guarantee is not obvious. +- The exception is load-bearing: a value that crossed a persistence, RPC, or version boundary is never guaranteed by the code on the other side — a peer may be older or buggy, disk bytes may be corrupt — so the Cross-Cutting Domain Invariant patterns apply at every consumer, and re-checks immediately before a destructive action (delete, overwrite, quorum decision) stay. Deleting an existing guard is a behavior change requiring adversarial review, not cleanup. +- Every new branch needs a nameable trigger: a concrete input, state, or failure that reaches it — for boundary-crossing values, corrupt or stale persisted/peer data is always nameable. If you cannot name one, do not write the branch. If the case is truly unreachable, encode the invariant in the type; where that is impossible, return a typed internal error (fail closed). `debug_assert!` is acceptable only for pure internal arithmetic on values that never crossed a disk/RPC/config boundary — never as the sole guard on decoded or peer-supplied data. +- Never substitute a default where the value is required (e.g. `unwrap_or_default()` on metadata that must exist) — that converts corruption into a wrong answer. Return the typed error instead: explicit failure over implicit success. +- Attach error context once, at the layer where it is actionable: re-wrapping equivalent context at every hop is noise, and expanding a fallible chain into nested `match` blocks where `?` or a combinator suffices is a finding. Never add context by converting a typed error into a generic variant below an error-aggregation or quorum layer (`reduce_errs` classifies by variant equality) — context there belongs in a `tracing` event, not the error value. ## Sources of Truth @@ -141,7 +154,7 @@ Pick the tier from the riskiest file touched; when in doubt, pick the higher. - **Exempt:** docs/comments/instruction-only changes, formatting, typos with no runtime surface. Skip this section. - **Mechanical:** pure renames, file moves, test-only or tooling changes — - correctness adversary only. + correctness and simplicity adversaries only. - **Standard (the default):** any change that affects behavior. - **High risk:** touches locking, erasure coding, quorum/heal, replication, multipart, RPC, lifecycle/tiering, metadata formats (`xl.meta`), @@ -161,9 +174,8 @@ encode this repo's shipped bugs. - **Correctness adversary** — construct a concrete input/state/interleaving that yields wrong output, data loss, or a crash. Probe error paths and edge - values (empty, nil UUID, zero-length, quorum−1, missing version). For code - diffs, a materially smaller or more idiomatic diff achieving the same - behavior is also a finding (see Change Style for Existing Logic). + values (empty, nil UUID, zero-length, quorum−1, missing version). +- **Simplicity adversary** — same behavior, less code. Hunt the materially smaller or more idiomatic diff (see Change Style for Existing Logic, Reuse Before You Write, and Necessary Code Only): reimplemented workspace helpers, one-caller extractions, rewrites where an in-place edit suffices, defensive branches with no nameable trigger, redundant error wrapping, near-duplicate tests, narration comments. A smaller diff achieving identical behavior is a finding, reported with the concrete replacement; forced reuse of a helper with mismatched semantics is equally a finding. - **Security reviewer** — authn/authz bypass, injection, secret leakage, untrusted deserialization (see Serde Safety), path traversal, timing leaks. - **Concurrency/durability reviewer** — lock ordering, races, cancellation, @@ -179,11 +191,12 @@ encode this repo's shipped bugs. wrong while all tests stay green — if one exists, coverage is insufficient. A missing test is a finding, not a note. -Standard tier: correctness adversary + test-coverage skeptic, plus every -role whose domain the diff touches (async or shared-state code → -concurrency; parsing of untrusted input → security; public crate API shape -→ compatibility; per-request or per-object hot paths → performance). -High risk: all six roles. +Standard tier: correctness adversary + simplicity adversary + test-coverage +skeptic, plus every role whose domain the diff touches (async or +shared-state code → concurrency; parsing of untrusted input → security; +public crate API shape → compatibility; per-request or per-object hot paths +→ performance). +High risk: all seven roles. ### Protocol