test: assert the empty-derivation early return forks nothing - #106
Conversation
Closes #63. `checkDeployedOnSupportedNetworks` returns before touching any RPC endpoint when there is nothing to check, and nothing observed it. The branch is taken on every CI run by `RegistryDeployChainTest`, which has released nothing, and that contract passed identically with the early return deleted: it would fork all five supported networks, find nothing to check on each, and pass. The absence of the fork is asserted directly. `vm.activeFork()` reverts when no fork is selected, so a failing low-level call to it IS "no network was reached". It runs the whole inherited entry point rather than an empty array handed straight to the matrix, so a fork opened while deriving is inside what is asserted, and it asserts the empty released set as a premise so the first release fails here naming what changed rather than reporting a matrix with nothing to check forked. The complement goes in `RainDeployVerifyChainTest`, which already forks every supported network, rather than beside the empty case: asserting it there would hand the one contract that exists to need no RPC endpoint the five-endpoint dependency the early return removes from it. It runs at ONE subject because that is the length that discriminates -- every other test here runs the matrix over two released suites, and the only other contract reaching it with a subject does so at one while forking on its own beforehand, so a guard keyed anywhere below two returns early on every subject in the repo and no existing test can see it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 22 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 (2)
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 |
|
Reviewed 0b4b6f0: ready — Closes #63. The count is the whole finding and the test is placed to match it. Every other test in that contract runs the matrix over TWO released suites, so a guard keyed anywhere below two returns early on every subject in the repo and no existing test notices: the only other contract reaching the matrix with a subject does so at exactly one, and it forks on its own beforehand, which makes a matrix that returned without forking indistinguishable there. One subject is therefore the length that discriminates, and asserting Putting it in the contract that already forks every supported network is also right — asserting it from the empty-set side would hand the contract that exists precisely to need no RPC a five-endpoint dependency. CI green, 0 unresolved threads — vacuous, CodeRabbit reports |
2 participants
Closes #63 (audit
cov-10, LOW).The gap
checkDeployedOnSupportedNetworksreturns before touching any RPC endpoint when there is nothing to check, and nothing observed it.RegistryDeployChainTesthas released nothing, so it takes that branch on every CI run — and passed identically with the early return deleted, simply forking all five supported networks, finding nothing to check on each, and passing. The property under test was the absence of the forks, which no test looked at.What is asserted
test/src/abstract/RegistryDeployChain.t.sol—testChainWithNothingToCheckForksNothing.vm.activeFork()reverts when no fork is selected, so a failing low-level call to it IS "no network was reached". Two departures from the issue's sketch, both to make the assertion mean what it says:test/src/abstract/RainDeployVerifyChain.t.sol—testChainWithASingleSubjectDoesFork. The complement, placed per the issue's own verification-block caveat: it goes in the contract that already forks every supported network, never beside the empty case, because asserting it there would hand the one contract that exists to need no RPC endpoint the five-endpoint dependency the early return removes from it.It runs at one subject, and the count is the point. Every other test in that contract runs the matrix over its two released suites, so a guard keyed anywhere below two returns early on every subject in the repo and no existing test sees it —
RainDeployVerifyChainCandidateTestis the only other contract that reaches the matrix with a subject, at exactly one, and it forks on its own before calling, so a matrix that returned without forking is indistinguishable there.No
src/change — this PR is tests only.QA
testChainWithNothingToCheckForksNothing(test/src/abstract/RegistryDeployChain.t.sol),testChainWithASingleSubjectDoesFork(test/src/abstract/RainDeployVerifyChain.t.sol) — both pass on the unmutated baseline atce1e371, and each fails under the mutation of the exact line it is about, verified by applying the mutant tosrc/abstract/RainDeployVerifyChain.solin the working tree and re-runningforge test(the baseline was committed first, so the revert isgit checkout -- src/...). Neither existed on base, so "fails on base" is verified as the mutation table below: on base the guard is unasserted, and the first mutant reproduces base's own untested state.src/abstract/RainDeployVerifyChain.sol:115-117→ delete theif (derived.length == 0) { return; }guard entirely → killed bytestChainWithNothingToCheckForksNothing(FAIL: the matrix forked a network with nothing to check).src/abstract/RainDeployVerifyChain.sol:115→derived.length == 0→derived.length < 2→ killed bytestChainWithASingleSubjectDoesFork(FAIL: the matrix checked a subject without forking). The other 8 chain tests all PASSED under this mutant — includingtestChainMatrixReachesTheLastSupportedNetwork,testChainMatrixCoversEverySupportedNetwork,testChainIgnoresAnUndeployedCandidateand the new empty-set test — which is the evidence that one subject is the only length in the repo that discriminates, and why the complement is not a duplicate of the existing "the matrix does run" tests.checkDeployedOnSupportedNetworksitself, which states the guard's purpose ("Nothing to check is not a reason to touch five RPC endpoints"), plusRegistryDeployChainTest's contract natspec andCLAUDE.md, which both state "it forks nothing and passes" as a repo property. The expected values come from that stated intent, not from the implementation: the observable isvm.activeFork()'s documented revert-when-nothing-selected behaviour, independent of anythingRainDeployVerifyChaincomputes, and the fork counts are never read back from the code under test.RegistryDeployChain.t.sol, since that would re-impose the five-endpoint dependency on the one contract the guard frees from it; (B) is therefore inRainDeployVerifyChainTest, which already forks, as that ruling directs.🤖 Generated with Claude Code