test: scope the recorder's claim to the calls it can record

[thedavidmeister]

Closes #137

TestCloneableCallRecorder's notice said it "records the selector of every call
the proxy receives, in order", and the sSelectors doc repeated "Every selector
the proxy has been called with, in order". Only initialize(bytes) and the
fallback push. A call whose selector matches the fixture's own selectors() or
the auto-getter for bytes public sData dispatches to those view functions,
which cannot record.

That is not incidental — testCloneDeterministicInitializeIsTheOnlyCall asserts
selectors.length == 1 and then calls selectors() and sData(), so the test
depends on those two calls NOT being recorded. The same overclaim was restated in
that test's own NatSpec, under the suite's only check of
ICloneableFactoryV3.cloneDeterministic's "MUST NOT call any other functions on
the cloned proxy before initialize completes successfully".

Both claims are now scoped to what the fixture records.

The finding's stronger alternative — delete the two externals and read
sSelectors / sData with vm.load — is not taken. It would close a two-selector
hole that the code under test cannot reach: LibICloneableFactoryV4 knows nothing
about the fixture's ABI, so a regression adding a stray call to the fresh proxy
lands on an ICloneableV2 selector or a random one, both of which the fallback
records. The cost is hand-decoding a dynamic bytes4[] and a bytes out of clone
storage in two tests.

The finding also proposed a sentence on the non-payable fallback. Dropped: it
re-teaches the EVM, and the library's entry points are non-payable, so no value
reaches the proxy for it to describe.

Comment-only. No source, ABI or bytecode change.

QA

• Discriminating tests: n/a — comment-only change; tests never assert NatSpec
  text. The behaviour the comment now describes is already exercised:
  testCloneDeterministicInitializeIsTheOnlyCall reads both accessors after
  asserting the recorded length, which only passes because they do not record.
• Mutations applied: n/a — no executable line changes anywhere, so there is
  nothing to mutate.
• Oracle: test/concrete/TestCloneableCallRecorder.sol itself — selectors()
  and the sData auto-getter are view and reach neither push site (initialize,
  fallback), and Solidity's dispatcher matches them before the fallback.
• Category check: the category is "a fixture's guarantee stated wider than its
  own dispatch allows". TestCloneable and TestCloneFactory are the other
  fixtures with externals; neither claims to observe calls, so neither carries the
  shape. Distinct from TestCloneableCallRecorder is an ICloneableV2 that initializes successfully but can be re-initialized by anyone, contradicting TestCloneable's single-conforming-fixture notice #96/PR docs: scope TestCloneable's conforming-fixture notice to what is true #103, which added the "refuses nothing / not
  conforming" paragraph and did not touch these clauses; this branch keeps that
  paragraph and merges current main.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

Next included review available in 41 minutes.

Check out review usage here.

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

Run ID: 8fc451c9-e0de-43b7-a797-2a9c9a94ad54

📥 Commits

Reviewing files that changed from the base of the PR and between 710dff5 and 39ad610.

📒 Files selected for processing (2)

• test/concrete/TestCloneableCallRecorder.sol
• test/src/lib/LibICloneableFactoryV4.cloneDeterministic.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

• X
• Mastodon
• Reddit
• LinkedIn

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