audit: named returns must be locally provable; org default outranks file-local style

.claude-plugin/marketplace.json

@@ -10,7 +10,7 @@
       "name": "audit",
       "source": "./",
       "description": "Multi-pass codebase audit: a strictly-sequential pipeline of file-scoped review passes (process, security, test coverage, documentation, code quality, correctness/intent, hazard surface) plus triage, with findings reported (not fixed) and tracked as GitHub issues.",
-      "version": "0.20.0",
+      "version": "0.21.0",
       "author": { "name": "Rain Open Source Software Ltd" },
       "keywords": ["audit", "security", "code-review", "test-coverage", "correctness", "hazard", "solidity"]
     }

.claude-plugin/plugin.json

@@ -1,7 +1,7 @@
 {
   "name": "audit",
   "displayName": "Audit",
-  "version": "0.20.0",
+  "version": "0.21.0",
   "description": "Multi-pass codebase audit (process, security, test coverage, documentation, code quality, correctness/intent verification, hazard surface) plus triage. Passes run strictly sequentially; file-scoped subagents within each pass read one file in full and report findings (not fixes), tracked as GitHub issues.",
   "author": {
     "name": "Rain Open Source Software Ltd",

skills/audit/SKILL.md

@@ -1,7 +1,7 @@
 ---
 name: audit
 description: Full codebase audit — seven review dimensions (process, security, test coverage, documentation, code quality, correctness/intent, hazard surface) plus triage. Reviews EVERY source file across all languages as a whole-repo snapshot (not a diff), reports problems (never fixes them, never "works correctly"), severity-rates each, attaches a concrete proposed fix, and tracks findings as GitHub issues; triage then re-validates each finding against live source and applies fixes TDD-style. Triggers on "audit this codebase", "security review", "full audit", "review the whole repo for bugs/coverage/docs/quality/correctness/hazards", "find what's wrong before an external audit".
-version: 0.19.0
+version: 0.21.0
 ---
 
 # Codebase Audit (whole-repo, multi-dimension)
@@ -53,6 +53,16 @@ Exclude auto-generated files (bindings, build artifacts, `*.pointers.sol` and si
 - **Rounding direction.** All rounding from precision loss (Float→fixed-decimal conversions, integer division) MUST favor **non-interactive** participants (order owner, contract, protocol — contracts count as non-interactive). For each rounding op: identify WHO is non-interactive, WHICH direction it rounds, WHETHER that direction favors the non-interactive party. Flag any rounding that favors **interactive** participants (`msg.sender`, arb callers, external protocols).
 - **Variable naming.** Flag short/meaningless names (single chars `r`/`n`/`x`, abbreviations `ob`/`cfg`/`val`) for closure params, loop vars, or local bindings as **LOW** (e.g. `raindex_cfg` not `ob`, `network_key` not `nk`). Short-but-meaningful names (`id`, `url`, `key` when unambiguous) are fine.
 - **Solidity storage-class naming (rainlanguage).** In Solidity, encode a variable's storage class in its name so its nature is legible at the *use* site, not just its declaration: **immutables** are `i` + UpperCamel (`iBeacon`, `iOrchestratorBeacon`); **mutable storage** (state variables that are neither `constant` nor `immutable`) are `s` + UpperCamel (`sBalance`); **locals and function parameters** contain **no underscore** — `fooBar`, never `_fooBar` (leading) or `foo_bar` (internal), since the leading underscore is reserved for the internal-function convention. Flag an `immutable` not named `iFoo`, a mutable state var not named `sFoo`, and any local/param containing an underscore. Applies to all Solidity in scope, **tests and scripts included**; an interface-mandated parameter name or a documented, deliberate exception may be allow-listed, but default to flagging. **LOW/INFO.** (This is Solidity-only — Rust/TS keep their own idioms, e.g. Rust `snake_case` locals are correct.)
+- **Solidity named returns must be locally provable (rainlanguage).** The naming rules above reach declarations, locals and params but NOT **return declarations**, and a named return is where Solidity silently supplies a zero/empty default. Unnamed, that same fall-through draws solc's *"Unnamed return variable can remain unassigned. Add an explicit return with value to all non-reverting code paths **or name the variable**"* — so naming it suppresses the compiler's own diagnostic, by the compiler's own suggestion (identical on solc 0.8.25 and 0.8.35: every unnamed shape with a non-reverting path to the end warns, named ones never do, and the sole unnamed exception is a completely empty body). It is NOT a compile error either way. Since dimension 4 #4 rates a build warning **LOW or higher**, this rule is the audit standing in for the signal the name removed. The finding is the subset whose assignment **cannot be proved from the declaration's own function**, where *provable* means: on **every path that reaches the function's end** (its closing brace or a bare `return;`), the name is either assigned or the path reverts — judged from that function's body alone. A path reaching the end unassigned returns the zero/empty value. Judge nesting by that test rather than by the syntax: a total `if`/`else` (or a `try`/`catch` assigning in both arms) nested inside a loop or another conditional is NOT total, because the enclosing construct can be skipped.
+  - **Assigned only inside a loop or a branch**, where a guard elsewhere in the function is what makes every path assign it — provable, but only via that guard, so correctness rests on code far from the declaration (if no guard makes it total, it is the next case instead). Worked example: `rainlanguage/rain.deploy`'s `LibRainDeploy.deployToNetworks` declares `returns (address deployedAddress)` and assigns it only in the two arms of an `if`/`else` **inside the `for` loop over `networks`** — safe solely because an `if (networks.length == 0) revert NoNetworks();` at the top of the function guarantees at least one iteration. Weaken or drop that guard and the function returns `address(0)` while still compiling — silently, since the name is what suppresses the warning — and nothing catches the zero: every check in the function is either *before* the loop (so it has already run against inputs, not against the return) or *inside* it (so it is skipped along with the loop — including the codehash check on `deployedAddress` itself), and there is none after it. The hazard is that the proof sits three levels away from the declaration. **LOW.**
+  - **Never assigned on a reachable path** — no guard makes it total, so a silent zero/empty return where the unnamed form would have drawn the warning above. This is the same fail-open shape as **Fail-closed on input** above, with the zero value as the permissive default. **MEDIUM**, and higher on that rule's own scale by what the zero gates — **CRITICAL/HIGH** where it bypasses auth, lifts a spend/write bound, or destroys data. Read the sense before rating it: `returns (bool valid)` falling through to `false` fails *closed*, but a `returns (bool blocked)` / `(bool paused)` falling through to `false` reads as "proceed", and a `returns (address recipient)` falling through to `address(0)` sends funds nowhere — those are not MEDIUMs.
+  - **Shadowed on every path by an explicit `return`** — the name is a dead declaration and pure reader tax. **LOW.**
+  - **Last-iteration-wins** — a named return assigned once per loop iteration returns only the final one. Benign when every iteration computes the same value (as in the example above: a Zoltu address is deterministic, so every network yields the same one), a real defect when they can diverge; this is **Explicit handling of ambiguous sets** above reached through a return declaration rather than a `.find`. **LOW**, **MEDIUM** when the iterations can diverge, and **HIGH** on that rule's own scale when the value returned drives a verdict, permission, transfer, or destructive action.
+  - **Mixed forms introduced by one diff** — cheap and **authoring-time only**, for code the diff *adds*, where the fix is free. The shape is one diff adding adjacent helpers in both forms — a `returns (address derivedAddress)` filled by implicit assignment with no `return`, beside sibling `returns (address)` / `returns (bytes32)` helpers with an explicit `return`. Pick one form for the code the diff adds. **INFO.**
+
+  The first four stand regardless of when the code was written; the mixed-forms case is new-code only. **This is NOT a ban on named returns and NOT a mandate to convert them.** A named return sometimes genuinely simplifies the source — a value accumulated across branches, a slot written from an `assembly` block, a multi-value tuple whose component names are the documentation — and converting an existing one to unnamed means adding an explicit `return` to every path of a function whose paths you must first prove you enumerated: a semantic-risk edit bought for a style win, worst in exactly the audited deploy code where that trade is least affordable. So the proposed fix for the loop/branch case is NEVER "unname it" — it is to make the assignment **locally provable** (assign a default at the top of the function, or hoist the guard's invariant into the declaration), which is a smaller and safer change than the refactor.
+
+  **Non-findings, stated so a reviewer does not reach for them:** a named return documenting a component of a multi-value tuple; a named return referenced from an `assembly` block; one assigned unconditionally on entry, or on every branch of a total `if`/`else`; and a file whose local convention is named returns, absent one of the four cases above. The first two of those justify the **named form**, not the assignment: a name written from `assembly`, or documenting a tuple component, is still a case 1/2/4 finding when a path reaches the function's end unassigned, and the fix there is the local proof, never unnaming it. Case 3 conversely does not reach a name that IS the documentation: in a partially-named tuple (`returns (bool ok, bytes32)`) the name is what tells a reader which component is which, so an explicit `return` beside it is not reader tax — and there it is the *unnamed* sibling that solc warns about, not the named one. Cases 2 and 4 are defects **Fail-closed on input** and **Explicit handling of ambiguous sets** reach by another route: file such a finding **once**, here, taking the severity from whichever scale rates it highest. Consistency with the org default is **INFO** at most and NEVER justifies a sweep (unnamed is the overwhelming org default — ~87% of the return components of function *declarations* under `src/`, generated files excluded, measured across `rain.erc4626.words`, `rain.math.float`, `rain.interpreter` and `rain.deploy`; count the `returns` of function *types* too and it is ~88%, so state the unit if you re-measure — but a per-file count is not a mandate; see also dimension 4 #1 on judging any convention against the widest scope observable rather than the surrounding file).
 - **Solidity/Foundry test rules.** Always use **specific** revert expectations — never bare `vm.expectRevert()` (matches any revert, can pass for the wrong reason). Use `vm.expectRevert(abi.encodeWithSelector(Error.selector, args...))` or `vm.expectRevert(Error.selector)`; only `vm.expectRevert(bytes(""))` (with a comment) when a revert genuinely carries no data. Per-test `forge-config: default.fuzz.runs` overrides exist intentionally for slow fuzz tests — don't remove without benchmarking (run it, check timing); conversely add a reduced-runs override when an un-overridden fuzz test takes more than a few seconds.
 - **Derived constants carry a re-derivation test.** A **derived constant** is a hardcoded literal that is the output of a documented formula/derivation. It is a *second source of truth* for a value that already has a canonical derivation, so without a test that recomputes it the two silently diverge — a mistyped slot reads the wrong storage, a stale `TYPEHASH` breaks signature verification, a wrong codehash admits the wrong bytecode. **The formula in a comment is unenforced; only a test enforces it.** For every derived constant, require a test that RE-DERIVES the value from its documented inputs and asserts equality against the literal; a bare literal with a `// keccak256(...)` comment and no such test is a finding. Covers (non-exhaustive): **ERC-7201 storage slots** (`keccak256(abi.encode(uint256(keccak256(<namespace>)) - 1)) & ~bytes32(uint256(0xff))` — the full compliance checklist is in the Security dimension); **EIP-712** domain separators and `*_TYPEHASH` constants; **keccak-derived ids** (role/permission ids, deployment-suite ids, selectors pinned as literals); **codehashes** (`keccak256(RUNTIME_CODE)` pinned as `bytes32`); **deterministic deploy addresses** (Zoltu/CREATE2 addresses pinned as `address`, re-derivable via the CREATE2 formula over the creation code); and **bitmasks / offsets / sizes** derived from a struct layout or spec. Shape:
   ```solidity
@@ -144,7 +154,7 @@ Review all documentation for **completeness and accuracy** against the implement
 
 ### 4. Code quality
 Review for maintainability, consistency, and good abstractions across the whole repo:
-1. **Style consistency** — similar code using different patterns for the same thing.
+1. **Style consistency** — similar code using different patterns for the same thing. **Establish the convention from the WIDEST scope you can actually observe before judging any one file — never from the surrounding file alone.** A file can itself be the outlier, and measuring locally then *inverts* the finding: the deviating majority inside that one file reads as the standard, and the conventional minority beside it gets flagged as the deviation. `rainlanguage/rain.deploy`'s `LibRainDeploy.sol` is predominantly named returns (on `main`, 5 named return components to 1 unnamed) against an org default that is overwhelmingly unnamed — so a reviewer measuring file-locally blesses the named returns and flags the unnamed sibling as the inconsistency, which is exactly backwards. So take the convention, in this order: from a **Domain rule** that states it, where one exists — those rules carry the org-wide defaults precisely because an audit reads ONE repo and cannot measure the org; else from the repo's own `CLAUDE.md` / `AGENTS.md`, since a convention the repo documents deliberately is that repo's reference and not a deviation; else from the widest scope you can observe (the repo, else the directory). **When a file-local convention contradicts a wider one, the wider one is the reference and file-local consistency is the weaker signal.** Two limits keep this from becoming unbounded. Where nothing wider than the file is observable, there is no style-consistency finding to make — do not manufacture one from a two-file sample. And where a Domain rule sets the convention **per file kind**, that rule is the reference and a raw majority count is NOT: most files floating `^` does not make a deliberately pinned `=` on a concrete contract an inconsistency. Otherwise this is general — it holds for every convention where a single file can be the outlier (import style, error style, test structure, naming), not just returns. A whole-file deviation, with no other defect, is at most **INFO** and never justifies a mass edit; see the Shared rules' **Solidity pragma convention** and **Solidity named returns must be locally provable** for the two cases where the sweep would itself cause harm.
 2. **Leaky abstractions** — internal details exposed through public interfaces, implementation concerns crossing module boundaries, tight coupling between things that should be independent.
 3. **Commented-out code** — each instance should be reinstated or deleted, not left commented.
 4. **Build warnings** — no warnings from the project's toolchain; build warnings are real problems (**LOW or higher, NOT INFO**).