Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRenames and API tweaks across eval (eval2→eval4), many opcode/library renames and additions, parser/sub-parse safety guards and new errors, substantial test additions/updates, deployment constant updates, CLI/crate doc and minor dependency edits, and a submodule pointer bump. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/rain.interpreter.interface`:
- Line 1: Update the PR and commit metadata to document why
lib/rain.interpreter.interface was bumped: replace the vague commit message
"bump int" with a descriptive note summarizing the upstream changes (list key
modified APIs or behavioral changes) and add a short compatibility summary
stating whether the interpreter/interface boundary is backward-compatible or
what breaking changes exist; include evidence of testing by attaching results or
links showing you ran the interpreter integration tests and the
interface/contract tests (or describe manual checks) that exercise the key
symbols affected (refer to the lib/rain.interpreter.interface submodule and any
public API names that changed), and add a one-paragraph changelog entry in the
PR description referencing the new commit pointer and the test verification
performed.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
foundry.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
lib/rain.interpreter.interface
External audit fixes (all MEDIUM): - EXT-M01: Bounds check on second-byte read in literal dispatch - EXT-M02: Cursor bounds check after parseInterstitial in pragma - EXT-M03: Dispatch length overflow check in subParseLiteral - EXT-M04: LHS item count overflow check in parse Additional fixes: - EXT-L01: Explicit UppercaseHexPrefix error for 0X literals - EXT-I02: Documented unused ParseState param in boundHex - EXT-I03: Fixed misleading comment about non-ASCII in skipMask - R02-RUST-01: Genesis block underflow guard in replay_transaction - P0-1: Added Bash tool to audit-pass2 skill - P0-2: Added full command syntax to CLAUDE.md build pipeline - P0-3: Referenced foundry.toml as source of truth in CLAUDE.md - P0-4: Added file paths to TESTING.md base contracts - P0-5: Extracted shared GENERAL_RULES.md for audit skills - P0-6: Referenced known-false-positives.md in audit rules - P0-7: Added DISPaiRegistry to architecture docs Removed audit finding labels from test NatSpec comments. Regenerated pointers and deploy constants. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Pass 2 fixes: - P2-EI-1: Add eval4 InputsLengthMismatch library-level test, rename eval2->eval4 - P2-EI-2: Add integrityCheck2 BadOpInputsLength/BadOpOutputsLength tests - P2-EI-3: Add evalLoop remainder-only path test (7 fuzzed constants) - P2-CC-01: Fix Rainterpreter.supportsInterface to include IOpcodeToolingV1 - P2-CC-02: Add RainterpreterExpressionDeployer pointer consistency test - P2-CC-03: Add same-sender StateNamespace isolation test - A30-P2-2: Document ParserOutOfBounds as unreachable defensive guard - A30-P2-3: Raise testEndSourceByteLengthFuzz upper bound from 50 to 255 - A32-2: Add fuzz test for skipComment with arbitrary body content - A42-1: Add pushInputs push-overflow test - A44-1-P2: Add subParseWords no-sub-parsers-registered test - P2-EAD-02: Verify all 72 authoringMetaV2 word names Pass 5 (new): - A01: Math opcodes correctness review (31 opcodes verified, 3 INFO) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rrays Rename source and test files so CamelCase filenames convert to the same kebab-case as their authoring meta word names. Reorder all four parallel arrays in LibAllStandardOps (authoring meta, operand handlers, integrity pointers, opcode pointers) to match filesystem sort order. Renames: LibOpCtPop→BitwiseCountOnes, LibOpDecodeBits→BitwiseDecode, LibOpEncodeBits→BitwiseEncode, LibOpShiftBitsLeft→BitwiseShiftLeft, LibOpShiftBitsRight→BitwiseShiftRight, LibOpTimestamp→BlockTimestamp, LibOpPow→Power, LibOpMaxUint256→Uint256MaxValue, LibOpUint256Pow→Uint256Power. Add filesystem ordering test (LibCamelToKebab + ffi-based find|sort) to prevent future drift. Update deploy constants and regenerate pointers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
P2-01: 10 logic opcodes (greater-than, less-than, greater-than-or-equal-to, less-than-or-equal-to, equal-to, binary-equal-to, is-zero, if, any, every) P2-02: 4 math opcodes (max-positive-value, max-negative-value, min-positive-value, min-negative-value) P2-03: hash Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
P5-EXPGROWTH-01: Add @notice and @return tags to integrity in exponential-growth, linear-growth, uint256-max-value. P5-UINT256POW-01: Clarify left-to-right associativity in uint256-power library NatSpec. P5-HEADROOM-01: Dismissed — Rain-invented opcode, well tested. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/lib/eval/LibEval.sol (1)
3-3:⚠️ Potential issue | 🟡 MinorPragma version uses caret instead of exact version.
The coding guidelines specify "Solidity version must be exactly 0.8.25" for
**/*.solfiles. This file uses^0.8.25instead of=0.8.25.Suggested fix
-pragma solidity ^0.8.25; +pragma solidity =0.8.25;As per coding guidelines: "Solidity version must be exactly 0.8.25 with EVM target cancun."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/eval/LibEval.sol` at line 3, The pragma in LibEval.sol uses a caret range (^0.8.25) instead of the exact version required; update the pragma token (the line starting with "pragma solidity") to specify the exact compiler version by replacing the range specifier with an exact version specifier so it reads as =0.8.25, and ensure your project/EVM settings target Cancun as required by the coding guidelines.src/lib/op/erc20/LibOpERC20Allowance.sol (1)
3-3:⚠️ Potential issue | 🟡 MinorPin pragma to exact
0.8.25to match repository policy.Using
^0.8.25allows newer 0.8.x compilers; this file should be pinned to=0.8.25per coding guidelines and foundry.toml configuration (solc = "0.8.25").🔧 Proposed fix
-pragma solidity ^0.8.25; +pragma solidity =0.8.25;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/op/erc20/LibOpERC20Allowance.sol` at line 3, Update the solidity version pragma in this file: replace the caret-pinned pragma token "pragma solidity ^0.8.25;" with an exact pinned pragma "pragma solidity =0.8.25;" so the compiler version matches repository policy and foundry.toml; locate the line containing the "pragma solidity" declaration in LibOpERC20Allowance.sol and change the operator from ^ to =.test/src/lib/op/bitwise/LibOpBitwiseShiftRight.t.sol (1)
23-60:⚠️ Potential issue | 🟡 MinorRemove per-test fuzz run overrides.
Line 26, Line 45, and Line 59 set per-test fuzz runs to
100, which conflicts with the repo convention to rely on global fuzz settings.Suggested diff
- /// forge-config: default.fuzz.runs = 100 @@ - /// forge-config: default.fuzz.runs = 100 @@ - /// forge-config: default.fuzz.runs = 100Based on learnings: In Solidity test files across the repo, fuzz/test run settings should rely on global
foundry.tomland not per-function/per-test forge-config annotations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/src/lib/op/bitwise/LibOpBitwiseShiftRight.t.sol` around lines 23 - 60, Remove the per-test fuzz run overrides by deleting the "forge-config: default.fuzz.runs = 100" annotation lines from the three tests in LibOpBitwiseShiftRight.t.sol (functions testOpShiftBitsRightIntegrityHappy, testOpShiftBitsRightIntegrityZero, and testOpShiftBitsRightIntegrityNoop) so the tests rely on the global foundry.toml fuzz settings; ensure you only remove the annotation comments and do not modify the surrounding test logic or function signatures.
🧹 Nitpick comments (4)
test/src/lib/parse/LibParsePragma.keyword.t.sol (1)
297-300: Optional: tighten the test intent and remove redundant setup.You can drop the extra literal parser assignment and assert the returned cursor equals
endfor a clearer boundary invariant.Proposed cleanup
ParseState memory state = LibParseState.newState(data, "", "", LibAllStandardOps.literalParserFunctionPointers()); - state.literalParsers = LibAllStandardOps.literalParserFunctionPointers(); uint256 cursor = Pointer.unwrap(data.dataPointer()); @@ - uint256 cursorAfter = state.parsePragma(cursor, end); - (cursorAfter); + uint256 cursorAfter = state.parsePragma(cursor, end); + assertEq(cursorAfter, end, "cursor should stop at explicit end");Also applies to: 306-308
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/src/lib/parse/LibParsePragma.keyword.t.sol` around lines 297 - 300, Remove the redundant assignment to state.literalParsers after calling LibParseState.newState (which already sets literal parsers via LibAllStandardOps.literalParserFunctionPointers()), and tighten the test by asserting the returned cursor equals end to verify the boundary; specifically update the test around the LibParseState.newState invocation to delete the extra state.literalParsers = LibAllStandardOps.literalParserFunctionPointers() line and add an assertion that the function under test returns end.test/src/lib/parse/LibSubParse.subParseWords.t.sol (1)
172-181: Prefer asserting the specific custom error selector.
vm.expectRevert()here will pass on any revert reason, which weakens regression signal.🎯 Tighten the revert assertion
+import {UnknownWord} from "src/error/ErrParse.sol"; @@ - vm.expectRevert(); + vm.expectRevert(UnknownWord.selector); this.externalSubParseWords(bytecode);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/src/lib/parse/LibSubParse.subParseWords.t.sol` around lines 172 - 181, Replace the broad vm.expectRevert() in testSubParseWordsNoSubParsersUnknownReverts with an assertion for the specific custom error selector for UnknownWord so the test only passes for that exact revert; locate the testSubParseWordsNoSubParsersUnknownReverts function and the call this.externalSubParseWords(bytecode) and use vm.expectRevert with the abi-encoded selector for the UnknownWord error (referencing UnknownWord and OPCODE_UNKNOWN to form the expected selector/arguments) before invoking externalSubParseWords.test/src/lib/op/LibAllStandardOps.filesystemOrdering.t.sol (1)
81-85: Consider documenting alias maintenance requirements.The
isAliasfunction currently only checks for"now". If additional alias words are added in the future (words sharing a file with another word), this function must be updated. Consider adding a comment noting this maintenance requirement, or alternatively, deriving aliases programmatically (e.g., by detecting consecutive words that map to the same file).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/src/lib/op/LibAllStandardOps.filesystemOrdering.t.sol` around lines 81 - 85, The isAlias function only returns true for the hardcoded "now" token and needs a maintenance note or automated detection; update the isAlias(bytes32 word) implementation or its surrounding comment to document that any new alias words (tokens that share a file with another word) must be added here, and optionally implement programmatic detection (e.g., in the code that maps words-to-files detect duplicate file mappings or consecutive words mapping to the same file and treat them as aliases) so future aliases aren’t silently missed—refer to isAlias to locate the change and either add the explicit comment about maintenance or replace the hardcoded check with the detection logic.test/src/lib/op/bitwise/LibOpBitwiseCountOnes.t.sol (1)
16-23: Consider renaming test functions for consistency.The contract was renamed to
LibOpBitwiseCountOnesTestand references now useLibOpBitwiseCountOnes, but the test function names still use the oldCtPopnaming (e.g.,testOpCtPopIntegrity,testOpCtPopRun). For consistency and discoverability, consider renaming these to match the new naming convention (e.g.,testOpBitwiseCountOnesIntegrity).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/src/lib/op/bitwise/LibOpBitwiseCountOnes.t.sol` around lines 16 - 23, The test names still use the old "CtPop" prefix while the contract/target is now LibOpBitwiseCountOnes; rename the functions testOpCtPopIntegrity and testOpCtPopRun to testOpBitwiseCountOnesIntegrity and testOpBitwiseCountOnesRun respectively, and update any references to those functions or their names in the test contract LibOpBitwiseCountOnesTest and related calls so names match the LibOpBitwiseCountOnes operator for consistency and discoverability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/error/ErrParse.sol`:
- Around line 199-203: The NatSpec for the error LHSItemCountOverflow currently
has an untagged description and a `@param`, so add an explicit `@notice` tag before
the description sentence(s) to make the block fully-tagged; keep the existing
`@param`(uint256 offset) line unchanged and ensure the `@notice` text matches the
current description about the 255 LHS items per line/source and overflow
behavior.
In `@test/lib/string/LibCamelToKebab.sol`:
- Around line 6-12: The NatSpec block currently uses `@notice` followed by
untagged rule lines; update the comment so every entry is explicitly tagged
(e.g., keep `@notice` for the summary and add `@dev` entries for the rule list and
behavior lines describing the hyphen insertion rules and lowercasing). Locate
the CamelCase-to-kebab conversion NatSpec (the block above the conversion
implementation in LibCamelToKebab) and prepend `@dev` to each of the rule lines
and any additional explanatory sentences so no lines remain untagged.
In `@test/src/lib/integrity/LibIntegrityCheck.badOpIO.t.sol`:
- Around line 15-17: The NatSpec block describing integrityCheck2's revert
behavior contains untagged continuation lines (mentions BadOpInputsLength and
BadOpOutputsLength) which violates the rule that all entries must be explicitly
tagged; update that doc block so every line is explicitly tagged (e.g. prefix
each line with `@notice`) or collapse the text into a single `@notice` line
describing the surgical corruption test for
integrityCheck2/BadOpInputsLength/BadOpOutputsLength to ensure no untagged lines
remain.
In `@test/src/lib/parse/LibParseState.endSourceTotalOpsOverflow.t.sol`:
- Around line 35-40: The test testTotalOpsOverflow256 constructs two items that
are 129 ops each (using buildTree(6)), so it currently tests 258 total ops
rather than the intended 256 boundary; change the construction to produce
exactly 128 ops per item (e.g., use buildTree(5) or otherwise adjust the tree
depth used in the bytes.concat calls that build "a(... a())") so the two items
sum to 256, leaving the test exercising the exact overflow boundary; update the
occurrences of buildTree(6) inside testTotalOpsOverflow256 to the corrected
value and keep the vm.expectRevert(SourceTotalOpsOverflow.selector) assertion.
In `@test/src/lib/parse/LibSubParse.subParseLiteral.t.sol`:
- Around line 167-169: Update the test doc comment (the block describing
dispatch region behavior) to reflect that a dispatch region exceeding 0xFFFF now
causes an explicit revert instead of silent truncation; replace the lines
mentioning "silent truncation" and "sub-parser receives corrupted data" with
wording that explains the parser now reverts when a dispatch/body length exceeds
the 2-byte limit so the test matches the asserted revert behavior in this file's
test for subParseLiteral.
---
Outside diff comments:
In `@src/lib/eval/LibEval.sol`:
- Line 3: The pragma in LibEval.sol uses a caret range (^0.8.25) instead of the
exact version required; update the pragma token (the line starting with "pragma
solidity") to specify the exact compiler version by replacing the range
specifier with an exact version specifier so it reads as =0.8.25, and ensure
your project/EVM settings target Cancun as required by the coding guidelines.
In `@src/lib/op/erc20/LibOpERC20Allowance.sol`:
- Line 3: Update the solidity version pragma in this file: replace the
caret-pinned pragma token "pragma solidity ^0.8.25;" with an exact pinned pragma
"pragma solidity =0.8.25;" so the compiler version matches repository policy and
foundry.toml; locate the line containing the "pragma solidity" declaration in
LibOpERC20Allowance.sol and change the operator from ^ to =.
In `@test/src/lib/op/bitwise/LibOpBitwiseShiftRight.t.sol`:
- Around line 23-60: Remove the per-test fuzz run overrides by deleting the
"forge-config: default.fuzz.runs = 100" annotation lines from the three tests in
LibOpBitwiseShiftRight.t.sol (functions testOpShiftBitsRightIntegrityHappy,
testOpShiftBitsRightIntegrityZero, and testOpShiftBitsRightIntegrityNoop) so the
tests rely on the global foundry.toml fuzz settings; ensure you only remove the
annotation comments and do not modify the surrounding test logic or function
signatures.
---
Nitpick comments:
In `@test/src/lib/op/bitwise/LibOpBitwiseCountOnes.t.sol`:
- Around line 16-23: The test names still use the old "CtPop" prefix while the
contract/target is now LibOpBitwiseCountOnes; rename the functions
testOpCtPopIntegrity and testOpCtPopRun to testOpBitwiseCountOnesIntegrity and
testOpBitwiseCountOnesRun respectively, and update any references to those
functions or their names in the test contract LibOpBitwiseCountOnesTest and
related calls so names match the LibOpBitwiseCountOnes operator for consistency
and discoverability.
In `@test/src/lib/op/LibAllStandardOps.filesystemOrdering.t.sol`:
- Around line 81-85: The isAlias function only returns true for the hardcoded
"now" token and needs a maintenance note or automated detection; update the
isAlias(bytes32 word) implementation or its surrounding comment to document that
any new alias words (tokens that share a file with another word) must be added
here, and optionally implement programmatic detection (e.g., in the code that
maps words-to-files detect duplicate file mappings or consecutive words mapping
to the same file and treat them as aliases) so future aliases aren’t silently
missed—refer to isAlias to locate the change and either add the explicit comment
about maintenance or replace the hardcoded check with the detection logic.
In `@test/src/lib/parse/LibParsePragma.keyword.t.sol`:
- Around line 297-300: Remove the redundant assignment to state.literalParsers
after calling LibParseState.newState (which already sets literal parsers via
LibAllStandardOps.literalParserFunctionPointers()), and tighten the test by
asserting the returned cursor equals end to verify the boundary; specifically
update the test around the LibParseState.newState invocation to delete the extra
state.literalParsers = LibAllStandardOps.literalParserFunctionPointers() line
and add an assertion that the function under test returns end.
In `@test/src/lib/parse/LibSubParse.subParseWords.t.sol`:
- Around line 172-181: Replace the broad vm.expectRevert() in
testSubParseWordsNoSubParsersUnknownReverts with an assertion for the specific
custom error selector for UnknownWord so the test only passes for that exact
revert; locate the testSubParseWordsNoSubParsersUnknownReverts function and the
call this.externalSubParseWords(bytecode) and use vm.expectRevert with the
abi-encoded selector for the UnknownWord error (referencing UnknownWord and
OPCODE_UNKNOWN to form the expected selector/arguments) before invoking
externalSubParseWords.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (54)
CLAUDE.mdis excluded by!CLAUDE.mdTESTING.mdis excluded by!TESTING.mdaudit/2026-03-01-01/pass0/process.mdis excluded by!audit/**audit/2026-03-01-01/pass1/BaseRainterpreterExtern.mdis excluded by!audit/**audit/2026-03-01-01/pass1/ErrAll.mdis excluded by!audit/**audit/2026-03-01-01/pass1/LibDeployRegistry.mdis excluded by!audit/**audit/2026-03-01-01/pass1/LibEval.mdis excluded by!audit/**audit/2026-03-01-01/pass1/LibExtern.mdis excluded by!audit/**audit/2026-03-01-01/pass1/LibIntegrityCheck.mdis excluded by!audit/**audit/2026-03-01-01/pass1/LibInterpreterState.mdis excluded by!audit/**audit/2026-03-01-01/pass1/LibOpBitwise.mdis excluded by!audit/**audit/2026-03-01-01/pass1/LibOpCoreOps.mdis excluded by!audit/**audit/2026-03-01-01/pass1/LibOpERC20.mdis excluded by!audit/**audit/2026-03-01-01/pass1/LibOpERC721EVM.mdis excluded by!audit/**audit/2026-03-01-01/pass1/LibOpLogic.mdis excluded by!audit/**audit/2026-03-01-01/pass1/LibOpMath.mdis excluded by!audit/**audit/2026-03-01-01/pass1/LibOpStore.mdis excluded by!audit/**audit/2026-03-01-01/pass1/LibParse.mdis excluded by!audit/**audit/2026-03-01-01/pass1/LibParseLiteral.mdis excluded by!audit/**audit/2026-03-01-01/pass1/LibParseState.mdis excluded by!audit/**audit/2026-03-01-01/pass1/LibParseUtilities.mdis excluded by!audit/**audit/2026-03-01-01/pass1/Rainterpreter.mdis excluded by!audit/**audit/2026-03-01-01/pass1/RainterpreterExpressionDeployer.mdis excluded by!audit/**audit/2026-03-01-01/pass1/RainterpreterParser.mdis excluded by!audit/**audit/2026-03-01-01/pass1/RainterpreterReferenceExtern.mdis excluded by!audit/**audit/2026-03-01-01/pass1/RainterpreterStore.mdis excluded by!audit/**audit/2026-03-01-01/pass1/RustCrates.mdis excluded by!audit/**audit/2026-03-01-01/pass2/CoreConcrete.mdis excluded by!audit/**audit/2026-03-01-01/pass2/ErrAll.mdis excluded by!audit/**audit/2026-03-01-01/pass2/ExternAbstractDeploy.mdis excluded by!audit/**audit/2026-03-01-01/pass2/LibEvalIntegrity.mdis excluded by!audit/**audit/2026-03-01-01/pass2/LibOpAll.mdis excluded by!audit/**audit/2026-03-01-01/pass2/LibParse.mdis excluded by!audit/**audit/2026-03-01-01/pass2/LibParseUtilities.mdis excluded by!audit/**audit/2026-03-01-01/pass2/RustCrates.mdis excluded by!audit/**audit/2026-03-01-01/pass3/CoreConcrete.mdis excluded by!audit/**audit/2026-03-01-01/pass3/ErrAll.mdis excluded by!audit/**audit/2026-03-01-01/pass3/ExternAbstract.mdis excluded by!audit/**audit/2026-03-01-01/pass3/LibEvalIntegrity.mdis excluded by!audit/**audit/2026-03-01-01/pass3/LibOpAll.mdis excluded by!audit/**audit/2026-03-01-01/pass3/LibParse.mdis excluded by!audit/**audit/2026-03-01-01/pass3/RustCrates.mdis excluded by!audit/**audit/2026-03-01-01/pass4/CoreConcrete.mdis excluded by!audit/**audit/2026-03-01-01/pass4/ErrRust.mdis excluded by!audit/**audit/2026-03-01-01/pass4/ExternAbstract.mdis excluded by!audit/**audit/2026-03-01-01/pass4/LibEvalParse.mdis excluded by!audit/**audit/2026-03-01-01/pass4/LibOpAll.mdis excluded by!audit/**audit/2026-03-01-01/pass4/LibParseUtilities.mdis excluded by!audit/**audit/2026-03-01-01/pass5/LibOpMath.mdis excluded by!audit/**audit/2026-03-01-01/triage.mdis excluded by!audit/**src/generated/Rainterpreter.pointers.solis excluded by!**/generated/**src/generated/RainterpreterExpressionDeployer.pointers.solis excluded by!**/generated/**src/generated/RainterpreterParser.pointers.solis excluded by!**/generated/**src/generated/RainterpreterReferenceExtern.pointers.solis excluded by!**/generated/**
📒 Files selected for processing (85)
crates/eval/src/error.rscrates/eval/src/fork.rssrc/concrete/Rainterpreter.solsrc/concrete/extern/RainterpreterReferenceExtern.solsrc/error/ErrParse.solsrc/error/ErrSubParse.solsrc/lib/deploy/LibInterpreterDeploy.solsrc/lib/eval/LibEval.solsrc/lib/extern/reference/op/LibExternOpContextRainlen.solsrc/lib/integrity/LibIntegrityCheck.solsrc/lib/op/LibAllStandardOps.solsrc/lib/op/bitwise/LibOpBitwiseCountOnes.solsrc/lib/op/bitwise/LibOpBitwiseDecode.solsrc/lib/op/bitwise/LibOpBitwiseEncode.solsrc/lib/op/bitwise/LibOpBitwiseShiftLeft.solsrc/lib/op/bitwise/LibOpBitwiseShiftRight.solsrc/lib/op/erc20/LibOpERC20Allowance.solsrc/lib/op/evm/LibOpBlockTimestamp.solsrc/lib/op/math/LibOpPower.solsrc/lib/op/math/uint256/LibOpUint256MaxValue.solsrc/lib/op/math/uint256/LibOpUint256Power.solsrc/lib/parse/LibParse.solsrc/lib/parse/LibParsePragma.solsrc/lib/parse/LibParseState.solsrc/lib/parse/LibSubParse.solsrc/lib/parse/literal/LibParseLiteral.solsrc/lib/parse/literal/LibParseLiteralHex.solsrc/lib/parse/literal/LibParseLiteralSubParseable.solsrc/lib/state/LibInterpreterState.soltest/lib/string/LibCamelToKebab.soltest/lib/string/LibCamelToKebab.t.soltest/src/concrete/Rainterpreter.ierc165.t.soltest/src/concrete/RainterpreterExpressionDeployer.pointers.t.soltest/src/concrete/RainterpreterParser.parsePragmaEmpty.t.soltest/src/concrete/RainterpreterReferenceExtern.repeat.t.soltest/src/concrete/RainterpreterReferenceExtern.subParserIndexOutOfBounds.t.soltest/src/concrete/RainterpreterStore.getUninitialized.t.soltest/src/concrete/RainterpreterStore.namespaceIsolation.t.soltest/src/concrete/RainterpreterStore.overwriteKey.t.soltest/src/concrete/RainterpreterStore.setEmpty.t.soltest/src/concrete/RainterpreterStore.setEvent.t.soltest/src/lib/eval/LibEval.fBounds.t.soltest/src/lib/eval/LibEval.inputsLengthMismatch.t.soltest/src/lib/eval/LibEval.maxOutputs.t.soltest/src/lib/eval/LibEval.remainderOnly.t.soltest/src/lib/integrity/LibIntegrityCheck.badOpIO.t.soltest/src/lib/integrity/LibIntegrityCheck.t.soltest/src/lib/op/LibAllStandardOps.filesystemOrdering.t.soltest/src/lib/op/LibAllStandardOps.t.soltest/src/lib/op/bitwise/LibOpBitwiseCountOnes.t.soltest/src/lib/op/bitwise/LibOpBitwiseDecode.t.soltest/src/lib/op/bitwise/LibOpBitwiseEncode.t.soltest/src/lib/op/bitwise/LibOpBitwiseShiftLeft.t.soltest/src/lib/op/bitwise/LibOpBitwiseShiftRight.t.soltest/src/lib/op/crypto/LibOpHash.t.soltest/src/lib/op/evm/LibOpBlockTimestamp.t.soltest/src/lib/op/logic/LibOpAny.t.soltest/src/lib/op/logic/LibOpBinaryEqualTo.t.soltest/src/lib/op/logic/LibOpEqualTo.t.soltest/src/lib/op/logic/LibOpEvery.t.soltest/src/lib/op/logic/LibOpGreaterThan.t.soltest/src/lib/op/logic/LibOpGreaterThanOrEqualTo.t.soltest/src/lib/op/logic/LibOpIf.t.soltest/src/lib/op/logic/LibOpIsZero.t.soltest/src/lib/op/logic/LibOpLessThan.t.soltest/src/lib/op/logic/LibOpLessThanOrEqualTo.t.soltest/src/lib/op/math/LibOpMaxNegativeValue.t.soltest/src/lib/op/math/LibOpMaxPositiveValue.t.soltest/src/lib/op/math/LibOpMinNegativeValue.t.soltest/src/lib/op/math/LibOpMinPositiveValue.t.soltest/src/lib/op/math/LibOpPower.t.soltest/src/lib/op/math/LibOpSub.t.soltest/src/lib/op/math/uint256/LibOpUint256MaxValue.t.soltest/src/lib/op/math/uint256/LibOpUint256Power.t.soltest/src/lib/parse/LibParse.lhsOverflow.t.soltest/src/lib/parse/LibParseInterstitial.t.soltest/src/lib/parse/LibParseOperand.handleOperand.t.soltest/src/lib/parse/LibParsePragma.keyword.t.soltest/src/lib/parse/LibParseStackTracker.t.soltest/src/lib/parse/LibParseState.endSource.t.soltest/src/lib/parse/LibParseState.endSourceTotalOpsOverflow.t.soltest/src/lib/parse/LibSubParse.constantAccumulation.t.soltest/src/lib/parse/LibSubParse.subParseLiteral.t.soltest/src/lib/parse/LibSubParse.subParseWords.t.soltest/src/lib/parse/literal/LibParseLiteral.dispatch.t.sol
✅ Files skipped from review due to trivial changes (10)
- test/src/concrete/RainterpreterStore.overwriteKey.t.sol
- test/src/concrete/RainterpreterStore.getUninitialized.t.sol
- src/lib/integrity/LibIntegrityCheck.sol
- test/src/lib/parse/LibSubParse.constantAccumulation.t.sol
- src/lib/parse/literal/LibParseLiteralHex.sol
- test/src/concrete/RainterpreterStore.setEmpty.t.sol
- src/lib/state/LibInterpreterState.sol
- test/src/concrete/RainterpreterReferenceExtern.subParserIndexOutOfBounds.t.sol
- test/src/concrete/RainterpreterParser.parsePragmaEmpty.t.sol
- test/src/concrete/RainterpreterStore.setEvent.t.sol
…r docs - P3-ERR-1: Add @notice to 17 errors across ErrBitwise, ErrEval, ErrExtern, ErrParse - P3-EA-01: Change "word dispatches" to "opcode dispatches" in BaseRainterpreterExtern - P3-EA-02: Add @notice and @return to buildOpcode/IntegrityFunctionPointers - P3-EA-03: Name parameters and add @param/@return to 3 context op subParser functions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/op/math/uint256/LibOpUint256Power.sol`:
- Around line 10-12: The NatSpec block for LibOpUint256Power has an untagged
sentence "Errors on overflow." that is being parsed as continuation text; update
the comment to explicitly tag that sentence (for example prefix with `@dev` or
`@notice` as per project convention) so it is not treated as an implicit
continuation—locate the contract/contract-like declaration LibOpUint256Power and
its header comment and change the untagged line to an explicit NatSpec tag
(e.g., "@dev Errors on overflow.").
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
audit/2026-03-01-01/triage.mdis excluded by!audit/**
📒 Files selected for processing (14)
src/abstract/BaseRainterpreterExtern.solsrc/concrete/extern/RainterpreterReferenceExtern.solsrc/error/ErrBitwise.solsrc/error/ErrEval.solsrc/error/ErrExtern.solsrc/error/ErrParse.solsrc/lib/extern/reference/op/LibExternOpContextCallingContract.solsrc/lib/extern/reference/op/LibExternOpContextRainlen.solsrc/lib/extern/reference/op/LibExternOpContextSender.solsrc/lib/op/math/growth/LibOpExponentialGrowth.solsrc/lib/op/math/growth/LibOpLinearGrowth.solsrc/lib/op/math/uint256/LibOpUint256MaxValue.solsrc/lib/op/math/uint256/LibOpUint256Power.soltest/src/lib/op/logic/LibOpGreaterThanOrEqualTo.t.sol
✅ Files skipped from review due to trivial changes (1)
- src/lib/op/math/growth/LibOpLinearGrowth.sol
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/op/math/uint256/LibOpUint256MaxValue.sol
- test/src/lib/op/logic/LibOpGreaterThanOrEqualTo.t.sol
- src/lib/extern/reference/op/LibExternOpContextRainlen.sol
- CR-7: Fix boundary test to use exactly 128+128=256 ops (was 129+129=258)
- CR-8: Update stale comment describing old silent truncation behavior
- CR-9: Remove redundant literalParsers reassignment in pragma test
- CR-10: Assert specific UnknownWord("") error instead of bare expectRevert
- CR-12: Rename test functions from CtPop to BitwiseCountOnes
- Fix cargo fmt in fork.rs
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- P3-EA-05: Fix typo "determin" -> "determine" - P3-LPS-01: Add @title/@notice to LibParseState library - P3-LPS-02: Add @notice to checkParseMemoryOverflow - P3-LPP-02: Add @dev NatSpec to 4 pragma keyword constants - P3-LPST-02: Add @dev NatSpec to ParseStackTracker type - P3-LPL-02: Add @dev NatSpec to 5 literal parser index constants - P3-CC-01: Merge untagged lines into @notice on opcodeFunctionPointers - P3-CC-02: Use @inheritdoc on buildOperandHandler/LiteralParser - P3-CC-03: Add @notice/@return to 3 virtual functions in RainterpreterParser - P3-CC-04: Add @notice to 4 IDISPaiRegistry functions - P3-OPALL-01/02: Add @notice/@return to integrity() in 4 ERC uint256 files - P3-OPALL-03: Add @return to referenceFn() in LibOpUint256MaxValue - P3-OPALL-04: Add @notice to integrity() in LibOpEnsure Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Solidity fixes: - Remove unused IERC165 import, LibUint256Array import/using - Add virtual/override keywords to concrete contracts - Extract magic numbers to named constants (MAX_PAREN_OFFSET, MAX_STACK_RHS_OFFSET) - Replace raw hex masks with type(uint8).max/type(uint16).max - Add @title NatSpec to 7 parse libraries - Remove redundant casts and double parens - Suppress forge-lint unsafe-typecast warnings in test files - Update deploy constants after pointer cascade Rust fixes: - Add error path tests for Forker::call, call_committing, roll_fork - Add Forker::new() + add_or_select empty-forks test - Add RainSourceTrace::from_data() edge case tests (empty, partial, boundary) - Add CLI Parse command test - Add crate-level //! docs to all 6 crates - Add /// doc comments to all undocumented public items in cli, eval, parser, dispair - Fix "Rainalang" typo, wrong param names in new_with_fork doc, missing .await - Add missing decode_error param to alloy_call/alloy_call_committing docs - Remove unused serde_json/reqwest/once_cell dependencies from eval - Replace wildcard imports with specific imports in dispair and parser - Fix inconsistent TypedError format string in alloy_call_committing Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/op/math/uint256/LibOpUint256MaxValue.sol (1)
20-20:⚠️ Potential issue | 🟡 MinorUpdate stale opcode name in notice comment.
Line 20 still references
max-uint256, but this library and surrounding docs useuint256-max-value. Please keep the naming consistent to avoid confusion in generated docs/tests.Suggested doc fix
- /// `@notice` `max-uint256` opcode. Pushes type(uint256).max onto the stack. + /// `@notice` `uint256-max-value` opcode. Pushes type(uint256).max onto the stack.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/op/math/uint256/LibOpUint256MaxValue.sol` at line 20, The notice comment in LibOpUint256MaxValue.sol is using the stale opcode name `max-uint256`; update that comment to use the consistent name `uint256-max-value` (the comment above the opcode implementation that currently reads "max-uint256 opcode" should be changed to "uint256-max-value opcode") so docs/tests and surrounding libraries use the same opcode identifier.crates/cli/src/commands/parse.rs (1)
15-22:⚠️ Potential issue | 🟠 MajorDuplicate
-dshort flag inForkParseArgsCli.Lines 15 and 21 both use implicit
short, which resolves to-dfor bothdeployeranddecode_errors, causing a CLI argument collision.Suggested fix
pub struct ForkParseArgsCli { - #[arg(short, long, help = "The address of the deployer")] + #[arg(short = 'd', long, help = "The address of the deployer")] deployer: Address, #[arg(short, long, help = "The Rainlang string to parse")] rainlang_string: String, - #[arg(short, long, help = "Decode errors using the openchain.xyz database")] + #[arg(long, help = "Decode errors using the openchain.xyz database")] decode_errors: bool, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/commands/parse.rs` around lines 15 - 22, The struct ForkParseArgsCli has two fields using implicit short flags causing a collision (deployer and decode_errors both map to -d); update the arg attributes to give unique short flags (e.g., keep deployer as short='d' and set decode_errors to short='e' or remove the short for decode_errors) by editing the attributes on the deployer and/or decode_errors fields so their #[arg(...)] specify distinct short characters or omit short for one field; ensure the attribute changes are applied to the deployer: Address and decode_errors: bool fields to resolve the -d conflict.
🧹 Nitpick comments (11)
crates/eval/src/trace.rs (1)
567-622: Optional: factor repeated fixture-building into small test helpers.The byte-vector setup repeats across multiple tests. A tiny helper would keep these tests easier to extend.
♻️ Optional refactor
+ fn make_header(parent: u16, source: u16) -> Vec<u8> { + vec![ + (parent >> 8) as u8, + (parent & 0xFF) as u8, + (source >> 8) as u8, + (source & 0xFF) as u8, + ] + } + + fn make_word(last_byte: u8) -> [u8; 32] { + let mut word = [0u8; 32]; + word[31] = last_byte; + word + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/eval/src/trace.rs` around lines 567 - 622, Multiple tests (test_from_data_trailing_partial_word, test_from_data_one_full_word, test_from_data_one_full_word_plus_partial, test_from_data_two_full_words) repeat byte-vector construction for RainSourceTrace::from_data; extract a small test helper (e.g., fn build_trace_bytes(parent: u8, source: u8, payload: &[u8]) -> Vec<u8> or fn make_trace_from_parts(...)->RainSourceTrace) that constructs the 4-byte header and appends payload chunks (padding/truncation handled by the helper), then update each test to call that helper and assert on the returned RainSourceTrace as before to remove duplication.test/src/lib/op/LibAllStandardOps.filesystemOrdering.t.sol (1)
89-99: Consider adding NatSpec for consistency.The
bytes32ToStringhelper lacks documentation whileisAliashas@notice. Adding a brief@noticewould maintain consistency across helpers.+ /// `@notice` Converts a null-terminated bytes32 to a string. function bytes32ToString(bytes32 b) internal pure returns (string memory) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/src/lib/op/LibAllStandardOps.filesystemOrdering.t.sol` around lines 89 - 99, Add a short NatSpec `@notice` to the bytes32ToString helper to match the existing documentation style (e.g., similar to isAlias); update the bytes32ToString function declaration to include a one-line `@notice` describing that it converts a bytes32 value to a string by trimming trailing zero bytes, and optionally add `@param` and `@return` tags to document the input and returned string for consistency with other helpers.crates/eval/src/fork.rs (1)
819-927: Consider collapsing repetitive invalid-address tests into a helper/macro.The added coverage is strong, but this block is quite repetitive and can be made easier to maintain.
♻️ Optional refactor sketch
+fn assert_invalid_address_error<T>(result: Result<T, ForkCallError>) { + assert!(matches!( + result, + Err(ForkCallError::ExecutorError(ref msg)) if msg == "invalid address!" + )); +} + #[test] fn test_call_invalid_from_address_too_short() { let forker = Forker::new().unwrap(); - let result = forker.call(&[0u8; 19], &[0u8; 20], &[]); - assert!(matches!(result, Err(ForkCallError::ExecutorError(ref msg)) if msg == "invalid address!")); + assert_invalid_address_error(forker.call(&[0u8; 19], &[0u8; 20], &[])); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/eval/src/fork.rs` around lines 819 - 927, The tests repeat the same assertion for invalid-address errors across many cases (test_call_invalid_*, test_call_committing_*, test_call_invalid_to_address_*, etc.); extract the duplicated assertion into a small helper (e.g., fn assert_invalid_address_error<T>(res: Result<T, ForkCallError>)) or a test macro (e.g., invalid_address_test!) and replace each assert!(matches!(... if msg == "invalid address!")) with a single call to that helper, keeping existing calls to Forker::call, Forker::call_committing and roll_fork unchanged; ensure the helper lives near the tests and is used by all the tests that validate the "invalid address!" error.src/lib/parse/literal/LibParseLiteral.sol (1)
30-32: Make each line explicitly tagged in this NatSpec block.Line 32 continues
@noticewithout an explicit tag.As per coding guidelines, "When a NatSpec doc block contains any explicit tag (e.g.
@title), all entries must be explicitly tagged — untagged lines continue the previous tag, not implicit@notice."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/parse/literal/LibParseLiteral.sol` around lines 30 - 32, The NatSpec block atop LibParseLiteral contains mixed tagged and untagged lines so the continued line is treated as part of the previous tag; edit the doc comment for the LibParseLiteral contract so every descriptive line uses an explicit NatSpec tag (e.g., prefix the continued description lines with `@notice` or split into `@dev/`@notice/@title as appropriate) so no line remains untagged and the intended tags (`@title`, `@notice`) are explicit for each line.src/lib/parse/LibParseState.sol (1)
81-84: Apply explicit NatSpec tags on continued lines in these tagged blocks.Both blocks mix explicit tags with untagged continuation lines.
As per coding guidelines, "When a NatSpec doc block contains any explicit tag (e.g.
@title), all entries must be explicitly tagged — untagged lines continue the previous tag, not implicit@notice."Also applies to: 1047-1058
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/parse/LibParseState.sol` around lines 81 - 84, The NatSpec blocks that start with an explicit tag (e.g., the `@dev` block describing the maximum RHS offset and the other block around topLevel0) contain continued lines without explicit tags; update those blocks so every line uses an explicit NatSpec tag instead of relying on implicit continuation. Concretely, add the same explicit tag (for example `@dev` or `@notice`) to each continued line in the block describing the RHS offset/topLevel0 and the block at 1047-1058 so no lines are untagged, referencing the topLevel0 field and the RHS offset wording to locate the exact comment blocks to change.src/lib/parse/literal/LibParseLiteralDecimal.sol (1)
10-12: Use explicit NatSpec tags for continued lines in this doc block.Line 12 is an untagged continuation under an explicitly tagged block.
As per coding guidelines, "When a NatSpec doc block contains any explicit tag (e.g.
@title), all entries must be explicitly tagged — untagged lines continue the previous tag, not implicit@notice."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/parse/literal/LibParseLiteralDecimal.sol` around lines 10 - 12, The top-of-file NatSpec for the LibParseLiteralDecimal library contains an untagged continuation line; update the doc block so every line uses explicit NatSpec tags (e.g., change the untagged continuation on the line after `@title` to an explicit `@notice` or `@dev` entry) for the LibParseLiteralDecimal comment block to comply with the rule that any doc block with an explicit tag must tag all subsequent entries explicitly.src/lib/parse/LibParseOperand.sol (1)
21-23: Use explicit NatSpec tags on every line in this doc block.Line 23 is an untagged continuation under an explicitly tagged block. Please repeat the tag on continued lines.
As per coding guidelines, "When a NatSpec doc block contains any explicit tag (e.g.
@title), all entries must be explicitly tagged — untagged lines continue the previous tag, not implicit@notice."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/parse/LibParseOperand.sol` around lines 21 - 23, The top-of-file NatSpec for LibParseOperand currently mixes an explicit `@title` tag with untagged continuation lines; update the header doc block so every line uses an explicit NatSpec tag (e.g., repeat `@title` or add `@notice/`@dev as appropriate) instead of leaving line 23 untagged; locate the LibParseOperand library/header comment and duplicate the proper tag on the continued line(s) so each line is explicitly tagged.src/lib/parse/LibParse.sol (1)
61-65: Use explicit NatSpec tags on continued lines in this block.The
@devblock continues on untagged lines. Please explicitly tag continued lines.As per coding guidelines, "When a NatSpec doc block contains any explicit tag (e.g.
@title), all entries must be explicitly tagged — untagged lines continue the previous tag, not implicit@notice."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/parse/LibParse.sol` around lines 61 - 65, The NatSpec `@dev` block describing the maximum paren offset is using untagged continued lines; update that comment so every line continues with an explicit tag (e.g., repeat `@dev` on each subsequent line) instead of bare text. Locate the block referencing "Maximum paren offset", "parenOffset", and "pushOpToSource" in LibParse.sol and prefix each continued line with `@dev` so the entire paragraph uses explicit NatSpec tags. Ensure no untagged lines remain in that comment block.src/lib/parse/literal/LibParseLiteralHex.sol (1)
20-22: Use explicit NatSpec tags on all continued lines in these blocks.Both tagged doc blocks include untagged continuation lines.
As per coding guidelines, "When a NatSpec doc block contains any explicit tag (e.g.
@title), all entries must be explicitly tagged — untagged lines continue the previous tag, not implicit@notice."Also applies to: 28-30
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/parse/literal/LibParseLiteralHex.sol` around lines 20 - 22, The NatSpec blocks for LibParseLiteralHex contain continuation lines without explicit tags; update each doc block so every line uses explicit NatSpec tags (e.g., convert untagged continuation lines to use `@notice` or `@dev` as appropriate) — specifically edit the top doc block for the LibParseLiteralHex declaration and the subsequent documentation block around the parse function (the block referenced at lines 28-30) so no lines rely on implicit continuation of `@title` or other tags.test/src/lib/parse/LibParsePragma.keyword.t.sol (1)
305-307: AssertcursorAfterto make the regression stricter.Line 306 currently discards the result. Adding
assertEq(cursorAfter, end)would directly verify the parser stops at the intended boundary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/src/lib/parse/LibParsePragma.keyword.t.sol` around lines 305 - 307, The test currently ignores the result of state.parsePragma; make the regression stricter by asserting the parser stopped at the intended boundary: after calling uint256 cursorAfter = state.parsePragma(cursor, end); add an assertion using assertEq(cursorAfter, end) (referencing the cursorAfter variable, the end sentinel, and the state.parsePragma call) so the test fails if the parser does not return the expected cursor position.test/src/lib/parse/LibSubParse.subParseLiteral.t.sol (1)
176-180: Prefer asserting “no sub-parser call” on this overflow path.Since overflow should revert before delegation, consider replacing the mock with an explicit zero-call expectation to lock in short-circuit behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/src/lib/parse/LibSubParse.subParseLiteral.t.sol` around lines 176 - 180, Remove the vm.mockCall that stubs ISubParserV4.subParseLiteral2 on subParser and instead assert that subParser receives no calls on this overflow path; specifically, replace the vm.mockCall(subParser, abi.encodeWithSelector(ISubParserV4.subParseLiteral2.selector), ...) with a zero-call expectation or an explicit call-count assertion (e.g., using vm.expectCall / call recording) to verify that subParseLiteral2 is never invoked when the overflow short-circuits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@foundry.toml`:
- Line 25: The foundry config currently enables FFI in the default profile via
the line "ffi = true" under [profile.default]; remove that setting from
[profile.default] to reduce attack surface, or if FFI may be needed later move
the setting into a separate profile block (e.g., create "[profile.ffi]" and
place "ffi = true" there) so FFI is not enabled by default; locate the "ffi =
true" entry and either delete it or relocate it to the new profile stanza and
ensure no other default profile settings are changed.
In `@src/lib/op/erc20/uint256/LibOpUint256ERC20Allowance.sol`:
- Around line 15-16: The NatSpec block for the
contract/LibOpUint256ERC20Allowance doc comment uses an untagged continuation
line (the second line under the comment), which violates the rule that all
entries must be explicitly tagged; update the docblock for
LibOpUint256ERC20Allowance so the description is a single explicit `@notice` entry
(or explicitly tag each line with `@notice`), ensuring there are no untagged
continuation lines beneath `@title/`@notice.
In `@src/lib/op/erc20/uint256/LibOpUint256ERC20BalanceOf.sol`:
- Around line 15-16: The NatSpec block for the contract/Library
LibOpUint256ERC20BalanceOf contains an untagged continuation line after an
explicit `@notice`; update the doc block so every line is explicitly tagged (e.g.,
make the second/comment line also start with `@notice` or replace continuation
text with `@dev/`@notice tags) so no line remains untagged — locate the NatSpec
above the LibOpUint256ERC20BalanceOf declaration and add the appropriate
`@notice/`@dev tags to each line to satisfy the guideline.
In `@src/lib/parse/LibParse.sol`:
- Around line 181-183: The current overflow guard compares state.topLevel1 and
state.lineTracker to 0xFF which is too permissive and can allow counters to grow
into adjacent packed regions; change the check in the block that reverts
LHSItemCountOverflow(state.parseErrorOffset(cursor)) to enforce the actual
maximums used by the packed counter layout (introduce or use constants like
MAX_TOP_LEVEL1_COUNT and MAX_LINE_TRACKER_COUNT) and compare against those
instead of 0xFF, or compute the bounds from the bit-width of the packed fields
(e.g., (1 << WIDTH) - 1) so the comparison prevents any counter from exceeding
its allocated packed region before proceeding.
In `@src/lib/parse/LibParsePragma.sol`:
- Around line 25-27: The NatSpec block for LibParsePragma mixes explicit tags
and an untagged continuation; update the block so every line uses an explicit
tag (e.g., add an `@notice` to the currently untagged line on the third line of
the comment) to comply with the repo rule. Locate the contract/comment for
LibParsePragma and change the continuation line to an explicit tag (such as
`@notice`) so all entries in the doc block are explicitly tagged.
In `@src/lib/parse/LibParseStackTracker.sol`:
- Around line 7-9: Update the NatSpec for LibParseStackTracker to use explicit
tags on every line and correct the bit-layout description: replace the
mixed/untagged block with e.g. an `@notice/`@dev block that states the packed
layout exactly — low 8 bits store `current`, bits 8–15 store `inputs`, and bits
16+ store `max` (not a 128/128 split) — and ensure the continuation lines
(previously lines 12–14) are all explicitly tagged so no text remains untagged.
In `@test/src/lib/eval/LibEval.inputsLengthMismatch.t.sol`:
- Around line 18-20: The top-of-file NatSpec block for
LibEvalInputsLengthMismatchTest mixes tagged and untagged lines; update that
docblock so every line uses explicit NatSpec tags (e.g., repeat `@title`, `@notice`
or use `@dev/`@param as appropriate) rather than relying on implicit continuation;
also fix the other mixed lines referenced around the same block (the
continuation at 31-32) so all entries are explicitly tagged to comply with the
repo guideline.
In `@test/src/lib/eval/LibEval.remainderOnly.t.sol`:
- Around line 19-21: The NatSpec block beginning "/// `@notice` Tests that the
evalLoop remainder path correctly dispatches" contains tagged lines followed by
untagged continuation lines; update every line in that docblock (and the other
similar block with untagged continuations) to include explicit tags (e.g. prefix
continuation lines with /// `@notice` or the appropriate `@title/`@dev tag) so no
line relies on implicit continuation of the previous tag.
---
Outside diff comments:
In `@crates/cli/src/commands/parse.rs`:
- Around line 15-22: The struct ForkParseArgsCli has two fields using implicit
short flags causing a collision (deployer and decode_errors both map to -d);
update the arg attributes to give unique short flags (e.g., keep deployer as
short='d' and set decode_errors to short='e' or remove the short for
decode_errors) by editing the attributes on the deployer and/or decode_errors
fields so their #[arg(...)] specify distinct short characters or omit short for
one field; ensure the attribute changes are applied to the deployer: Address and
decode_errors: bool fields to resolve the -d conflict.
In `@src/lib/op/math/uint256/LibOpUint256MaxValue.sol`:
- Line 20: The notice comment in LibOpUint256MaxValue.sol is using the stale
opcode name `max-uint256`; update that comment to use the consistent name
`uint256-max-value` (the comment above the opcode implementation that currently
reads "max-uint256 opcode" should be changed to "uint256-max-value opcode") so
docs/tests and surrounding libraries use the same opcode identifier.
---
Nitpick comments:
In `@crates/eval/src/fork.rs`:
- Around line 819-927: The tests repeat the same assertion for invalid-address
errors across many cases (test_call_invalid_*, test_call_committing_*,
test_call_invalid_to_address_*, etc.); extract the duplicated assertion into a
small helper (e.g., fn assert_invalid_address_error<T>(res: Result<T,
ForkCallError>)) or a test macro (e.g., invalid_address_test!) and replace each
assert!(matches!(... if msg == "invalid address!")) with a single call to that
helper, keeping existing calls to Forker::call, Forker::call_committing and
roll_fork unchanged; ensure the helper lives near the tests and is used by all
the tests that validate the "invalid address!" error.
In `@crates/eval/src/trace.rs`:
- Around line 567-622: Multiple tests (test_from_data_trailing_partial_word,
test_from_data_one_full_word, test_from_data_one_full_word_plus_partial,
test_from_data_two_full_words) repeat byte-vector construction for
RainSourceTrace::from_data; extract a small test helper (e.g., fn
build_trace_bytes(parent: u8, source: u8, payload: &[u8]) -> Vec<u8> or fn
make_trace_from_parts(...)->RainSourceTrace) that constructs the 4-byte header
and appends payload chunks (padding/truncation handled by the helper), then
update each test to call that helper and assert on the returned RainSourceTrace
as before to remove duplication.
In `@src/lib/parse/LibParse.sol`:
- Around line 61-65: The NatSpec `@dev` block describing the maximum paren offset
is using untagged continued lines; update that comment so every line continues
with an explicit tag (e.g., repeat `@dev` on each subsequent line) instead of bare
text. Locate the block referencing "Maximum paren offset", "parenOffset", and
"pushOpToSource" in LibParse.sol and prefix each continued line with `@dev` so the
entire paragraph uses explicit NatSpec tags. Ensure no untagged lines remain in
that comment block.
In `@src/lib/parse/LibParseOperand.sol`:
- Around line 21-23: The top-of-file NatSpec for LibParseOperand currently mixes
an explicit `@title` tag with untagged continuation lines; update the header doc
block so every line uses an explicit NatSpec tag (e.g., repeat `@title` or add
`@notice/`@dev as appropriate) instead of leaving line 23 untagged; locate the
LibParseOperand library/header comment and duplicate the proper tag on the
continued line(s) so each line is explicitly tagged.
In `@src/lib/parse/LibParseState.sol`:
- Around line 81-84: The NatSpec blocks that start with an explicit tag (e.g.,
the `@dev` block describing the maximum RHS offset and the other block around
topLevel0) contain continued lines without explicit tags; update those blocks so
every line uses an explicit NatSpec tag instead of relying on implicit
continuation. Concretely, add the same explicit tag (for example `@dev` or
`@notice`) to each continued line in the block describing the RHS offset/topLevel0
and the block at 1047-1058 so no lines are untagged, referencing the topLevel0
field and the RHS offset wording to locate the exact comment blocks to change.
In `@src/lib/parse/literal/LibParseLiteral.sol`:
- Around line 30-32: The NatSpec block atop LibParseLiteral contains mixed
tagged and untagged lines so the continued line is treated as part of the
previous tag; edit the doc comment for the LibParseLiteral contract so every
descriptive line uses an explicit NatSpec tag (e.g., prefix the continued
description lines with `@notice` or split into `@dev/`@notice/@title as appropriate)
so no line remains untagged and the intended tags (`@title`, `@notice`) are explicit
for each line.
In `@src/lib/parse/literal/LibParseLiteralDecimal.sol`:
- Around line 10-12: The top-of-file NatSpec for the LibParseLiteralDecimal
library contains an untagged continuation line; update the doc block so every
line uses explicit NatSpec tags (e.g., change the untagged continuation on the
line after `@title` to an explicit `@notice` or `@dev` entry) for the
LibParseLiteralDecimal comment block to comply with the rule that any doc block
with an explicit tag must tag all subsequent entries explicitly.
In `@src/lib/parse/literal/LibParseLiteralHex.sol`:
- Around line 20-22: The NatSpec blocks for LibParseLiteralHex contain
continuation lines without explicit tags; update each doc block so every line
uses explicit NatSpec tags (e.g., convert untagged continuation lines to use
`@notice` or `@dev` as appropriate) — specifically edit the top doc block for the
LibParseLiteralHex declaration and the subsequent documentation block around the
parse function (the block referenced at lines 28-30) so no lines rely on
implicit continuation of `@title` or other tags.
In `@test/src/lib/op/LibAllStandardOps.filesystemOrdering.t.sol`:
- Around line 89-99: Add a short NatSpec `@notice` to the bytes32ToString helper
to match the existing documentation style (e.g., similar to isAlias); update the
bytes32ToString function declaration to include a one-line `@notice` describing
that it converts a bytes32 value to a string by trimming trailing zero bytes,
and optionally add `@param` and `@return` tags to document the input and returned
string for consistency with other helpers.
In `@test/src/lib/parse/LibParsePragma.keyword.t.sol`:
- Around line 305-307: The test currently ignores the result of
state.parsePragma; make the regression stricter by asserting the parser stopped
at the intended boundary: after calling uint256 cursorAfter =
state.parsePragma(cursor, end); add an assertion using assertEq(cursorAfter,
end) (referencing the cursorAfter variable, the end sentinel, and the
state.parsePragma call) so the test fails if the parser does not return the
expected cursor position.
In `@test/src/lib/parse/LibSubParse.subParseLiteral.t.sol`:
- Around line 176-180: Remove the vm.mockCall that stubs
ISubParserV4.subParseLiteral2 on subParser and instead assert that subParser
receives no calls on this overflow path; specifically, replace the
vm.mockCall(subParser,
abi.encodeWithSelector(ISubParserV4.subParseLiteral2.selector), ...) with a
zero-call expectation or an explicit call-count assertion (e.g., using
vm.expectCall / call recording) to verify that subParseLiteral2 is never invoked
when the overflow short-circuits.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
CLAUDE.mdis excluded by!CLAUDE.mdCargo.lockis excluded by!**/*.lockaudit/2026-03-01-01/triage.mdis excluded by!audit/**src/generated/RainterpreterExpressionDeployer.pointers.solis excluded by!**/generated/**src/generated/RainterpreterParser.pointers.solis excluded by!**/generated/**
📒 Files selected for processing (51)
crates/bindings/src/lib.rscrates/cli/src/commands/eval.rscrates/cli/src/commands/parse.rscrates/cli/src/execute.rscrates/cli/src/lib.rscrates/cli/src/output.rscrates/dispair/src/lib.rscrates/eval/Cargo.tomlcrates/eval/src/error.rscrates/eval/src/eval.rscrates/eval/src/fork.rscrates/eval/src/lib.rscrates/eval/src/trace.rscrates/parser/src/lib.rscrates/parser/src/v2.rscrates/test_fixtures/src/lib.rsfoundry.tomlsrc/concrete/Rainterpreter.solsrc/concrete/RainterpreterDISPaiRegistry.solsrc/concrete/RainterpreterExpressionDeployer.solsrc/concrete/RainterpreterParser.solsrc/concrete/extern/RainterpreterReferenceExtern.solsrc/interface/IDISPaiRegistry.solsrc/lib/deploy/LibInterpreterDeploy.solsrc/lib/extern/LibExtern.solsrc/lib/op/erc20/uint256/LibOpUint256ERC20Allowance.solsrc/lib/op/erc20/uint256/LibOpUint256ERC20BalanceOf.solsrc/lib/op/erc20/uint256/LibOpUint256ERC20TotalSupply.solsrc/lib/op/erc721/uint256/LibOpUint256ERC721BalanceOf.solsrc/lib/op/logic/LibOpEnsure.solsrc/lib/op/math/uint256/LibOpUint256MaxValue.solsrc/lib/parse/LibParse.solsrc/lib/parse/LibParseOperand.solsrc/lib/parse/LibParsePragma.solsrc/lib/parse/LibParseStackTracker.solsrc/lib/parse/LibParseState.solsrc/lib/parse/LibSubParse.solsrc/lib/parse/literal/LibParseLiteral.solsrc/lib/parse/literal/LibParseLiteralDecimal.solsrc/lib/parse/literal/LibParseLiteralHex.solsrc/lib/parse/literal/LibParseLiteralSubParseable.soltest/lib/string/LibCamelToKebab.soltest/src/lib/eval/LibEval.inputsLengthMismatch.t.soltest/src/lib/eval/LibEval.remainderOnly.t.soltest/src/lib/op/LibAllStandardOps.filesystemOrdering.t.soltest/src/lib/op/bitwise/LibOpBitwiseCountOnes.t.soltest/src/lib/parse/LibParseInterstitial.t.soltest/src/lib/parse/LibParsePragma.keyword.t.soltest/src/lib/parse/LibParseState.endSourceTotalOpsOverflow.t.soltest/src/lib/parse/LibSubParse.subParseLiteral.t.soltest/src/lib/parse/LibSubParse.subParseWords.t.sol
💤 Files with no reviewable changes (1)
- crates/eval/Cargo.toml
✅ Files skipped from review due to trivial changes (9)
- crates/test_fixtures/src/lib.rs
- crates/eval/src/lib.rs
- crates/eval/src/eval.rs
- crates/cli/src/commands/eval.rs
- crates/bindings/src/lib.rs
- src/interface/IDISPaiRegistry.sol
- src/lib/op/erc20/uint256/LibOpUint256ERC20TotalSupply.sol
- src/lib/op/erc721/uint256/LibOpUint256ERC721BalanceOf.sol
- src/lib/extern/LibExtern.sol
🚧 Files skipped from review as they are similar to previous changes (1)
- test/src/lib/parse/LibSubParse.subParseWords.t.sol
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The second return value from pushStackName is intentionally discarded since only the existence check is needed for duplicate LHS detection. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
@coderabbitai assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
Contributor
|
Tip For best results, initiate chat on the files or code changes. SIZE=L |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
New Features
Bug Fixes / Safety
Chores
Tests