AMT coverage: g4-icloneablefactoryv3-newclone - #76
Conversation
…pe gaps Probe pass 1 against the pre-existing suite found the whole declaration-only half of this repo unguarded: the three deprecated interfaces, ICloneableFactoryV2 and ICloneableV2.InitializeSignatureFn are imported by nothing, and even ICloneableFactoryV3.NewClone survives having two of its three address parameters transposed, because that is invisible to every topic and every log data byte while telling every indexer the deployer is the clone. - test/src/lib/LibPublishedAbi.sol reads the compiled artifact, the only oracle for parameter names, same-typed parameter order, indexed flags and return types. - Declaration pins for ICloneableFactoryV3, ICloneableV2, ICloneableFactoryV2 and the three deprecated interfaces. - TestCloneableConformant, TestCloneableEmitter, TestCloneableReverter and TestCloneableV1Shaped: fixtures whose shape makes the once-only MUST, the typed-overload MUST, NewClone log ordering and initialize revert bubbling observable at all. - TestCloneFactory.t.sol pins the pure-delegation claim the concrete exists for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…undeployable code testCheckImplementationCodeEtched fails on UNMUTATED source whenever the fuzzer draws code whose first byte is 0xEF: vm.etch reads that as an EIP-7702 delegation designator and rejects it. EIP-3541 forbids deploying such code at all, so it can never be an implementation's code on chain, and excluding it narrows the fuzz domain to inputs the property is about rather than weakening it - the guard only ever reads code LENGTH. Reported independently as #64 and #68; fixed here because a baseline that goes red on a fuzz draw makes every mutation verdict in this campaign unreliable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 20 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (20)
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 |
`test/src/**` mirrors `src/**` and holds the `.t.sol` suites. This repo is the library half of the split — there is no `src/concrete/`, the concrete lives in rain.factory.deploy — so `test/src/concrete/` mirrored nothing. Test SUPPORT code (harnesses, mocks, fixtures) belongs outside the mirror, in `test/concrete/`, `test/lib/`, `test/abstract/`, as in rain.deploy and rain.math.float. Pure move plus the import paths that follow it. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ntable code Three parallel AMT branches (#76, #77, #78) each grew their own variant of `TestCloneable` because it satisfies neither of `ICloneableV2`'s normative MUSTs and moves in lockstep with the constant the library compares against. Fix it once, here, so the branches converge on one fixture instead of four: - `initialize` can NOT be called more than once. That is the interface's first MUST and no fixture honoured it. - The RECOMMENDED typed overload is present and reverts `InitializeSignatureFn` always, as the interface requires. - The success sentinel is written out from the LITERAL string the interface names, not imported from `ICLONEABLE_V2_SUCCESS`. Importing it put both sides of the library's comparison in lockstep: the constant could drift and every flow test would still pass, because the fixture drifted with it. A third party hard-codes `keccak256("ICloneableV2.initialize")`, so the fixture does too, and every existing flow test now discriminates a drift. Separately, `testCheckImplementationCodeEtched` could fail for a harness reason: `vm.etch` parses a `0xef01` prefix as an EIP-7702 delegation designator and rejects anything that is not exactly the 23-byte designator. EIP-3541 forbids deploying any `0xef`-leading code at all, so such code cannot exist at an implementation address on any chain and the guard is not specified over it; the fuzz domain is narrowed to code that could actually exist. The guard only ever reads code LENGTH, so nothing about the property changes. Gas snapshot regenerated for the extra `SSTORE` the initialization guard costs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three parallel AMT branches each invented their own `ICloneableV2` fixture on top of the two that already existed. The merge from `2026-08-24-test-fixtures-out-of-src-mirror` resolves this branch onto the shared set: - `TestCloneableCallRecorder` and `TestCloneableRevert` move to `test/concrete/`, out of the `test/src` mirror, and are the canonical versions — #76 carries the same two files byte for byte, having dropped its own `TestCloneableEmitter` and `TestCloneableReverter` duplicates, so the two branches merge without touching each other. - Both now return the success sentinel as a LITERAL rather than importing `ICLONEABLE_V2_SUCCESS`, matching `TestCloneable` after the base branch: a fixture that imports the constant the library compares against moves in lockstep with it and cannot discriminate a drift. - The `0xef` fuzz-domain narrowing on `testCheckImplementationCodeEtched` is dropped from this branch. All four AMT branches had patched that same line; it is a `main` defect and now lives there once. Gas snapshot regenerated: it moves for the extra `SSTORE` the base branch's initialization guard costs on every `TestCloneable` clone, as well as for this branch's new tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
# Conflicts: # test/concrete/TestCloneFactory.t.sol # test/concrete/TestCloneableConformant.sol # test/concrete/TestCloneableEmitter.sol # test/concrete/TestCloneableReverter.sol # test/concrete/TestCloneableV1Shaped.sol # test/src/lib/LibICloneableFactoryV4.checkImplementationCode.t.sol
…what they test The merge from `2026-08-24-test-fixtures-out-of-src-mirror` already relocated this branch's fixtures out of the `test/src` mirror, which only mirrors `src`. This finishes the job. Fixtures — three of this branch's five are gone, none of the coverage with them: - `TestCloneableConformant` was `TestCloneable` plus a one-shot init guard and the typed overload. The base branch gives `TestCloneable` both, for the whole suite rather than for two tests, so the second fixture had nothing left to add. `testInitializeOnlyOnce` and `testTypedOverloadRevertsInitializeSignatureFn` move onto it unchanged apart from the error name. - `TestCloneableReverter` and `TestCloneableEmitter` are replaced by `TestCloneableRevert` and `TestCloneableCallRecorder`, byte for byte the same files #78 carries, so the two branches merge without touching each other. - `TestCloneableRevert` and `TestCloneableFailure` stay SEPARATE. A fixture that returns a non-success sentinel cannot also revert with a typed error carrying data; collapsing them would weaken `testInitializeRevertBubblesVerbatim` to a bare decode revert. `testNothingCalledBeforeInitialize` gains a second, stronger oracle in the swap. `TestCloneableEmitter` announced a stray call by emitting from its fallback, so the whole assertion rode on a log count. `TestCloneableCallRecorder` records the SELECTOR SEQUENCE instead, and the test now asserts that exactly one call reached the clone and that it was `initialize(bytes)`. Mutation-checked: a `child.call(...)` inserted into `cloneAndInitialize` between `NewClone` and `initialize` is caught by the selector assertion ("2 != 1") and is INVISIBLE to the log-count assertion, because the recorder's fallback deliberately does not emit. `TestCloneableV1Shaped` was imported by nothing on this branch — a fixture built for a test that was never written. It now has it. `testInitializeSelectorSharedWithV1` pins that V1 and V2 publish the SAME `initialize(bytes)` selector, which is only half a hazard statement: the question it raises is whether a V4 factory silently accepts a legacy implementation and leaves a live clone that was never initialized. It does not — the `bytes32` return decode finds an empty returndata buffer and reverts before the sentinel comparison, with NO revert data, which the test asserts as observed behaviour rather than assuming `InitializationFailed`. Mutation-checked: give the fixture a `bytes32` return and the test fails. Layout, so a file name says what it tests: - `LibPublishedAbi.sol` is test support, not a mirror of any `src/lib` file, so it moves to `test/lib/`. - `ICloneableFactoryV2.sol.t.sol` and `ICloneableFactoryV3.sol.t.sol` lose the doubled extension. - `ICloneableV2.sol.t.sol` becomes `ICloneableV2.initialize.t.sol`, matching the `<unit>.<function>.t.sol` convention `test/src/lib` already uses. Every test in it is about `initialize` or its return sentinel. - `DeprecatedInterfaces.t.sol` splits into `ICloneableV1.t.sol`, `ICloneableFactoryV1.t.sol` and `IFactory.t.sol`, one per file in `src/interface/deprecated/`, which is what the mirror is for. The three tests were independent and shared nothing but the notice, which each file keeps. The `0xef` fuzz-domain narrowing this branch carried is resolved onto the base branch's version of the same line, which now holds it once for all four AMT branches instead of four times. Gas snapshot regenerated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Per CodeRabbit on this PR. The finding is correct and the comment on
`vm.assume(code[0] != 0xef)` was overclaiming.
The comment said no implementation on any chain can have `0xef`-leading code.
EIP-3541 does forbid DEPLOYING it, so no CREATE or CREATE2 can produce it — but
EIP-7702 leaves exactly one way an account can hold it anyway: a delegation
designator, `0xef0100 || address`, exactly 23 bytes. `EXTCODESIZE` on a
delegated EOA returns 23, not zero. The assume was silently excluding a case
that is real, behind a comment saying it was not.
`testCheckImplementationCodeEip7702Designator` pins it as a fixed case, since
the fuzz test cannot reach it. It PASSES the guard, which is the part worth
having on the record: a code-SIZE check cannot tell an implementation contract
from an EOA that has delegated, and a delegation is REVOCABLE by the account
holder where deployed code is not. Callers wanting an immutable implementation
do not get that from this guard.
Mutation-checked, not assumed: with the guard mutated to
`code.length == 0 || code[0] == 0xef`, this is the ONLY test in the suite that
fails (42 pass, 1 fail). The fuzz test cannot kill that mutant by construction,
because its assume excludes the input that would.
Scoped honestly in the NatSpec: `foundry.toml` pins `evm_version = "cancun"`,
which predates EIP-7702, so the test asserts that the 23-byte designator is
storable and passes the SIZE check. It does not exercise, and does not claim,
the execution semantics of delegation.
The rest of the assume stands: `0xef`-leading blobs of any OTHER length cannot
exist on any chain, and `vm.etch` rejects them outright ("Eip7702 is not 23
bytes long"), which is what was breaking the fuzz test 8 runs in 8.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
# Conflicts: # .gas-snapshot
|
@coderabbitai review Queuing a review — the original attempt hit the OSS rate limit before reading the diff, so this PR has never actually been reviewed. Orientation, since this is an audit deliverable rather than a feature:
|
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Changed files ==='
git diff --name-status HEAD^ HEAD
printf '%s\n' '=== Diff summary ==='
git diff --stat HEAD^ HEAD
printf '%s\n' '=== Full pull request diff ==='
git diff --find-renames --find-copies --unified=80 HEAD^ HEADLength of output: 18575 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Pull request metadata ==='
gh pr view 76 --repo rainlanguage/rain.factory \
--json baseRefName,baseRefOid,headRefName,headRefOid,commits \
--jq '{baseRefName, baseRefOid, headRefName, headRefOid, commits: [.commits[] | {oid, messageHeadline}]}'
base_oid="$(gh pr view 76 --repo rainlanguage/rain.factory --json baseRefOid --jq '.baseRefOid')"
merge_base="$(git merge-base HEAD "$base_oid")"
printf '%s\n' '=== Review range ==='
printf 'merge_base=%s\nhead=%s\n' "$merge_base" "$(git rev-parse HEAD)"
printf '%s\n' '=== Changed files in complete pull request ==='
git diff --name-status "$merge_base" HEAD
printf '%s\n' '=== Complete Solidity diff ==='
git diff --find-renames --find-copies --unified=60 "$merge_base" HEAD -- '*.sol'
printf '%s\n' '=== Changed non-Solidity files ==='
git diff --name-only "$merge_base" HEAD -- ':!*.sol'Length of output: 68655 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Foundry configuration diff and active configuration ==='
git diff c1c2afd3d88405d3228cb3b21e55c9d63ba8f5be HEAD -- foundry.toml
cat -n foundry.toml
printf '%s\n' '=== Relevant source and test outlines ==='
ast-grep outline src/lib/LibICloneableFactoryV4.sol --items all
ast-grep outline test/src/interface/ICloneableV2.initialize.t.sol --items all
ast-grep outline test/src/interface/ICloneableFactoryV3.t.sol --items all
ast-grep outline test/lib/LibPublishedAbi.sol --items all
printf '%s\n' '=== Clone initialization implementation and factory forwarding ==='
rg -n -A70 -B15 'function cloneAndInitialize|cloneAndInitialize\s*\(' src/lib/LibICloneableFactoryV4.sol test/concrete/TestCloneFactory.sol
rg -n -A45 -B10 'function cloneDeterministic|function cloneDeterministicOpenSalt|function predictDeterministic' test/concrete/TestCloneFactory.sol
printf '%s\n' '=== ABI declarations under test ==='
cat -n src/interface/ICloneableV2.sol
cat -n src/interface/ICloneableFactoryV2.sol
cat -n src/interface/ICloneableFactoryV3.sol
cat -n src/interface/deprecated/ICloneableV1.sol
cat -n src/interface/deprecated/ICloneableFactoryV1.sol
cat -n src/interface/deprecated/IFactory.sol
printf '%s\n' '=== Complete changed initialization test ==='
cat -n test/src/interface/ICloneableV2.initialize.t.sol
printf '%s\n' '=== Complete changed V3 declaration test ==='
cat -n test/src/interface/ICloneableFactoryV3.t.solLength of output: 49730 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Obsolete fixture-path references ==='
rg -n -C2 'test/src/concrete/(TestCloneFactory|TestCloneable|TestCloneableFailure|TestCloneableEmitter|TestCloneableReverter|TestCloneableConformant|TestCloneableV1Shaped)\.sol' test || true
printf '%s\n' '=== Solidity test discovery paths ==='
fd -e sol . test | sort
printf '%s\n' '=== Imports in changed Solidity files whose targets do not exist ==='
python3 - <<'PY'
from pathlib import Path
import re
import subprocess
base = subprocess.check_output(
["git", "merge-base", "HEAD", "c1c2afd3d88405d3228cb3b21e55c9d63ba8f5be"],
text=True,
).strip()
changed = subprocess.check_output(
["git", "diff", "--name-only", base, "HEAD", "--", "*.sol"],
text=True,
).splitlines()
missing = []
for filename in changed:
path = Path(filename)
for line_no, line in enumerate(path.read_text().splitlines(), 1):
match = re.match(r'\s*import\s+.*?from\s+"([^"]+)";', line)
if not match:
continue
target = match.group(1)
if target.startswith(("src/", "test/")) and not Path(target).exists():
missing.append(f"{filename}:{line_no}: {target}")
print("\n".join(missing) if missing else "No missing src/ or test/ import targets.")
PY
printf '%s\n' '=== Pull request check status ==='
gh pr checks 76 --repo rainlanguage/rain.factory || trueLength of output: 3342 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Imports in changed, present Solidity files whose targets do not exist ==='
python3 - <<'PY'
from pathlib import Path
import re
import subprocess
base = "c1c2afd3d88405d3228cb3b21e55c9d63ba8f5be"
changed = subprocess.check_output(
["git", "diff", "--diff-filter=AMR", "--name-only", base, "HEAD", "--", "*.sol"],
text=True,
).splitlines()
missing = []
checked = []
for filename in changed:
path = Path(filename)
if not path.is_file():
continue
checked.append(filename)
for line_no, line in enumerate(path.read_text(encoding="utf-8").splitlines(), 1):
match = re.match(r'\s*import\s+.*?from\s+"([^"]+)";', line)
if not match:
continue
target = match.group(1)
if target.startswith(("src/", "test/")) and not Path(target).is_file():
missing.append(f"{filename}:{line_no}: {target}")
print("Checked:")
print("\n".join(checked))
print("\nMissing targets:")
print("\n".join(missing) if missing else "None")
PY
printf '%s\n' '=== Pull request check status ==='
gh pr checks 76 --repo rainlanguage/rain.factoryLength of output: 2615
I reviewed the complete PR range, not only the final commit.
The moved fixtures have no stale imports or missing internal import targets. The
|
The split into one file per deprecated interface made `testICloneableV1AbiPinned` and friends look redundant against their new contract names, so the previous commit shortened all three to `testAbiPinned`. That was wrong twice over. The PR's `## QA` block names those three tests as the killers of M45, M41, M42, M43, M36 and M46. That block is a record of a mutation run that actually happened; renaming its subjects makes it unfollowable, and editing it to match would be rewriting the evidence rather than keeping it true. The short name also collided. Two `testAbiPinned` already existed, so the rename made five identically named tests in one suite and `--match-test testAbiPinned` select all of them. The file split stands; only the function names go back. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Note
Updated after the fixture consolidation and the layout fix — everything below is the original record and is left as written.
The
## QAblock is a record of a mutation run that happened; its test names are unchanged and still resolve. What moved around them:TestCloneableConformantfolded intoTestCloneable, which now carries the one-shot init guard and the typed overload for the whole suite (test: fixtures out of the src mirror, and one conforming ICloneableV2 #80).TestCloneableEmitterandTestCloneableReverterare replaced byTestCloneableCallRecorderandTestCloneableRevert— byte for byte the same two files AMT coverage: g2-libicloneablefactoryv4-predi #78 carries, so the branches merge without touching each other.testInitializeOnlyOnce,testTypedOverloadRevertsInitializeSignatureFnandtestInitializeRevertBubblesVerbatimare rewired onto them and otherwise unchanged.testNothingCalledBeforeInitializegot STRONGER, and had to. The Emitter announced a stray call by emitting from its fallback; the CallRecorder deliberately does not emit from its fallback, so the swap alone would have been a silent regression. It now asserts the recorded SELECTOR SEQUENCE — exactly one call reached the clone, and it wasinitialize(bytes). Verified both directions: insert achild.call(...)intocloneAndInitializebetweenNewCloneandinitializeand the selector assertion kills it; remove the selector assertion and the same mutant PASSES on the log assertions alone.TestCloneableV1Shapedhad no test. It was imported by nothing.testV1ShapedImplementationIsRejectedis the test it was built for: a V4 factory calling a V1-shaped implementation reverts on thebytes32return decode, with NO revert data, before the sentinel comparison — asserted as observed, not assumed to beInitializationFailed. Mutation-checked: give the fixture abytes32return and the test fails.LibPublishedAbi.sol→test/lib/; the doubled.sol.t.solextensions dropped;ICloneableV2.sol.t.sol→ICloneableV2.initialize.t.sol(contractICloneableV2InitializeTest);DeprecatedInterfaces.t.solsplit one-per-interface intoICloneableV1.t.sol,ICloneableFactoryV1.t.sol,IFactory.t.sol. Every file undertest/src/**now has asrc/**counterpart, which is what that mirror is for.test/src/concrete/below are nowtest/concrete/.QA
testNewCloneAbiPinned,testFunctionAbiPinned,testNewCloneTopicZeroPinned,testNewCloneNameIsSharedAcrossTheInterfaceFamily,testFunctionSelectorsPinned,testNewCloneSharedByBothEntryPoints,testAbiPinned(×2:ICloneableV2,ICloneableFactoryV2),testCloneSelectorPinned,testNewCloneTopicRelationshipsAcrossTheFamily,testInitializeSelectorPinned,testInitializeSelectorSharedWithV1,testInitializeSignatureFnSelectorPinned,testSuccessSentinelPinned,testInitializeOnlyOnce,testTypedOverloadRevertsInitializeSignatureFn,testInitializeRevertBubblesVerbatim,testNothingCalledBeforeInitialize,testICloneableV1AbiPinned,testICloneableFactoryV1AbiPinned,testIFactoryAbiPinned,testImplementsICloneableFactoryV4,testPredictDeterministicAddressIsPureDelegation,testPredictDeterministicAddressOpenSaltIsPureDelegation,testPredictionsAreStatic,testCloneEntryPointsRouteToTheirOwnDerivation— each verified to fail on the mutated base by the mutation probe below (baseline green at 42 tests before, 69 after; every mutant that these tests kill SURVIVED the pre-existing suite in pass 1, which is the "fails on base" evidence).src/interface/ICloneableFactoryV3.sol:31->NewClonesender/implementation transposed (M03) ->testNewCloneAbiPinnedsrc/interface/ICloneableFactoryV3.sol:31->NewClonesender/clone transposed (M04) ->testNewCloneAbiPinnedsrc/interface/ICloneableFactoryV3.sol:31->NewCloneimplementation/clone transposed (M08) ->testNewCloneAbiPinnedsrc/interface/ICloneableFactoryV3.sol:31->senderindexed (M05) ->testCloneDeterministicEvent,testCloneDeterministicOpenSaltEvent(pre-existing),testNewCloneAbiPinned,testNewCloneSharedByBothEntryPointssrc/interface/ICloneableFactoryV3.sol:31->dataindexed (M06) -> samesrc/interface/ICloneableFactoryV3.sol:31->saltindexed (M07) -> samesrc/interface/ICloneableV2.sol:48-> return namesuccessdropped (M44) ->testAbiPinnedsrc/interface/ICloneableFactoryV2.sol:15->NewClonearity 3->4 (M20) ->testAbiPinned,testNewCloneTopicZeroPinned,testNewCloneTopicRelationshipsAcrossTheFamily,testNewCloneNameIsSharedAcrossTheInterfaceFamilysrc/interface/ICloneableFactoryV2.sol:15->senderindexed (M21) ->testAbiPinnedsrc/interface/ICloneableFactoryV2.sol:15-> sender/implementation transposed (M40) ->testAbiPinnedsrc/interface/ICloneableFactoryV2.sol:31->clonereturnsbytes32(M23) ->testAbiPinnedsrc/interface/deprecated/ICloneableV1.sol:26-> parameter renamed (M45) ->testICloneableV1AbiPinnedsrc/interface/deprecated/ICloneableFactoryV1.sol:15-> sender/clone transposed (M41) ->testICloneableFactoryV1AbiPinnedsrc/interface/deprecated/IFactory.sol:10->NewChildsender/child transposed (M42) ->testIFactoryAbiPinnedsrc/interface/deprecated/IFactory.sol:18->Implementationsender/implementation transposed (M43) ->testIFactoryAbiPinnedsrc/interface/deprecated/IFactory.sol:35->isChildreturnsaddressnotbool(M36) ->testIFactoryAbiPinnedsrc/interface/deprecated/IFactory.sol:35->isChildparameter renamed (M46) ->testIFactoryAbiPinned17/30 killed; survived: 0; no-run: 13; harness errors: 0.keccak256("NewClone(address,address,address,bytes32,bytes)"),keccak256("InitializeSignatureFn()")and friends rather than read back off the declaration. ABI entries are written out by hand as the JSON a consumer downloads. The twoICloneableV2MUSTs come verbatim fromsrc/interface/ICloneableV2.sol:15-42.TestCloneFactory's delegation is checked againstLibICloneableFactoryV4's own functions composed independently in the test, not against the factory's own output.