audit: named returns must be locally provable; org default outranks file-local style - #65
Conversation
…ile-local style (0.21.0) Adds a Solidity named-return rule to the Shared rules' Domain rules, beside the other Solidity naming conventions those rules leave a gap in: they reach declarations, locals and params but not return declarations. The rule flags only the subset whose assignment cannot be proved from the declaration's own function — loop/branch-only assignment, never-assigned on a reachable path, shadowed on every path by an explicit return, and last-iteration-wins — plus mixed forms inside a single diff at INFO. It is explicitly not a ban and not a conversion mandate, matching the restraint the pragma convention and the stale-soldeer-deps rule already carry. Cross-references Fail-closed on input (the zero value is the permissive default) and Explicit handling of ambiguous sets (last-iteration-wins reached through a return declaration rather than a .find), which is why this is a correctness rule and not a style preference. Separately fixes dimension 4 #1: style consistency measured against the surrounding file inverts the ruling when the file is itself the outlier. The convention is now established from the org-wide default, with file-local consistency as the weaker signal. Kept general — it holds for every convention where one file can be the outlier. Realigns the SKILL.md frontmatter version with plugin.json/marketplace.json, which had drifted to 0.20.0 while the skill still declared 0.19.0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 33 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The pre-loop derived-address check exists only on rain.deploy#21's branch, not on main. State instead what holds on both: every check is either before the loop or inside it, and there is none after it, so the zero escapes either way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
QA evidenceHead SHA at time of writing: Checks runThis repo ships no test suite and no linter config. There is no Manifest and frontmatter well-formedness: And on CI, at head Note the pre-existing drift this PR closes: Verification of the
|
repo (src/, generated excluded) |
named | unnamed | % unnamed | issue's table | agrees |
|---|---|---|---|---|---|
rain.erc4626.words @ e02d60e |
4 | 44 | 91.7% | 4 / 44 | exact |
rain.math.float @ d3fb611 |
19 | 125 | 86.8% | 19 / 125 | exact |
rain.interpreter @ a6b7ad7 |
59 | 445 | 88.3% | 58 / 450 | differs |
rain.deploy @ origin/main |
5 | 1 | 16.7% | 5 / 1 | exact |
| total | 87 | 615 | 87.6% |
Three of four reproduce exactly, which is what establishes that the table's unit is the return component, not the clause (returns (bool, bytes32) counts 2). By clause the same trees give rain.erc4626.words 3/32, rain.math.float 10/100, rain.interpreter 50/318, rain.deploy 5/1 — which matches nothing in the table, so the component reading is confirmed rather than assumed.
Discrepancy: rain.interpreter is 59/445 by my count, not 58/450. A parser-nuance delta (one named component more, five unnamed fewer, out of 504). It does not move the conclusion — ~11.7% named either way — and the shipped figure is the independently measured one. Excluding/including the six src/generated/*.pointers.sol files changes nothing: they declare no functions with returns.
rain.deploy's 5 / 1 is the main count. At fb137c6 it is 6 / 1, because zoltuAddress is one of the functions #21 adds. The table row and the worked example's line numbers therefore come from different revisions of that repo — cosmetic, but worth recording since both figures appear in the issue.
Scope note
git diff --stat origin/main HEAD is skills/audit/SKILL.md plus the two version manifests — no other file touched, and nothing was written outside this branch's own clone.
The named-return rule turned on a term it never defined — "cannot be proved from the declaration's own function" — leaving a reader to guess at try/catch, at a branch whose complement reverts, and at a total if/else nested in a loop. Two sessions could reach opposite findings on the rule's own worked example, whose shape (an if/else inside a for loop) is indistinguishable from the "total if/else" non-finding as it was written. Provability is now a stated test over paths reaching the function's end, and nesting is judged by that test rather than by the syntax. Cases 2 and 4 carried flat severities while every neighbouring rule scales by what the value gates, so a never-assigned `returns (bool blocked)` capped at MEDIUM where Fail-closed on input rates the same fail-open default CRITICAL/HIGH. Both now escalate on the scale of the rule they cross- reference, and file once rather than twice. The non-findings mixed two kinds without saying so: some justify the named form (assembly, tuple documentation), some establish provability itself. Only the second kind discharges cases 1/2/4, and case 3 does not reach a name that is the documentation. The mixed-forms example cited rain.deploy#21 adding `zoltuAddress` with a named return. That PR's head is now 528a7c7, "style(deploy): drop the named return from zoltuAddress" — the citation is falsified by going and looking, so the case states the shape instead. Dimension 4 #1 mandated an org-wide measurement an audit cannot make: the skill's file discovery globs one repo. It now prefers a Domain rule (which is where the org-wide defaults actually live), then the repo's own documented conventions, then the widest observable scope — and makes no finding at all where nothing wider than the file is observable. A raw majority count no longer overrides a per-file-kind rule, which would have inverted the pragma convention exactly as #1 inverted named returns. The LibRainDeploy count is pinned to `main` and the load-bearing claim made rot-tolerant. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The rule's central premise was that a named return supplies a zero "on a path the unnamed form would refuse to compile", and case 2 called the unnamed form "a compile error". Both are false. Checked against solc 0.8.25: the unnamed form compiles and emits a WARNING — Unnamed return variable can remain unassigned. Add an explicit return with value to all non-reverting code paths or name the variable. — exit 0. Every unnamed shape with a non-reverting path to the function end warns (empty body, no return statement, return only inside a loop, return in if but not else); a named return draws nothing on any of them; the sole unnamed exception is a completely empty body. That makes the true motivation stronger than the false one. The compiler already detects this class, and it offers "or name the variable" as one of two remedies — so naming the return suppresses the diagnostic on the compiler's own suggestion. Since dimension 4 #4 rates a build warning LOW or higher, the rule is the audit standing in for a signal the name removed, rather than a claim about what compiles. The same probe falsified a sentence added in the previous commit: a partially named tuple does NOT force an explicit return. `returns (bool ok, bytes32)` with no return statement compiles, warning on the unnamed component only. The case-3 carve-out now rests on the name being the component's documentation, which is the real reason, and notes which component solc actually warns about. Prevalence re-measured independently rather than carried over: 86 named / 561 unnamed = 86.7% unnamed across the four repos' src/ function declarations, generated files excluded. "~88%" is reachable only by also counting the `returns` of function TYPES (87.8%), which are types and not declarations, so the figure is now ~87% with its unit stated and the other convention named. The 5-named-to-1-unnamed count for LibRainDeploy.sol on main is confirmed exact (it is 5/2 on rain.deploy#21's head, which is why the pin matters). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The warning behaviour is identical on 0.8.25 and 0.8.35 — same two diagnostics, exit 0 both — so the claim is pinned to the range rather than one version. The worked example now says why dropping the guard is silent: the name is the thing that suppresses the warning. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Independent adversarial review —
|
main |
#21 head 528a7c7 |
|
|---|---|---|
NoNetworks guard, top of function |
L218 | L238 |
| pre-loop derived-address check (inputs only) | — | L247–250 |
assignments (both if/else arms, inside loop) |
L248, L260 | L278, L290 |
codehash check on deployedAddress |
L264 — inside loop | L294 — inside loop |
| any check after the loop | none | none |
The two revisions do differ exactly as expected — the pre-loop derived-address check exists only on the branch — but since both of its pre-loop checks test inputs, the shipped wording is true either way. The 9d001d6 commit that made this revision-independent was the right call.
5 named / 1 unnamed for LibRainDeploy.sol is exact — on main. Confirmed by independent parse: 5/1 at 4422e29, 5/2 at 528a7c7. The text did not say which, so I pinned it to main and made the load-bearing claim ("predominantly named returns") survive #21 landing. Incidentally the lone unnamed one on main is supportedNetworks(), whose docstring already says @return networks — the name exists in the doc but not the code.
Case 1 flagging currently-correct code is consistent with "findings are PROBLEMS". I checked this against the rule two levels above it. It survives on the same footing as the ERC-165 rule's "a finding even when it happens to be correct today" and Hazard cat. 3: the finding is the non-local proof, not the behaviour. Not a defect.
No overlap with the storage-class naming rule beside it. It opens by delineating precisely — declarations/locals/params vs return declarations. Clean.
Defects found and fixed
1. The rule's central premise was false. (the big one) The text claimed a named return supplies a zero "on a path the unnamed form would refuse to compile", and case 2 called the unnamed form "a compile error". Neither is true. Compiled probes on solc 0.8.25 and 0.8.35, identical results, exit 0 both:
Warning: Unnamed return variable can remain unassigned. Add an explicit return
with value to all non-reverting code paths or name the variable.
It is a warning, never an error. Every unnamed shape with a non-reverting path to the function end warns (empty body, no return at all, return only inside a loop, return in if but not else); a named return draws nothing on any of them; the sole unnamed exception is a completely empty body.
This makes the true motivation stronger than the false one, so the rule now rests on it: the compiler already detects this class, and it offers "or name the variable" as one of its two suggested remedies — so naming the return suppresses the diagnostic on the compiler's own advice. Since dimension 4 #4 rates a build warning LOW or higher, the rule is now framed as the audit standing in for a signal the name removed, rather than as a claim about what compiles.
2. The mixed-forms worked example is falsified by going and looking. The text cited rain.deploy#21 as adding zoltuAddress(...) returns (address derivedAddress). That PR's head commit is literally titled style(deploy): drop the named return from zoltuAddress — at 528a7c7 it is returns (address) with an explicit return, i.e. the same form as the three sibling helpers. The one cited instance of the INFO case no longer exists. (The PR body verified it at fb137c6, one commit earlier — accurate when written, rotted since.) A reader who checks the only example and finds the opposite will distrust the whole rule, so the case now states the shape and cites no live PR.
3. cannot be proved from the declaration's own function was never defined — the rule's central term. Applying dimension 0 to the added text as the process document it is, this is the "missing defaults or undefined terms" defect, and it left every boundary case open. Worse, the rule's own worked example (an if/else inside a for) is syntactically indistinguishable from its own non-finding ("on every branch of a total if/else") — two sessions could reach opposite findings on deployToNetworks itself. Now defined: on every path reaching the function's end, the name is either assigned or the path reverts, judged from that body alone — and nesting is judged by that test, not by syntax, so a total if/else or a both-arms try/catch inside a loop is explicitly not total.
That one definition resolves the boundary cases I constructed: try/catch (catch is a path), a branch whose complement reverts (the path reverts ⇒ provable), a total if/else with no trailing code (provable), and loop nesting.
4. Cases 1 and 2 had no stated discriminator. The same try-assigns / catch-doesn't code could be read as case 1 (LOW) or case 2 (MEDIUM). Now explicit: case 1 = a guard does make it total (hazard is the distance to the proof); case 2 = no guard does. Each cross-references the other.
5. Severities were flat where every neighbouring rule scales by what the value gates. Case 2 cross-references Fail-closed on input, which rates a fail-open default "CRITICAL/HIGH if it bypasses auth, lifts a spend/write bound, or destroys data" — but capped itself at MEDIUM; case 4 cross-references Explicit handling of ambiguous sets ("HIGH if it drives a verdict / permission / value / destructive action") and capped at MEDIUM. Both now escalate on the scale of the rule they cite. I also added the sense check, because a zero is only permissive in one direction: returns (bool valid) falling to false fails closed, but returns (bool blocked) / (bool paused) falling to false reads as "proceed", and returns (address recipient) falling to address(0) sends funds nowhere.
Relatedly, the cross-references left ownership unstated — file it once, here, taking the highest applicable severity.
6. The non-findings list silently mixed two kinds. Assembly and tuple documentation justify the named form; unconditional on entry and total if/else establish provability. Only the second kind discharges cases 1/2/4. This is the collision you asked about: a named return written from assembly and only inside a loop hit both clauses with no stated precedence. Resolved — the naming justifications do not discharge provability, and the fix there is the local proof, never unnaming it (which the rule already forbids). Conversely case 3 must not reach a name that is the documentation, so it now excludes the partially-named tuple.
7. My own first fix was wrong, and the compiler caught it. I initially wrote that a partially-named tuple "forces an explicit return on every path". It does not — returns (bool ok, bytes32) with no return statement compiles, warning on the unnamed component only. Rewritten to rest on the real reason (the name is the component's documentation) and to note that it is the unnamed sibling solc warns about.
8. Dimension 4 #1 over-reached into something unactionable. "Establish the convention from the ORG-WIDE default" cannot be done by this skill: its own file discovery globs one repo, so a reviewer has nothing org-wide to measure. It also contradicted its own fallback ladder's last rung ("else the directory"), and said nothing about what to do when no wider scope is observable. Reordered to what is actually actionable: a Domain rule that states the convention (which is where the org-wide defaults actually live, carried in the doc precisely because one repo can't measure the org), else the repo's own CLAUDE.md/AGENTS.md, else the widest observable scope.
Two limits added so "general" did not become "unbounded":
- Where nothing wider than the file is observable there is no style-consistency finding — do not manufacture one from a two-file sample.
- Where a Domain rule sets the convention per file kind, a raw majority count is not the reference. As written, feat: marketplace + consolidate to one ultracode-native audit skill #1 would have inverted the pragma convention in exactly the way it was rewritten to stop inverting named returns: most files float
^, so a deliberately pinned=on a concrete contract would read as the deviation. It cross-referenced the pragma rule only as a don't-mass-edit caveat, not as "the majority is the wrong reference here".
And on your question about licensing a flag on a deliberate per-repo convention — yes, it did. CLAUDE.md/AGENTS.md is now the repo's reference, matching how Shared rules already opens ("First read CLAUDE.md / AGENTS.md for project structure and conventions") and how the stale-soldeer rule respects an explicit freeze marker.
9. The prevalence figure was not reproducible as stated. Measured independently with a comment/string-stripping parser over all four repos' src/ trees (8 *.pointers.sol files excluded), attribution-audited so every returns token maps to exactly one owner, zero unclassifiable:
| repo | named | unnamed | % unnamed |
|---|---|---|---|
| rain.erc4626.words | 4 | 30 | 88.2% |
| rain.math.float | 19 | 125 | 86.8% |
| rain.interpreter | 58 | 405 | 87.5% |
| rain.deploy | 5 | 1 | 16.7% |
| total | 86 | 561 | 86.7% |
86.7%, not ~88%. The gap is entirely one counting decision: returns clauses of function types (function(ParseState memory, uint256, uint256) view returns (uint256, bytes32) used as a param/array-element type — 57 extra components, all unnamed). Include them and it is 87.8%, which rounds to 88. That also reconciles all three prior counts: this PR's rain.erc4626.words 4/44 is exactly my 4/30 plus 14 function-type components. So no measurement was careless — the unit was just never stated, and a reader re-measuring declarations gets a different number. Now ~87% with its unit spelled out and the other convention named. The conclusion is untouched at any of these figures.
Not a defect, recorded so it isn't re-raised
- Length/compression risk. The rule is long, and dimension 0 flags instructions "fragile under context compression". I judged it acceptable rather than restructuring: the restraint ("NOT a ban and NOT a mandate") is bolded and front-loaded. I did split the non-findings and precedence into their own block so the stance and the mechanics are separately legible, which is how the Temporal and Deploy-pin rules are already shaped.
- Overlap with Fail-closed / ambiguous sets is real but intended, and the file has precedent for a rule reachable from several routes (Derived constants spans Domain rules, dimension 2 and Hazard cat. 3). Fixed by stating ownership, not by removing the cross-references.
- The
~88%aggregate says nothing aboutrain.deploy(6 of 647 components, 0.9%) — but the rule never uses it that way, and dimension 4 feat: marketplace + consolidate to one ultracode-native audit skill #1 handles the outlier case directly. Not a defect.
Out of scope — needs its own issue
The version-hygiene workflow cannot see the drift this PR just fixed. Confirmed by reading .github/workflows/version-hygiene.yaml: job 1 compares plugin.json ↔ marketplace.json; job 2 compares plugin.json base ↔ head when skills/ changed. Neither job ever reads skills/audit/SKILL.md — grep -rn "SKILL.md" .github/workflows/ returns nothing. So the skill's own frontmatter version: is ungated and can drift straight back. It already had: on main, SKILL.md declared 0.19.0 while plugin.json/marketplace.json said 0.20.0 — two minors adrift, invisible to CI, which is exactly why this PR had to realign three files instead of two. The fix is a third assertion (frontmatter version == plugin.json version), deliberately not made here.
Verification
This repo has no CLAUDE.md, no flake, no pre-commit and no test over SKILL.md structure, so version-hygiene is the only automated gate and I replicated both jobs by hand with real jq (1.8.2) against origin/main before each push:
JOB1 PASS: versions agree: 0.21.0
JOB2 PASS: bumped 0.20.0 -> 0.21.0
SKILL.md frontmatter: version: 0.21.0
All three declarations agree at 0.21.0. No further bump: 0.21.0 is unreleased, so this content belongs to it.
Solidity claims were checked by compiling probe contracts, not by reading — which is how findings 1 and 7 surfaced.
🤖 Generated with Claude Code
…he scoped dimension-4 header, keep 0.22.0
2 participants
Closes #64
Two independent edits to
skills/audit/SKILL.md.1. New Shared rule: Solidity named returns must be locally provable
Added to the Domain rules beside the other Solidity naming conventions, because that is precisely the gap they leave: the storage-class rule and the short-names rule reach declarations, locals and params, but neither reaches a return declaration. A named return is where Solidity silently supplies a zero/empty default on a path the unnamed form would refuse to compile.
What it flags
The finding is only the subset whose assignment cannot be proved from the declaration's own function:
return— dead declaration, pure reader taxThe first four stand regardless of when the code was written. The mixed-forms case is new-code only, where the fix is free.
What it explicitly does NOT do
It is not a ban and not a conversion mandate. A named return sometimes genuinely simplifies the source — a value accumulated across branches, a slot written from
assembly, a multi-value tuple whose component names are the documentation. And converting an existing named return to unnamed means adding an explicitreturnto 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, or hoist the guard's invariant into the declaration), a strictly smaller and safer change than the refactor.
Five non-findings are stated explicitly so a reviewer does not reach for them: a tuple component documented by its name; a named return referenced from an
assemblyblock; one assigned unconditionally on entry or on every branch of a totalif/else; and a file whose local convention is named returns, absent one of the four cases — INFO at most, never a sweep.Why the restraint is there, in the org's own words
The skill already carries this exact shape twice, and the new rule reuses their voice rather than inventing a third:
Why this is correctness, not style
Two Shared rules already reach the same conclusions by a different route, and the new rule cross-references both:
.find.2. Dimension 4 #1 no longer inverts the ruling on an outlier file
#1measured "style consistency" against the surrounding file. Where the file is the outlier that 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. Applied torain.deploy'sLibRainDeploy.sol(5 named return components to 1 unnamed) a reviewer blesses the named returns and flags the unnamed sibling — exactly backwards.#1now requires the convention to be established from the widest first-party scope observable (org, else repo, else directory) before judging any one file, and states that when a file-local convention contradicts a wider default, the wider default is the reference and file-local consistency is the weaker signal.The fix is deliberately 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 capped at INFO, with pointers to the pragma and named-return rules as the two cases where the sweep would itself cause harm.
Prevalence, counted independently
Counted with a comment/string-stripping parser over each repo's default branch
src/, generated files excluded, per return component (which is the unit the issue's table uses — areturns (bool, bytes32)counts 2):src/, generated excluded)Three of the four reproduce the issue's table exactly.
rain.interpreterdiffers slightly — mine is 59/445, the issue says 58/450 — a parser-nuance delta that does not move the conclusion (~12% named either way). Per the reporting instruction, my counts stand.Discrepancies found while verifying (details in the QA comment)
fb137c6, is the head of rain.deploy#21, notmain. The issue's line numbers are that branch's.5 named / 1 unnamedfigure for rain.deploy is themaincount; atfb137c6it is 6/1, becausezoltuAddressis one of the functions audit: add events-belong-on-the-interface check (Code quality #12) #21 adds.Both are cosmetic to the rule, and the rule text deliberately cites the function and shape rather than line numbers, which would rot.
Versions
plugin.jsonandmarketplace.json0.20.0 → 0.21.0 (lockstep, asversion-hygienerequires). TheSKILL.mdfrontmatter had drifted — it still declared 0.19.0 whileplugin.jsonsaid 0.20.0 (the 0.20.0 bump came in via a merge-conflict resolution) — so it is realigned to 0.21.0 with the other two.QA
SKILL.md(git ls-filesis 6 files: the skill,README.md,install.sh, the two.claude-pluginmanifests and one workflow); there are no structure/ordering assertions to extend and no prose linter configured. The only automated gate is.github/workflows/version-hygiene.yaml, and both of its jobs were replicated locally againstorigin/mainbefore pushing — job 1 (plugin.json 0.21.0 == marketplace.json 0.21.0) PASS, job 2 (skills/ changed ⇒ plugin.json bumped, 0.20.0 → 0.21.0) PASS. Evidence transcribed in the QA comment.rain.deployLibRainDeploy.sol), where a wrong claim would have invalidated the rule. All four load-bearing claims were checked line by line and held; see the QA comment.rainlanguage/rain.deploysrc/lib/LibRainDeploy.solat bothorigin/mainandfb137c6for the worked example, and thesrc/trees ofrain.erc4626.words,rain.math.float,rain.interpreterandrain.deployfor the prevalence table. The prevalence counts come from a parser written for this task, not from the issue's table; where the two disagree (rain.interpreter) the independently measured figure is the one shipped.deployToNetworks-shaped loop-only assignment as a finding with a local fix, forbids the mass edit, and feat: marketplace + consolidate to one ultracode-native audit skill #1 can no longer produce the inverted ruling.🤖 Generated with Claude Code