Skip to content

AMT coverage: g4-icloneablefactoryv3-newclone - #76

Open
thedavidmeister wants to merge 9 commits into
mainfrom
2026-08-24-amt-g4-icloneablefactoryv3-newclone
Open

AMT coverage: g4-icloneablefactoryv3-newclone#76
thedavidmeister wants to merge 9 commits into
mainfrom
2026-08-24-amt-g4-icloneablefactoryv3-newclone

Conversation

@thedavidmeister

@thedavidmeister thedavidmeister commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Note

Updated after the fixture consolidation and the layout fix — everything below is the original record and is left as written.

The ## QA block is a record of a mutation run that happened; its test names are unchanged and still resolve. What moved around them:

  • Fixtures 5 → 3. TestCloneableConformant folded into TestCloneable, 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). TestCloneableEmitter and TestCloneableReverter are replaced by TestCloneableCallRecorder and TestCloneableRevert — byte for byte the same two files AMT coverage: g2-libicloneablefactoryv4-predi #78 carries, so the branches merge without touching each other. testInitializeOnlyOnce, testTypedOverloadRevertsInitializeSignatureFn and testInitializeRevertBubblesVerbatim are rewired onto them and otherwise unchanged.
  • testNothingCalledBeforeInitialize got 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 was initialize(bytes). Verified both directions: insert a child.call(...) into cloneAndInitialize between NewClone and initialize and the selector assertion kills it; remove the selector assertion and the same mutant PASSES on the log assertions alone.
  • TestCloneableV1Shaped had no test. It was imported by nothing. testV1ShapedImplementationIsRejected is the test it was built for: a V4 factory calling a V1-shaped implementation reverts on the bytes32 return decode, with NO revert data, before the sentinel comparison — asserted as observed, not assumed to be InitializationFailed. Mutation-checked: give the fixture a bytes32 return and the test fails.
  • Layout. LibPublishedAbi.soltest/lib/; the doubled .sol.t.sol extensions dropped; ICloneableV2.sol.t.solICloneableV2.initialize.t.sol (contract ICloneableV2InitializeTest); DeprecatedInterfaces.t.sol split one-per-interface into ICloneableV1.t.sol, ICloneableFactoryV1.t.sol, IFactory.t.sol. Every file under test/src/** now has a src/** counterpart, which is what that mirror is for.
  • Paths under test/src/concrete/ below are now test/concrete/.

QA

  • Discriminating tests: 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).
  • Mutations applied:
    • src/interface/ICloneableFactoryV3.sol:31 -> NewClone sender/implementation transposed (M03) -> testNewCloneAbiPinned
    • src/interface/ICloneableFactoryV3.sol:31 -> NewClone sender/clone transposed (M04) -> testNewCloneAbiPinned
    • src/interface/ICloneableFactoryV3.sol:31 -> NewClone implementation/clone transposed (M08) -> testNewCloneAbiPinned
    • src/interface/ICloneableFactoryV3.sol:31 -> sender indexed (M05) -> testCloneDeterministicEvent, testCloneDeterministicOpenSaltEvent (pre-existing), testNewCloneAbiPinned, testNewCloneSharedByBothEntryPoints
    • src/interface/ICloneableFactoryV3.sol:31 -> data indexed (M06) -> same
    • src/interface/ICloneableFactoryV3.sol:31 -> salt indexed (M07) -> same
    • src/interface/ICloneableV2.sol:48 -> return name success dropped (M44) -> testAbiPinned
    • src/interface/ICloneableFactoryV2.sol:15 -> NewClone arity 3->4 (M20) -> testAbiPinned, testNewCloneTopicZeroPinned, testNewCloneTopicRelationshipsAcrossTheFamily, testNewCloneNameIsSharedAcrossTheInterfaceFamily
    • src/interface/ICloneableFactoryV2.sol:15 -> sender indexed (M21) -> testAbiPinned
    • src/interface/ICloneableFactoryV2.sol:15 -> sender/implementation transposed (M40) -> testAbiPinned
    • src/interface/ICloneableFactoryV2.sol:31 -> clone returns bytes32 (M23) -> testAbiPinned
    • src/interface/deprecated/ICloneableV1.sol:26 -> parameter renamed (M45) -> testICloneableV1AbiPinned
    • src/interface/deprecated/ICloneableFactoryV1.sol:15 -> sender/clone transposed (M41) -> testICloneableFactoryV1AbiPinned
    • src/interface/deprecated/IFactory.sol:10 -> NewChild sender/child transposed (M42) -> testIFactoryAbiPinned
    • src/interface/deprecated/IFactory.sol:18 -> Implementation sender/implementation transposed (M43) -> testIFactoryAbiPinned
    • src/interface/deprecated/IFactory.sol:35 -> isChild returns address not bool (M36) -> testIFactoryAbiPinned
    • src/interface/deprecated/IFactory.sol:35 -> isChild parameter renamed (M46) -> testIFactoryAbiPinned
    • 13 further mutants (M01, M02, M10, M11, M12, M13, M22, M30, M31, M32, M33, M34, M35) do not compile and are reported as NO-RUN, never as killed. Full run: 17/30 killed; survived: 0; no-run: 13; harness errors: 0.
  • Oracle: the interface NatSpec and the literal signature strings, never the implementation. Topics and selectors are recomputed inline from 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 two ICloneableV2 MUSTs come verbatim from src/interface/ICloneableV2.sol:15-42. TestCloneFactory's delegation is checked against LibICloneableFactoryV4's own functions composed independently in the test, not against the factory's own output.
  • Category check: campaign brief asks for (A) a pre-existing-suite attribution pass over all 26 behaviours, (B) discriminating tests for every survivor, (C) an adversarial pass filed as issues. Covered A (pass 1, 23 mutants, 3 killed / 14 survived / 6 no-run, killers named), B (0 survivors on the final pass; the 6 behaviours with no scoreable mutation are marked UNPROBED in the matrix with the reason, not claimed as covered), C (NewClone is declared by three published interfaces under two signatures; ICloneableFactoryV1 and V2 share one topic0 #65, TestCloneable, the only worked ICloneableV2 fixture, violates the interface's once-only initialize MUST that ICloneableFactoryV4's open-salt safety argument rests on #67, ICloneableV2's normative NatSpec points implementers at the superseded ICloneableFactoryV2, and ICloneableV1 names a non-existent ICloneFactoryV1 #69 filed with both labels; four refuted or out-of-slice candidates recorded in the matrix section above rather than filed).

baku-ccron and others added 2 commits August 24, 2026 07:48
…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>
@coderabbitai

coderabbitai Bot commented Aug 24, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 20 minutes.

View limit details

Limit 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.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 1f33a11d-0d20-4ca1-bab5-74c87b5de7a5

📥 Commits

Reviewing files that changed from the base of the PR and between c1c2afd and cd4e60e.

📒 Files selected for processing (20)
  • .gas-snapshot
  • foundry.toml
  • test/concrete/TestCloneFactory.sol
  • test/concrete/TestCloneFactory.t.sol
  • test/concrete/TestCloneable.sol
  • test/concrete/TestCloneableCallRecorder.sol
  • test/concrete/TestCloneableFailure.sol
  • test/concrete/TestCloneableRevert.sol
  • test/concrete/TestCloneableV1Shaped.sol
  • test/lib/LibPublishedAbi.sol
  • test/src/concrete/TestCloneable.sol
  • test/src/interface/ICloneableFactoryV2.t.sol
  • test/src/interface/ICloneableFactoryV3.t.sol
  • test/src/interface/ICloneableV2.initialize.t.sol
  • test/src/interface/deprecated/ICloneableFactoryV1.t.sol
  • test/src/interface/deprecated/ICloneableV1.t.sol
  • test/src/interface/deprecated/IFactory.t.sol
  • test/src/lib/LibICloneableFactoryV4.checkImplementationCode.t.sol
  • test/src/lib/LibICloneableFactoryV4.cloneDeterministic.t.sol
  • test/src/lib/LibICloneableFactoryV4.cloneDeterministicOpenSalt.t.sol

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

baku-ccron and others added 2 commits August 24, 2026 12:17
`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>
thedavidmeister pushed a commit that referenced this pull request Aug 24, 2026
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>
baku-ccron and others added 4 commits August 24, 2026 12:40
# 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>
@thedavidmeister

Copy link
Copy Markdown
Contributor Author

@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:

  • Nothing under src/ is touched. The diff is tests, fixtures and file layout.
  • Fixture count goes 5 to 3 on this branch. TestCloneableConformant folds into TestCloneable, which now carries the one-shot init guard and the typed overload for the whole suite. TestCloneableEmitter and TestCloneableReverter are replaced by TestCloneableCallRecorder and TestCloneableRevert, byte for byte the same two files AMT coverage: g2-libicloneablefactoryv4-predi #78 carries so the branches merge without touching each other.
  • Please look hardest at testNothingCalledBeforeInitialize. The swap from TestCloneableEmitter to TestCloneableCallRecorder would have been a silent REGRESSION on its own: the Emitter announced a stray call by emitting from its fallback, the CallRecorder deliberately does not emit from its fallback. I verified that directly — with the new selectors().length == 1 assertion removed, a child.call(...) inserted into cloneAndInitialize between NewClone and initialize PASSES; with it, that mutant is killed. If you think the log-stream assertions still carry weight I would rather hear it than assume.
  • TestCloneableV1Shaped was imported by nothing on this branch — a fixture with no test. testV1ShapedImplementationIsRejected is the test it was built for. It asserts an EMPTY revert (vm.expectRevert(bytes(""))), because the bytes32 return decode reverts with no data before the sentinel comparison is reached, rather than producing InitializationFailed. That is the observed behaviour, mutation-checked: give the fixture a bytes32 return and the test fails. Push back if you think pinning a data-less revert is too brittle.
  • TestCloneableFailure and TestCloneableRevert are deliberately NOT collapsed. A fixture that returns a non-success sentinel cannot also revert with a typed error carrying data.

@coderabbitai

coderabbitai Bot commented Aug 24, 2026

Copy link
Copy Markdown
🧩 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^ HEAD

Length 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.sol

Length 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 || true

Length 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.factory

Length of output: 2615


@thedavidmeister No actionable findings.

I reviewed the complete PR range, not only the final commit.

testNothingCalledBeforeInitialize now has the required direct oracle. The selectors().length == 1 assertion detects a successful stray proxy call. The log assertions still add value because they verify that NewClone occurs before the clone emits Initializing, and that data reaches initialize unchanged.

testV1ShapedImplementationIsRejected correctly pins the observed empty revert. cloneAndInitialize performs a typed bytes32 decode at the external initialize call. A V1-shaped implementation returns empty data, so decoding reverts before the sentinel comparison.

The moved fixtures have no stale imports or missing internal import targets. The legal, static, and test GitHub checks pass.

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant