test kvs short-circuit and store.set revert paths in LibFlow.flow

[thedavidmeister]

Summary

Two new tests pinning LibFlow.flow's store-side behavior:

• testFlowBasicFlowTimeNoStoreSetWhenKvsEmpty — explicit count=0 expectCall on STORE.set when kvs is empty. Locks the existing short-circuit so a future change that drops it is caught. Mutation verified: removing the kvs.length > 0 guard makes the test fail; reverting passes.

• testFlowBasicFlowTimeStoreSetRevertBubbles — when kvs is non-empty and store.set reverts, the revert MUST propagate out of flow() rather than be caught and swallowed.

Closes #329.

Test plan

• both tests pass on current code (one as static, one as 100-run fuzz)
• mutation: drop the kvs.length > 0 guard → empty-kvs test fails

🤖 Generated with Claude Code

Summary by CodeRabbit

• Tests
  • Enhanced test coverage for storage operations, including validation of empty input scenarios and error propagation handling.

[coderabbitai]

Walkthrough

Two new test cases verify uncovered code paths in LibFlow.flow: one asserts interpreterStore.set is not called when kvs is empty; another confirms that reverts from interpreterStore.set propagate correctly out of flow.flow.

Changes

LibFlow.flow edge-case test coverage

Layer / File(s)	Summary
Test: Empty kvs branch test/src/concrete/Flow.time.t.sol	testFlowBasicFlowTimeNoStoreSetWhenKvsEmpty mocks STORE.set and asserts via vm.expectCall(..., 0) that it is never invoked when kvs is empty.
Test: Store revert propagation test/src/concrete/Flow.time.t.sol	testFlowBasicFlowTimeStoreSetRevertBubbles mocks STORE.set to revert with a custom payload and asserts the revert bubbles out of flow.flow via vm.expectRevert.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related issues

• [TEST-4] [LOW] testFlowBasicFlowTime tests store writes, not time #422: Related to interpreterStore.set behavior and write coverage in Flow.time.t.sol; this PR adds the explicit edge-case assertions for empty kvs and revert propagation that may address overlapping testing objectives.

🚥 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 accurately summarizes the main change: adding tests for the kvs short-circuit and store.set revert paths in LibFlow.flow, which matches the two new test functions added.
Linked Issues check	✅ Passed	Both tests from issue #329 are implemented: testFlowBasicFlowTimeNoStoreSetWhenKvsEmpty verifies empty kvs doesn't call set, and testFlowBasicFlowTimeStoreSetRevertBubbles verifies store.set reverts propagate correctly.
Out of Scope Changes check	✅ Passed	All changes are within scope: two test functions added to Flow.time.t.sol directly address the objectives in issue #329 with no unrelated modifications.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch 2026-05-05-issue-329-kvs-shortcircuit-tests

---

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 and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/src/concrete/Flow.time.t.sol`:
- Around line 59-63: Replace the ABI-encoded revert payloads with raw string
literals: in the vm.mockCallRevert call that currently passes
abi.encode("STORE_SET_FAILED") (with IInterpreterStoreV2.set.selector) and in
the vm.expectRevert call that uses abi.encode("STORE_SET_FAILED"), change both
to the raw string "STORE_SET_FAILED" so the mock revert and the expected revert
use the same raw UTF-8 message per Foundry convention.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 8cd9ee35-495a-4e6e-ade6-3a31d9623c5b

📥 Commits

Reviewing files that changed from the base of the PR and between ceaea3c and c8746fb.

📒 Files selected for processing (1)

• test/src/concrete/Flow.time.t.sol

[coderabbitai]

🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

Foundry vm.mockCallRevert revert data bytes encoding — does passing a string literal differ from abi.encode of a string?

💡 Result:

Yes, passing a string literal to vm.mockCallRevert differs from abi.encode(string) for the retdata bytes. The official Foundry documentation shows examples using string literals directly for retdata, such as vm.mockCallRevert(..., "BALANCE_CHECK_FAILED") [1], and then vm.expectRevert("BALANCE_CHECK_FAILED") succeeds [1][2]. This works because Solidity implicitly converts the string literal "msg" to bytes via bytes("msg"), producing raw bytes equal to the UTF-8 encoding of the string (length-prefixed dynamic bytes data without Error(string) selector) [3][4]. In contrast, abi.encode(string) on a string produces ABI-encoded bytes for a single dynamic string argument: 32-byte offset (0x20) followed by length and data [5]. For example, abi.encode("msg") yields different bytes than bytes("msg"). A plain string literal revert like revert("msg") produces Error(string) ABI-encoded data: 0x08c379a0 + offset + length + "msg" [6]. But Foundry docs specify "raw return message" for retdata [1], and examples use raw string bytes to match vm.expectRevert(string) [1]. Thus, string literal → bytes("msg") (raw) ≠ abi.encode("msg") (ABI single arg) ≠ abi.encodeWithSignature("Error(string)", "msg") (full revert) [3]. Use string literals for raw revert messages matching vm.expectRevert(string); use abi.encodeWithSelector for custom errors [1].

Citations:

• 1: https://book.getfoundry.sh/reference/cheatcodes/mock-call-revert
• 2: https://foundry-book.zksync.io/reference/cheatcodes/mock-call-revert
• 3: incorrect docs for vm.expectRevert foundry-rs/foundry#5288
• 4: https://getfoundry.sh/reference/cheatcodes/expect-revert
• 5: https://saxenism.com/blog/solidity-encodings
• 6: https://gist.github.com/pcaversaccio/e1aebf9222424e9c1fbc82b8bf5fea8b

---

🏁 Script executed:

rg "slither-disable-next-line" test/src/concrete/Flow.time.t.sol

Repository: rainlanguage/flow

Length of output: 43

---

Use a raw string literal for the mock revert payload instead of abi.encode().

The test is self-consistent and will pass, but abi.encode("STORE_SET_FAILED") produces ABI-encoded bytes (32-byte offset + 32-byte length + 32-byte padded data), whereas a raw string literal produces raw UTF-8 bytes without the ABI encoding wrapper. These are different formats.

The idiomatic Foundry pattern (as shown in official mockCallRevert documentation) is to pass a raw string literal directly:

vm.mockCallRevert(address(token), ..., "BALANCE_CHECK_FAILED");
vm.expectRevert("BALANCE_CHECK_FAILED");

Using abi.encode() here diverges from Foundry convention and could mislead future readers about the intended format.

♻️ Proposed fix

-        vm.mockCallRevert(
-            address(STORE), abi.encodeWithSelector(IInterpreterStoreV2.set.selector), abi.encode("STORE_SET_FAILED")
-        );
-
-        vm.expectRevert(abi.encode("STORE_SET_FAILED"));
+        vm.mockCallRevert(
+            address(STORE), abi.encodeWithSelector(IInterpreterStoreV2.set.selector), "STORE_SET_FAILED"
+        );
+
+        vm.expectRevert("STORE_SET_FAILED");

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/src/concrete/Flow.time.t.sol` around lines 59 - 63, Replace the
ABI-encoded revert payloads with raw string literals: in the vm.mockCallRevert
call that currently passes abi.encode("STORE_SET_FAILED") (with
IInterpreterStoreV2.set.selector) and in the vm.expectRevert call that uses
abi.encode("STORE_SET_FAILED"), change both to the raw string "STORE_SET_FAILED"
so the mock revert and the expected revert use the same raw UTF-8 message per
Foundry convention.