Refuse a post-deploy check that has no reads

[thedavidmeister]

Closes #61 (audit finding cov-08).

The hole

LibRainDeploy.checkResolvedAddresses guarded only that readCalls and expectedAddresses pair up, then looped over them:

if (readCalls.length != expectedAddresses.length) { revert ResolvedAddressesLengthMismatch(...); }
for (uint256 i = 0; i < readCalls.length; i++) { ... }

With both lengths zero the pairing guard passes and the loop body never runs, so the function returns having asserted nothing. checkResolvedAddressesOnNetworks then forks every network, logs Checking resolved addresses on network: … for each, and returns success across all of them having read nothing at all.

That is the same hazard NoNetworks already exists for one argument along — its own test says "an empty target set can never be mistaken for every read checking out" — and the read list had no equivalent. A consumer that builds readCalls from a source that came back empty (a config, a loop over resolved names) gets a green "verified on every network" immediately before migrating onto that deployment.

Nothing pinned it either way: testCheckResolvedAddressesLengthMismatchReverts fuzzes under vm.assume(readCallsLength != expectedLength), so equal-and-zero is excluded by construction, and every other call site passed a one- or two-element list.

The fix

New error, fail-closed on the empty read set, matching the NoNetworks / NoDeployCandidates house rule:

error NoResolvedAddressReads(address target);

The guard runs after the pairing check, not before it. An unpaired call is a mispairing whichever side happens to be empty, and reporting the empty pair as a "mismatch of zero against zero" would say nothing. So (0, n) stays a ResolvedAddressesLengthMismatch naming both lengths, and only the empty PAIR — the one case pairing cannot see — raises the new error. Hoisting it above the pairing check would swallow real mispairings; mutant M4 below is exactly that placement, and it is killed.

checkResolvedAddressesOnNetworks repeats the guard before its fork loop, for the reason it already repeats the pairing check there: an empty read set is reported without an RPC round trip, so a network outage cannot mask it.

Tests

Three new tests in test/src/lib/LibRainDeploy.t.sol:

• testCheckResolvedAddressesNoReadsReverts — an empty read set is refused rather than passing.
• testCheckResolvedAddressesEmptyPairIsNotALengthMismatch — the empty pair raises the new error, empty-reads-against-a-non-empty-expected-list is still ResolvedAddressesLengthMismatch(0, 1), and one real read still passes. The middle assertion deterministically pins the guard ORDER rather than leaving it to whether the fuzzer happens to draw a zero.
• testCheckResolvedAddressesOnNetworksNoReadsRevertsBeforeForking — refused before anything is forked, using an unconfigured RPC alias so reaching the fork loop is itself an error.

The pre-existing testCheckResolvedAddressesLengthMismatchReverts fuzz test is unchanged and still passes, which is the point of the ordering.

QA

• Discriminating tests: testCheckResolvedAddressesNoReadsReverts, testCheckResolvedAddressesEmptyPairIsNotALengthMismatch, testCheckResolvedAddressesOnNetworksNoReadsRevertsBeforeForking — each fails on base. Verified via mutants M1 and M5, which delete the added guards and so reproduce base behaviour exactly (literal base does not compile these tests, since NoResolvedAddressReads does not exist there); with the guard deleted, M1 is killed by the first two and M5 by the third.
• Mutations applied: 6 mutants via mutation-probe, 6/6 KILLED, 0 survived, 0 no-run, 0 harness errors, against a green 13-test baseline.
  • M1 checkResolvedAddresses: delete the empty-read guard entirely → testCheckResolvedAddressesNoReadsReverts, testCheckResolvedAddressesEmptyPairIsNotALengthMismatch
  • M2 checkResolvedAddresses: readCalls.length == 0 → == 1 → testCheckResolvedAddressesNoReadsReverts, …Match, …MismatchReverts, …DirtyWordReverts, …EmptyPairIsNotALengthMismatch
  • M3 checkResolvedAddresses: NoResolvedAddressReads(target) → (address(0)) → testCheckResolvedAddressesNoReadsReverts, testCheckResolvedAddressesEmptyPairIsNotALengthMismatch
  • M4 checkResolvedAddresses: hoist the guard ABOVE the pairing check → testCheckResolvedAddressesEmptyPairIsNotALengthMismatch (deterministic) and testCheckResolvedAddressesLengthMismatchReverts (fuzz-dependent — it needs the fuzzer to draw readCallsLength == 0, which is why the deterministic assertion was added rather than relying on it)
  • M5 checkResolvedAddressesOnNetworks: delete the pre-fork empty-read guard → testCheckResolvedAddressesOnNetworksNoReadsRevertsBeforeForking
  • M6 checkResolvedAddressesOnNetworks: NoResolvedAddressReads(target) → (address(0)) → testCheckResolvedAddressesOnNetworksNoReadsRevertsBeforeForking
  • The probe suite is deliberately scoped network-free (--match-test CheckResolvedAddresses minus the two fork tests) and was run with no .env present at all, so nothing in it can reach an RPC. A first pass over the full forge test produced 6/6 "killed" whose named killers were fork tests (testChainMatrix*, testDeployToNetworks*) that a guard inside checkResolvedAddresses cannot affect — flaky public-RPC failures being scored as kills. That matrix was discarded as untrustworthy rather than reported.
