Smar 49 create ai conventions checker git action#1623
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds six new TypeScript files under Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@test/codeRabbitViolations/BadFacet.sol`:
- Line 4: The import in BadFacet.sol currently uses a relative path
("../src/LiFiDiamond.sol"); update it to use the remapping alias defined in
remappings.txt (replace the relative import with the remapped path, e.g., import
"lifi/LiFiDiamond.sol";) so the file imports LiFiDiamond via the lifi/ → src/
remapping rather than a relative path.
In `@test/codeRabbitViolations/BadFilePlacement.sol`:
- Around line 4-9: Move the MisplacedFacet contract (contract MisplacedFacet)
from test/codeRabbitViolations/ into src/Facets/ and regenerate or scaffold it
using the project's plop facet template; then add required NatSpec to the
contract header including `@title` MisplacedFacet, `@author` LI.FI (https://li.fi),
`@notice` describing the facet's purpose, and `@custom`:version X.Y.Z (update X.Y.Z
to the correct version).
In `@test/codeRabbitViolations/BadImportOrder.sol`:
- Around line 6-9: All four import lines (SomeContract, ILiFi, LibAsset,
Ownable) are missing trailing semicolons and are in the wrong order; update the
import statements to include semicolons, reorder them to "external libs →
interfaces → libraries → contracts" (e.g., import Ownable first, then ILiFi,
then LibAsset, then SomeContract), and switch relative paths to remapped paths
using the lifi/ remapping (e.g., lifi/Libraries/LibAsset.sol,
lifi/Interfaces/ILiFi.sol, lifi/Facets/SomeFacet.sol) while keeping the external
OpenZeppelin import as-is.
In `@test/codeRabbitViolations/BadPositiveSlippage.sol`:
- Around line 5-17: swapAndStartBridge has multiple issues: compute and pass the
adjusted min amount to the bridge, declare/use a proper bridge interface
instance, rename params to use leading underscores, and add required NatSpec and
modifiers. Specifically: in swapAndStartBridge (rename params to _amount and
_minAmountOut) replace the bridge call to pass the adjustedMinAmount (the
computed updatedMinAmount) instead of _minAmountOut; ensure a bridgeProtocol
variable exists (inject or reference the appropriate BridgeProtocol/IBridge
instance used elsewhere in the codebase) and use it for bridge(...); add the
function NatSpec and the modifiers nonReentrant, refundExcessNative,
validateBridgeData to the signature; and keep the updatedMinAmount calculation
consistent with the pattern from AcrossFacetV4.sol (lines ~137-147).
In `@test/codeRabbitViolations/BadReceiver.sol`:
- Around line 14-22: The emitted events LiFiTransferStarted and
LiFiTransferCompleted are not declared, causing compilation to fail; fix by
either importing the ILiFi interface that declares these events (add an import
for ILiFi and/or implement/extend it) or declare the two events locally above
the functions so the contract compiles while preserving the violation; update
the contract to include event declarations for LiFiTransferStarted and
LiFiTransferCompleted (or import ILiFi) so handleMessage and complete can emit
them without compiler errors.
In `@test/codeRabbitViolations/BadSecurity.sol`:
- Around line 6-19: BadSecurityContract has multiple issues: replace the revert
string in emergencyUpgrade with a custom error (e.g., Unauthorized) and use it
instead of require; remove the direct admin bypass by guarding emergencyUpgrade
behind the proper governance flow/Safe/timelock or a governor-only modifier (do
not call it directly from owner), add NatSpec for the contract and both
functions, rename parameters to use leading underscores (_newContract, _value,
_target) and validate inputs in setConfig (check _target != address(0) and
bounds for _value, and reuse any existing Validatable helpers), and ensure owner
access is handled via the approved access-control/governance pattern rather than
an unchecked owner require.
In `@test/codeRabbitViolations/BadSolidityContract.sol`:
- Around line 13-17: The event declaration using a qualified name is invalid:
replace the invalid declaration `event BadSolidityContract.Transfer(uint256
amount)` with a normal event declaration `event Transfer(uint256 amount)` so the
existing `emit Transfer(amount)` call in the function resolves; keep a comment
above the event explaining this is a test for the violated style (qualified
ContractName.EventName) rather than leaving the invalid syntax in
`BadSolidityContract`.
- Around line 1-2: Add a real SPDX identifier as the first line (e.g. "//
SPDX-License-Identifier: MIT"), then preserve a single blank line before the
existing pragma solidity ^0.8.17; to turn the issue into a
blank-line-between-SPDX-and-pragma test; update BadSolidityContract.sol so the
SPDX line replaces the current comment and is followed by one empty line and
then the pragma.
🧹 Nitpick comments (2)
test/codeRabbitViolations/BadDRYExample.sol (1)
7-11: Unannotated convention violation: revert string instead of custom error.Line 10 uses
require(success, "Transfer failed")which violates the custom-error convention, but unlike the other violations in this file, it's not called out in the comments. If this file is meant to be a comprehensive test case for CodeRabbit, consider either annotating this as an intentional violation or switching to a custom error so it doesn't muddy the DRY-focused test.As per coding guidelines: "Use custom errors instead of revert strings; prefer existing errors/helpers before adding new ones."
test/codeRabbitViolations/BadNonEVMFacet.sol (1)
10-16:require(false, ...)is an unusual anti-pattern — considerrevert(...)for clarity.
require(false, "Invalid receiver")on Line 15 is functionally equivalent torevert("Invalid receiver")but reads oddly because the condition is alwaysfalse. If the intent is to demonstrate using a revert string instead of a custom error,revert("Invalid receiver")makes the violation clearer. Additionally, the function parameterdataon Line 10 is missing the required_prefix (_data), which is another unannotated convention violation worth calling out in the comments if this is a comprehensive test case.Suggested improvements
- function startBridge(BadBridgeData memory data) public { + function startBridge(BadBridgeData memory data) public { // Violación: parámetro sin underscore prefix // Violación: No valida que receiverAddress != bytes32(0) para non-EVM chains // Debería revertir con InvalidNonEVMReceiver() si es cero if (data.receiverAddress == bytes32(0)) { - // Solo requiere, debería usar error específico - require(false, "Invalid receiver"); + // Violación: debería usar custom error InvalidNonEVMReceiver() + revert("Invalid receiver"); }
Test Coverage ReportLine Coverage: 86.72% (2979 / 3435 lines) |
…nt script examples - Move Solidity violations to solidity/ subfolder - Add deploymentScripts/ with 4 examples violating deployment script conventions Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@test/codeRabbitViolations/solidity/BadFacet.sol`:
- Around line 6-13: The comment incorrectly claims the contract lacks the
"Facet" suffix while the contract is named BadBridgeFacet; fix by either
renaming the contract symbol BadBridgeFacet to a non-suffixed name like
BadBridge to actually trigger the violation, or update the comment to accurately
reflect the real violations; additionally address the unused import LiFiDiamond
(remove the import if not needed or add a clear comment documenting the
intentional unused-import violation) so imports and contract name are consistent
with the documented violations.
In `@test/codeRabbitViolations/solidity/BadImportOrder.sol`:
- Around line 6-9: All four import statements (imports for SomeContract, ILiFi,
LibAsset, and Ownable) are missing trailing semicolons causing parse failures;
update each import line (the lines importing SomeContract, ILiFi, LibAsset, and
Ownable) to include a semicolon at the end of the statement so the Solidity file
compiles and can be used to test the import-order violation.
In `@test/codeRabbitViolations/solidity/BadInterface.sol`:
- Around line 17-18: BadImplementation currently claims to implement
BadExternalProtocol but only defines function1(), causing a compile error;
either declare the contract abstract (add the abstract keyword to
BadImplementation) or add stub external override implementations for the
remaining four functions declared in BadExternalProtocol so all interface
functions are implemented; locate the contract BadImplementation and either
change its declaration to "abstract contract BadImplementation" or add empty
bodies for the missing functions with the same signatures and the "external
override" specifier to satisfy the interface.
In `@test/codeRabbitViolations/solidity/BadNonEVMFacet.sol`:
- Around line 10-16: The comment is misleading: update the inline comment above
the check in function startBridge (and any Spanish comments on the same lines)
to state that the check exists but the violation is using a revert string
instead of the custom error; then replace the require(false, "Invalid receiver")
usage with reverting via the custom error InvalidNonEVMReceiver() (referencing
BadBridgeData.receiverAddress check in startBridge) and ensure all comments are
in English.
In `@test/codeRabbitViolations/solidity/BadPositiveSlippage.sol`:
- Around line 5-17: The contract BadSlippageFacet’s swapAndStartBridge
references an undeclared bridgeProtocol and thus won’t compile; add a minimal
bridge interface (e.g., IBridge with function bridge(uint256,uint256) external)
and a state variable (IBridge public bridgeProtocol) and initialize it
(constructor or setter) so bridgeProtocol.bridge(...) resolves, and while
editing fix the inline comment in swapAndStartBridge changing "positivo" to
"positive" so the comment is English-consistent; keep references to
swapAndStartBridge, bridgeProtocol, and BadSlippageFacet when making the
changes.
In `@test/codeRabbitViolations/solidity/BadReceiver.sol`:
- Around line 14-22: The contract emits events LiFiTransferStarted and
LiFiTransferCompleted in handleMessage and complete but those events are not
declared or imported, causing compile failures; add minimal event declarations
(e.g., event LiFiTransferStarted(bytes32 indexed id, address from, address to,
uint256 amount, uint256 timestamp); and event LiFiTransferCompleted(bytes32
indexed id, address from, address to, uint256 amount);) at the top of the
contract or import the file that defines them, ensuring the event names and
parameter types match the emit calls in handleMessage and complete.
In `@test/codeRabbitViolations/solidity/BadSolidityContract.sol`:
- Around line 1-2: Update the violation comment at the top of
BadSolidityContract.sol to accurately reflect only the real issues: remove the
phrase "wrong pragma" from the comment that currently reads "// Violation:
Missing SPDX license, wrong pragma, and blank line between SPDX and pragma" so
it instead mentions only the missing SPDX license and the missing blank line
before the pragma; reference the existing pragma token "pragma solidity
^0.8.17;" to ensure you do not change the correct pragma itself.
🧹 Nitpick comments (7)
test/codeRabbitViolations/solidity/BadFilePlacement.sol (1)
1-9: Looks good as a violation example.The file correctly demonstrates the file-placement violation. Consider also documenting the missing NatSpec (
@title,@author,@notice,@custom:version) as an additional intentional violation in the comments, since that's another convention CodeRabbit should flag. As per coding guidelines: "Contracts and interfaces must include NatSpec:@title(matching contract/interface name),@author LI.FI (https://li.fi),@notice(describing purpose), and@custom:version X.Y.Z".test/codeRabbitViolations/deploymentScripts/BadUpdateScript.s.sol (1)
17-18: Contract namedDeployScriptbut extendsUpdateScriptBase— consider documenting this as an additional naming violation.Update scripts should follow the
Update*.s.solnaming pattern per project conventions. The contract is namedDeployScriptyet inherits fromUpdateScriptBase, which is itself a convention violation worth calling out in the comments. Also,using stdJson for string(line 18) is declared but never used in the contract body.test/codeRabbitViolations/solidity/BadFacet.sol (1)
17-22: Mixed language in comments and missing parameter underscore prefix.Line 17 has a Spanish comment ("Debería ser primero") — keep comments in English for consistency. Additionally, the function parameter
dataon line 22 should use the leading underscore prefix (_data) per conventions; this could be documented as yet another violation. As per coding guidelines: "Function parameters must use leading underscore prefix (e.g.,_amount)".test/codeRabbitViolations/solidity/BadSecurity.sol (1)
10-13: Consider documenting the revert-string violation as well.Line 11 uses
require(msg.sender == owner, "Not owner")which violates the convention to use custom errors instead of revert strings. This could be explicitly called out as an additional violation alongside the governance bypass. As per coding guidelines: "Use custom errors instead of revert strings; prefer existing errors/helpers before adding new ones."test/codeRabbitViolations/deploymentScripts/BadZkSyncSync.s.sol (1)
35-51: Unused variablejsonon Line 37.
string memory jsonis assigned viavm.readFile(path)but never referenced —_getConfigContractAddresstakespathdirectly. This dead variable may confuse readers of the violation example. Either remove it or use it.Proposed fix
function getConstructorArgs() internal override returns (bytes memory) { string memory path = string.concat(root, "/config/someFacet.json"); - string memory json = vm.readFile(path); // Example: New logic added here that wasn't synced to zksync version address router = _getConfigContractAddress(test/codeRabbitViolations/solidity/BadDRYExample.sol (1)
9-10: Additional undocumented violation: usesrequirewith a revert string instead of a custom error.Per project conventions, custom errors should be used instead of revert strings (preferring existing errors from
GenericErrors.sol). Consider adding a comment documenting this as an intentional violation, so the example is self-documenting like the others.As per coding guidelines: "Use custom errors instead of revert strings; prefer existing errors/helpers before adding new ones."
test/codeRabbitViolations/deploymentScripts/BadNamingPattern.s.sol (1)
31-41: Unused variablejsonon Line 33.Same issue as in
BadZkSyncSync.s.sol:jsonis assigned but never read since_getConfigContractAddresstakespathdirectly. Remove it or use it to keep the example clean.Proposed fix
function getConstructorArgs() internal override returns (bytes memory) { string memory path = string.concat(root, "/config/someFacet.json"); - string memory json = vm.readFile(path); - address router = _getConfigContractAddress(
…it detection - Move violation examples from test/codeRabbitViolations/deploymentScripts/ to script/deploy/violations/ - CodeRabbit reviews script/deploy/**/*.s.sol, so violations must be in that path to be detected Co-authored-by: Cursor <cursoragent@cursor.com>
- Move violation examples from test/codeRabbitViolations/solidity/ to src/violations/ - CodeRabbit reviews src/**/*.sol, so violations must be in that path to be detected - Fix imports to use lifi/ remapping instead of relative paths Co-authored-by: Cursor <cursoragent@cursor.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Fix all issues with AI agents
In `@script/deploy/violations/BadDeployPattern.s.sol`:
- Around line 18-31: The run() function computes constructorArgs and returns
them but the deploy call uses deploy(type(SomeFacet).creationCode) which relies
on base getConstructorArgs() instead, so either override getConstructorArgs() in
this script to return the encoded constructorArgs (so deploy() picks them up) or
call the deploy helper variant that accepts constructor args directly if
available (passing constructorArgs into deploy); update either the
getConstructorArgs() implementation or the deploy invocation accordingly to
ensure the hardcoded router and someValue are actually used during deployment.
In `@script/deploy/violations/BadNamingPattern.s.sol`:
- Line 6: The import of SomeFacet in BadNamingPattern.s.sol points to a missing
file "SomeFacet.sol" causing CI to fail; fix by either (A) adding a minimal stub
contract named SomeFacet in src/Facets/SomeFacet.sol that compiles (e.g.,
declare contract SomeFacet {}), or (B) change the import in
BadNamingPattern.s.sol to an existing facet import used elsewhere in the repo
(replace the import path and symbol with a known existing facet), and ensure the
imported symbol SomeFacet is referenced consistently in the test/deploy file.
In `@src/violations/BadDRYExample.sol`:
- Around line 1-23: Add a NatSpec module-level tag `@custom`:version to the
BadDRYContract contract declaration to satisfy the
VersionControlAndAuditVerification pipeline, and replace the revert string in
transferNative (the require in function transferNative) with a custom error
declaration and use that error when the native transfer fails; reference the
contract name BadDRYContract and function transferNative so the custom error can
be declared at the top (e.g., TransferFailed) and used instead of "Transfer
failed". Ensure any intentional re-implementation violations (validateAddress,
executeSwap) remain documented if they are deliberate but do not block adding
the `@custom`:version and switching transferNative to the custom error.
In `@src/violations/BadFacet.sol`:
- Line 17: Update the Spanish inline comment in src/violations/BadFacet.sol next
to the variable `receiverAddress` to English (e.g., change `// Debería ser
primero` to `// Should be first`) for consistency with the rest of the PR; also
scan for the similar Spanish comment in src/violations/BadPositiveSlippage.sol
(the `positivo` comment) and replace it with the English equivalent (`//
positive`) so all violation example comments are in English.
In `@src/violations/BadFilePlacement.sol`:
- Around line 1-9: The MisplacedFacet contract is missing required NatSpec tags
and is located outside the accepted Facets directory; update by moving or
recreating the facet using the project's plop facet template into the Facets
directory (or if this is only a test artifact, relocate it out of src/, e.g., to
test/) and add top-of-contract NatSpec annotations: `@title`, `@author` LI.FI
(https://li.fi), `@notice` with a brief description, and `@custom`:version with a
valid semantic version (X.Y.Z) to satisfy the CI; ensure the contract name
MisplacedFacet remains unchanged if preserving tests.
In `@src/violations/BadGasOptimization.sol`:
- Around line 7-15: Rename the function parameters in transfer from to and
amount to the project-convention names _to and _amount, remove or replace the
empty assembly { } block with either a well-commented minimal example operation
(if this file is intended to demonstrate unjustified assembly) or delete the
block entirely so it’s not a no-op, add NatSpec for the function and its
parameters/returns and an explicit visibility justification comment for why
transfer is public, and replace any manual low-level transfer logic with a call
to a vetted library (e.g., SafeTransferLib/Solady) rather than reimplementing
transfer behavior in transfer.
- Around line 4-6: Add the required NatSpec header above the contract
declaration for BadGasContract: include `@title` that matches the filename
(BadGasOptimization), `@author` LI.FI (https://li.fi), `@notice` describing the
contract purpose, and a `@custom`:version X.Y.Z tag; either update the contract
name (BadGasContract -> BadGasOptimization) or change the `@title` to
BadGasOptimization so they match, then place the NatSpec immediately above the
contract BadGasContract/BadGasOptimization declaration.
In `@src/violations/BadImportOrder.sol`:
- Around line 11-17: Add a NatSpec header to the contract BadImportOrder
including at minimum `@title`, `@author` LI.FI (https://li.fi), `@notice`, and a
`@custom`:version tag (use semantic version like 0.1.0 or appropriate X.Y.Z) so
the VersionControlAndAuditVerification action passes; place the comment block
immediately above the contract declaration for BadImportOrder and ensure the
`@custom`:version line is present and filled with the correct version number.
- Around line 6-9: All import statements in BadImportOrder.sol are missing
trailing semicolons which prevents compilation; update each import line
importing SomeContract, ILiFi, LibAsset, and Ownable to terminate with a
semicolon (e.g., add ";" at the end of the import declarations) so the compiler
accepts the file and CI/linting can run.
In `@src/violations/BadInterface.sol`:
- Around line 4-14: Add the required NatSpec headers to the declarations so CI
passes: for the interface BadExternalProtocol (and the companion
BadImplementation if present) add `@title`, `@author` LI.FI (https://li.fi), `@notice`
describing the purpose, and `@custom`:version with a semantic version (e.g.,
0.0.1) above each contract/interface; alternatively, if you intend this to be an
intentional-violation test fixture, move the file out of src/ (e.g., to test/)
so the VersionControlAndAuditVerification check does not apply. Ensure the
annotations appear immediately above the interface/contract declarations
(BadExternalProtocol, BadImplementation).
In `@src/violations/BadNonEVMFacet.sol`:
- Line 10: The startBridge function's parameter violates naming
conventions—rename the parameter BadBridgeData data to BadBridgeData _data in
the startBridge declaration and update every usage of data inside the function
body to _data; ensure the symbol startBridge and the BadBridgeData type are
preserved and only the parameter identifier and internal references are changed.
- Around line 1-5: The file defines contract BadNonEVMFacet but is missing
required NatSpec tags causing CI failure; either move this violation example out
of src/ into test/violations/ to avoid audit/NatSpec checks, or keep it in src/
and add full NatSpec above the pragma for the contract including `@title`, `@author`
LI.FI (https://li.fi), `@notice` describing the contract purpose, and
`@custom`:version X.Y.Z so CI (VersionControlAndAuditVerification) passes; update
the file header accordingly referencing BadNonEVMFacet.
In `@src/violations/BadSecurity.sol`:
- Around line 1-19: Add a NatSpec `@custom`:version tag for the contract to
satisfy the VersionControlAndAuditVerification pipeline: insert a Solidity
NatSpec comment containing a `@custom`:version value (e.g., `@custom`:version 1.0.0
or the repo-standard version) immediately above the contract declaration for
BadSecurityContract in BadSecurity.sol so the contract-level documentation
includes the required tag.
In `@src/violations/BadSolidityContract.sol`:
- Around line 16-17: The event declaration uses the invalid qualified syntax
"event BadSolidityContract.Transfer(...)" which makes the Solidity parser fail;
change the event declaration to a normal event name (e.g., declare "event
Transfer(uint256 amount);" in the BadSolidityContract contract) and then
demonstrate the ContractName.EventName pattern in an emit statement (e.g., use
"emit BadSolidityContract.Transfer(...)" inside a function) so the file compiles
under 0.8.17 and still shows the emit-syntax violation; update any references to
the old declaration accordingly (look for the event declaration and any emit
sites in BadSolidityContract).
🧹 Nitpick comments (3)
src/violations/BadSecurity.sol (1)
7-11:owneris never initialized —emergencyUpgradeis effectively dead code.Since
ownerdefaults toaddress(0)and there's no constructor or setter, therequire(msg.sender == owner)on Line 11 will always revert. This weakens the violation demonstration (governance bypass is never reachable). Consider adding a constructor that setsowner = msg.senderso the "admin without timelock" violation is actually exercisable. Also, Line 11 uses a revert string instead of a custom error — another undocumented convention violation.Proposed fix
contract BadSecurityContract { address public owner; + constructor() { + owner = msg.sender; + } + // Violation: Admin function that bypasses timelock/Safe governance function emergencyUpgrade(address newContract) public { - require(msg.sender == owner, "Not owner"); + require(msg.sender == owner, "Not owner"); // Also a violation: uses revert string instead of custom error // Direct upgrade without going through governance / Safe / timelock }script/deploy/violations/BadNamingPattern.s.sol (1)
33-33: Unusedjsonvariable.
vm.readFile(path)result is assigned tojsonbut never referenced —_getConfigContractAddresstakes the filepath, not the parsed JSON string. Remove the dead assignment to keep the example clean.Suggested fix
function getConstructorArgs() internal override returns (bytes memory) { string memory path = string.concat(root, "/config/someFacet.json"); - string memory json = vm.readFile(path); - address router = _getConfigContractAddress(script/deploy/violations/BadZkSyncSync.s.sol (1)
37-37: Unusedjsonvariable — same issue asBadNamingPattern.s.sol.
vm.readFile(path)result is stored but never consumed;_getConfigContractAddressworks with the file path directly.Suggested fix
function getConstructorArgs() internal override returns (bytes memory) { string memory path = string.concat(root, "/config/someFacet.json"); - string memory json = vm.readFile(path); // Example: New logic added here that wasn't synced to zksync version
- Add gas limit to call in BadDRYExample.sol to avoid security warning - Add address(0) validation in BadReceiver.sol constructor - Maintain violation intent while fixing security issues for CodeRabbit review Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/violations/BadReceiver.sol`:
- Around line 1-27: This file contains intentional violation examples (contract
BadReceiver, state var executor, constructor, handleMessage, complete) and must
be moved out of the production source tree: relocate the BadReceiver.sol file
from src/ to a non-production test folder such as test/violations/ (or
tests/fixtures/violations/), update any test imports or references to point to
the new path, and ensure the moved file is excluded from production
build/lint/audit pipelines so CI no longer enforces src/ contract conventions on
it.
- Line 5: Add a Solidity NatSpec header above the contract declaration for
BadReceiver: include `@title`, `@author`, `@notice`, and a `@custom`:version tag (e.g.,
"v1.0.0") to satisfy CI; place this block immediately above the line containing
"contract BadReceiver" so the compiler and pipeline detect the metadata.
- Remove old Solidity and deployment script violations - Add 6 TypeScript violation examples in script/violations/: * BadImportOrder.ts: Incorrect import order * BadEthersUsage.ts: Uses deprecated ethers.js instead of viem * BadTypeDefinition.ts: Interfaces without I prefix, enums without Enum suffix * BadDRY.ts: Duplicates code that should use existing helpers * BadSafeDecode.ts: Doesn't use formatDecodedTxDataForDisplay * BadCodeQuality.ts: Uses any types, poor error handling These files violate TypeScript conventions (rule 3) for CodeRabbit validation. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@script/violations/BadCodeQuality.ts`:
- Around line 40-47: The catch block in badFunction is unreachable because the
try body is empty; add a throwing statement inside the try (so the catch can
execute) and replace the generic thrown Error('Error') in the catch with a
meaningful message that describes the failure; locate badFunction and update its
try block to throw (e.g., a deliberate error or failing operation) and change
the catch's new Error call to a descriptive message related to that operation.
🧹 Nitpick comments (4)
script/violations/BadTypeDefinition.ts (1)
1-46: Well-structured violation examples — ESLint will still block CI.The naming convention violations are clearly documented and correctly demonstrate the anti-patterns. However, even for test/validation files, the ESLint errors (unused
consolaimport on Line 12, unused paramsnetworkandresulton Line 44) will fail CI. Consider prefixing unused identifiers with_to satisfy ESLint while keeping the violation demonstrations intact, or adding an ESLint-disable directive at the file level if the intent is for CodeRabbit to flag these independently of ESLint.Suggested fix for unused identifiers
-import { consola } from 'consola' +import { consola as _consola } from 'consola'Or alternatively, remove the unused import entirely since it doesn't contribute to the naming-convention violations being demonstrated.
script/violations/BadEthersUsage.ts (2)
14-15: ESLint flags import order violation here too.ESLint reports that
consolashould be imported beforeethers(Line 15). This is an import-order violation on top of the ethers-usage violation. If this file is meant to demonstrate only ethers violations, consider fixing the import order to keep each violation file focused on its specific anti-pattern. If it's intentional overlap, a comment noting it would help clarity.
39-43:ensureBalancehas an empty body — consider adding a minimal ethers-based stub.The other functions in this file contain actual ethers.js calls that CodeRabbit can flag.
ensureBalanceis just comments with no code, making it a weaker violation example compared togetProvider,getWallet, andsendTransaction.script/violations/BadCodeQuality.ts (1)
13-13:consolais imported but never used.ESLint flags this. Since the file demonstrates using
console.log/console.errorinstead ofconsola, the unused import is arguably part of the demonstration. Consider adding a comment to make this explicit, e.g.,// Violation: consola imported but console.* used instead below.
…ction - Fix BadCodeQuality: make catch block reachable by adding code that throws - Improve BadImportOrder: make violations more explicit, add type import violation - Improve BadEthersUsage: add explicit deprecated function names, export functions - Improve BadTypeDefinition: add comments about checking existing types, reuse violations - Improve BadDRY: add explicit comments about which helpers should be used - Improve BadSafeDecode: add explicit comments about formatDecodedTxDataForDisplay usage All violations are now more explicit and should be detected by CodeRabbit. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
2 participants
Which Jira task belongs to this PR?
https://lifi.atlassian.net/browse/SMAR-49
Why did I implement it this way?
At the end we decided to try the approach without creating a GitHub action, instead, we update Coderabbit to check it on the PRs.
We are using this PR as a test but won't be needed to merge
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)