Uh oh!
There was an error while loading. Please reload this page.
Seed suiteNames's named return before the loop - #103
Conversation
`suiteNames` declared `names` and assigned it only inside
`for (uint256 i = 0; i < suites.length; i++)`. Judged from this
function's body alone, `suites.length == 0` reaches the closing brace
with `names` unassigned and returns the empty string. What makes that
path unreachable is `checkedCandidateSuites` reverting
`NoDeployCandidates`, two functions away through `allSuites` — so the
return is total only via a guard this reader cannot see.
Seeding `names = ""` before the loop makes the assignment provable from
this body. The loop and its `i == 0` seed are untouched: with an empty
seed, `string.concat("", ", ", suites[0].suite)` would prefix the list
with a comma, so the ternary is still load-bearing.
No early return on the empty set. `checkedCandidateSuites`' NatSpec and
`testNoCandidateReverts` both require every reader to refuse an empty
declaration rather than answer from one, and an
`if (suites.length == 0) { return names; }` would write that answer into
the source as unreachable code.
Behaviour is unchanged on every reachable input, so the existing
`testSuiteNamesIsTheRegistry` and `testNoCandidateReverts` remain the
coverage.
Closes#72
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>Warning Review limit reached
Next review available in:23 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 (1)
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 |
…atSpec `/// @return networks` sat above `returns (string[] memory)` — unnamed — so `networks` was never a return-parameter name, solc had nothing to check it against, and the word was folded into the description text. Two forms close that gap: name the return, or drop the name from the tag. Drop it. `zoltuAddress` (src/lib/LibRainDeploy.sol:210-211) already documents its unnamed return as `/// @return The address the creation code deploys to.`, and issue 50's own text calls that form correct. This repo does not name return values: an unnamed return keeps solc's "unnamed return variable can remain unassigned" diagnostic, which naming it gives up — see #103, where the remedy for a named return's silent default is a line no test can kill. The signature, the local `string[] memory networks` and the explicit `return networks;` are unchanged. This reverts the code edit made earlier on this branch and fixes the docstring instead; the net change against `main` is one comment line. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
thedavidmeister
commented
Aug 16, 2026
Closing: the ruling is that this repo does not name return values, so the fix is to remove the name rather than to seed the default it silently supplies. This PR is also its own best argument for that. Its mutation run reports that deleting the line it adds SURVIVED the whole suite — the line exists to make an unreachable path assign, so no test can distinguish its presence. The remedy for a named-return hole is unkillable by any test, because the compiler was the only mechanism that could ever have caught the class, and naming the return is what turned it off. Superseded by the sweep issue, which also covers #72 and the generator that emits named returns into |
Closes#72 (audit finding
CQ4-02, dimension 4, severity LOW).The finding
suiteNames()declared the named returnnamesand assigned it only insidefor (uint256 i = 0; i < suites.length; i++). Judged from this function's body alone,suites.length == 0reaches the closing brace withnamesnever assigned and returns the empty string.What actually makes that path unreachable is
checkedCandidateSuites()revertingNoDeployCandidates— two functions away, reached viaallSuites(). So the return is total, but only via a guard this reader cannot see. That is the org rule's case 1: assigned only inside a loop, where a guard elsewhere is what makes every path assign it.It matters rather than being academic because
namesis thevalidSuitesfield ofUnknownDeploymentSuite— the only thing a mistypedDEPLOYMENT_SUITEtells a deployer. If the guard incheckedCandidateSuiteswere ever narrowed, this reports "no valid suites" as an empty string instead of failing.The change
Four lines, one of them code.
The loop and its
i == 0seed are deliberately untouched, and mutant M4 below is the evidence they must be: withnamesseeded empty and the ternary dropped, the list comes back as", address-registry-0-0-1, second-address, ...". The ternary is what keeps the first element unprefixed, so folding it into the seed is a behaviour change, not a simplification.Why not the fix proposed in the issue body
The issue's "After" block also added
if (suites.length == 0) { return names; }. The issue's own collapsed verification block flags and rejects that, and I agree on reading the source:allSuites()cannot return an empty array.checkedCandidateSuites' NatSpec forbids: "a reader that answers from an empty declaration is a reader through which the whole registry can be empty and green."testNoCandidateRevertsassertsexternalSuiteNames()reverts on an empty declaration. An early return is a second, contradictory spelling of that rule sitting in the source.So this PR implements the minimal remedy the verification block prescribes — seed only, no early return.
QA
names = "";is only observable onsuites.length == 0, whichcheckedCandidateSuitesmakes unreachable by revertingNoDeployCandidates. A test that reached the line would first have to disable the guard the repo exists to enforce. The three existing tests that already pin this function are audited below rather than duplicated, per "an existing test that already kills mutants is left as-is":testSuiteNamesIsTheRegistry,testEmptySuiteIsUnknown,testUnknownSuiteNamesEveryValidSuite(all intest/src/abstract/RainDeploySuitesBase.t.sol), plustestNoCandidateRevertspinning that the empty path reverts rather than answers.testSuiteNamesIsTheRegistry,testNoCandidateRevertsandtestEmptySuiteIsUnknowneach named[PASS]in the output. Each mutant was greped back out of the file aftersedto confirm it actually applied, so a no-op edit cannot fake a survival. All againstforge test --match-path test/src/abstract/RainDeploySuitesBase.t.sol:names = "";(the line this PR adds) → SURVIVED (10 passed, 0 failed). Expected and reported as such rather than papered over: the line's whole purpose is to make an unreachable path assign, so no test can distinguish its presence. This is the known outcome for this finding class, not a coverage gap.i == 0→i == 1→ KILLED, 3 failed.testSuiteNamesIsTheRegistry(second-address, address-registry-candidate, second-address-candidate != address-registry-0-0-1, second-address, ...),testEmptySuiteIsUnknown,testUnknownSuiteNamesEveryValidSuite. Run with the new seed present, which is what proves the seed did not weaken the ternary's coverage.", "→","→ KILLED, 3 failed, same three tests.string.concat(names, ", ", suites[i].suite)→ KILLED, 3 failed, same three tests, output", address-registry-0-0-1, ..."with the leading comma. This is the mutant that proves thei == 0branch cannot be collapsed into the seed.suiteNames' implementation — it is the registry order declared in the test fixture's ownreleasedSuites()/candidateSuites()(address-registry-0-0-1, second-address, address-registry-candidate, second-address-candidate), written out longhand as a literal in the test rather than recomputed by concatenating in the assertion. For the empty case the oracle ischeckedCandidateSuites' NatSpec, which states that every reader must refuse an empty declaration — that is what rules out the early return the issue body proposed.checkedCandidateSuites' NatSpec andtestNoCandidateReverts, and prescribes the seed-only remedy instead. No adjacent scope taken:allSuites,checkedCandidateSuitesandsuiteByNameare untouched.Full verification run
nix develop -c forge build— clean, exit 0.nix develop -c forge testwith all five RPC endpoints set: 215 passed, 0 failed, 0 skipped across 17 suites.nix develop -c forge fmt --check— clean, exit 0.(Without RPCs, 47 fork tests fail on
vm.createSelectFork: environment variable ..._RPC_URL not found. That is the requirement documented in CLAUDE.md, not this change — they pass once the endpoints are set, which is the 215/215 above.)🤖 Generated with Claude Code