• Oracle: the invariant is stated independently of this implementation — an assertion helper that asserts nothing must not report success, which is the rule NoNetworks already encodes one argument along and NoDeployCandidates encodes for the candidate list ("a candidate the loop never reaches is a contract whose snapshot nothing anywhere anchors"). Expected revert data is built from the error signature via abi.encodeWithSelector, not read back from a run.
• Category check: the issue asks for (a) a fail-closed error on the empty read set, (b) the guard in checkResolvedAddresses, (c) the guard before the fork loop in checkResolvedAddressesOnNetworks, (d) tests covering all three. All four covered. One deliberate deviation from the issue's proposed patch, which the issue's own verification block flagged: the guard is placed AFTER the pairing check, not before it, so the existing length-mismatch fuzz test keeps its meaning for (0, n).

Full suite green: 218 passed, 0 failed, 17 suites. forge fmt --check clean.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Added validation to reject resolved-address checks when no reads are supplied.
  • Empty read sets now fail before network reads or fork creation.
  • Preserved distinct errors for empty inputs versus mismatched read and expected-value lists.

• Tests

  • Added coverage for empty inputs, mismatched lists, valid reads, and multi-network validation.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: e7d6d761-4283-4889-854e-5557807e4f96

📥 Commits

Reviewing files that changed from the base of the PR and between 86f8d96 and 6287d8c.

📒 Files selected for processing (2)

• src/lib/LibRainDeploy.sol
• test/src/lib/LibRainDeploy.t.sol

---

Walkthrough

The PR adds NoResolvedAddressReads(address target) and rejects empty resolved-address read sets in single-network and multi-network checks. Tests cover error selection, valid reads, and rejection before network forking.

Changes

Resolved-address read validation

Layer / File(s)	Summary
Single-network empty-read guard src/lib/LibRainDeploy.sol, test/src/lib/LibRainDeploy.t.sol	checkResolvedAddresses now rejects empty reads with NoResolvedAddressReads after pairing validation. Tests distinguish empty pairs from length mismatches and confirm valid reads still succeed.
Multi-network pre-fork guard src/lib/LibRainDeploy.sol, test/src/lib/LibRainDeploy.t.sol	checkResolvedAddressesOnNetworks rejects empty reads before creating forks. Tests verify the dedicated error.
Estimated code review effort: 2 (Simple)	~10 minutes

Merge Risk: ⚪ Minimal · up to 6287d

The change prevents post-deploy address checks from succeeding when they perform no reads and adds targeted coverage for the empty-read cases. No actionable merge-blocking risk remains after normal checks and review.

Possibly related PRs

• rainlanguage/rain.deploy#26: Both PRs modify resolved-address checks and their tests.

Suggested labels: ai:ready

Suggested reviewers: claude

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: rejecting post-deploy checks with no reads.
Linked Issues check	✅ Passed	The implementation and tests satisfy issue #61 by rejecting empty reads, preserving mismatch errors, and checking before network forking.
Out of Scope Changes check	✅ Passed	All changes directly support issue #61 and its required validation and test coverage.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch 2026-08-15-issue-61-zero-reads-check

---

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

[thedavidmeister]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

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.

[thedavidmeister]

Reviewed 6287d8c: ready — Closes #61. Read the diff independently against the issue.

Both guards land where the PR says: NoResolvedAddressReads(target) after the pairing check in checkResolvedAddresses, and again before the fork loop in checkResolvedAddressesOnNetworks. The placement is a deliberate improvement on the issue's proposed patch — (0, 1) stays a ResolvedAddressesLengthMismatch naming both lengths, and only the empty pair, the case pairing cannot see, raises the new error. testCheckResolvedAddressesEmptyPairIsNotALengthMismatch pins that order deterministically rather than leaning on the pre-existing fuzz test, which excludes equal-and-zero by construction under its vm.assume.

CI green (rainix static/legal/test), CodeRabbit reviewed, 0 unresolved threads. Mutation matrix 6/6 killed, and the first matrix was discarded rather than reported when its named killers turned out to be fork tests failing on flaky public RPCs — a guard inside checkResolvedAddresses cannot affect those.

Residual, not blocking and not a defect in this diff: the guard relocates the empty-read hazard to the caller rather than removing it. A consumer that may legitimately have zero reads — MigrationRegistry resolves nothing at all — now needs if (readCalls.length > 0) at the call site, and a config that came back empty takes that branch silently. Nothing in src/ or script/ calls either function, so both are consumer-facing entry points and the repo has no example of the caller shape this is designed against. The version that closes it is a declared read count on the suite, checked against the list handed in, so "resolves nothing" is stated rather than expressed by absence — the same move NoDeployCandidates makes one level up.