-
Notifications
You must be signed in to change notification settings - Fork 61
Enforce Solidity comment style (block comments only; allow SPDX) #236
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
✅ Deploy Preview for compose-diamonds canceled.
|
Co-authored-by: mudgen <[email protected]>
Co-authored-by: mudgen <[email protected]>
|
@copilot Can you change the pull request so that comments starting with "//" after code on the same line are ignored? uint96 feeNumerator = 500; // 5% |
Co-authored-by: mudgen <[email protected]>
... Done in d8bd75a. The script now only flags |
|
Can you change this pull request so that comments using "/* */" Is allowed? |
|
@copilot Can you change this pull request so that comments using "/* */" Is allowed? |
Co-authored-by: mudgen <[email protected]>
|
@copilot Make it so these style comments are allowed: /*//////////////////////////////////////////////////////////////
STORAGE HELPERS
//////////////////////////////////////////////////////////////*/ |
Co-authored-by: mudgen <[email protected]>
... Done in 68989f0. The script now tracks block comment state, so |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces automated enforcement of Solidity comment style guidelines through a GitHub Actions workflow and bash script. The implementation aims to ensure all Solidity files use block comment style exclusively, with exceptions for SPDX identifiers, URLs, and inline comments after code.
Key changes:
- Added GitHub Actions workflow to run comment style checks on all pushes and PRs
- Created bash script with AWK logic to scan Solidity files for forbidden
//comments at line starts - Configured exceptions for SPDX identifiers, URLs, inline comments, and content inside block comments
Critical issue identified: The implementation contradicts the repository's coding guidelines. The script allows both /* */ and /** */ block comments, but .github/copilot-instructions.md lines 318-331 explicitly require only /** */ (documentation block comments). This fundamental misalignment needs to be corrected before the PR can achieve its stated goal of enforcing the Compose style guide.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
.github/workflows/enforce-solidity-comments.yml |
GitHub Actions workflow that runs the comment style check on push/PR events; includes permissions hardening with contents: read |
.github/scripts/check-solidity-comments.sh |
Bash script using AWK to scan tracked .sol files and flag // comments at line starts, with logic to track block comment state and handle exceptions |
| @@ -0,0 +1,29 @@ | |||
| name: Enforce Solidity comment style | |||
Copilot
AI
Nov 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR title states "block comments only; allow SPDX" but according to the repository coding guidelines in .github/copilot-instructions.md lines 318-331, the requirement is more specific: only /** */ (documentation block comments) are allowed, not general /* */ block comments.
The title should be more precise, for example: "Enforce Solidity comment style (/** */ only; allow SPDX)" to accurately reflect the intended coding standard.
| name: Enforce Solidity comment style | |
| name: Enforce Solidity comment style (/** */ only; allow SPDX) |
| - name: Make check script executable | ||
| run: chmod +x .github/scripts/check-solidity-comments.sh | ||
|
|
Copilot
AI
Nov 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The workflow makes the script executable with chmod +x before running it (line 23), but the script file should ideally be committed with executable permissions set in git. This makes the chmod step redundant and ensures the script is executable in all contexts (not just CI).
You can set the executable bit before committing with:
git update-index --chmod=+x .github/scripts/check-solidity-comments.shWhile the current approach works, it's more conventional to commit scripts with the correct permissions.
| - name: Make check script executable | |
| run: chmod +x .github/scripts/check-solidity-comments.sh |
| # - Any occurrence of SPDX-License-Identifier: (regardless of comment delimiters) | ||
| # - URLs containing http:// or https:// (to avoid flagging links) | ||
| # - Inline comments after code (e.g., `uint x = 1; // comment`) | ||
| # - Block comments '/* */' and '/** */' (both styles allowed) |
Copilot
AI
Nov 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The AWK script allows both /* */ and /** */ block comment styles (as stated in the header comment on line 10), but according to the repository coding guidelines in .github/copilot-instructions.md lines 318-331, only /** */ (documentation block comments) should be allowed. The guidelines explicitly state: "Use /** */ block comment style for all documentation and inline comments."
The current script doesn't enforce this distinction and would accept non-documentation block comments like /* foo */, which violates the coding standard.
Consider adding a check to flag /* that is not followed by * (i.e., flag /* but allow /**).
| # Track block comment state | ||
| # Check for block comment start (/* or /**) | ||
| if (line ~ /\/\*/) { | ||
| in_block = 1 | ||
| } | ||
| # Check for block comment end | ||
| if (line ~ /\*\//) { | ||
| in_block = 0 | ||
| next # This line ends a block comment, skip further checks | ||
| } | ||
| # If we are inside a block comment, skip all checks for this line | ||
| if (in_block) { next } |
Copilot
AI
Nov 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The block comment state tracking has a subtle bug: when a line contains both /* and */ (single-line block comment), the script correctly sets in_block = 1 at line 58, then sets in_block = 0 at line 62 and skips the line with next. However, this logic fails for edge cases:
-
Multi-line comment ending: When processing the closing line of a multi-line block comment (e.g.,
*/), the script setsin_block = 0and then skips further checks withnext. This means if someone writes*/ // bad comment, the trailing//comment would not be flagged. -
Same-line start and end: For a line like
/** foo */ // bad, the script would setin_block = 1(line 58), then immediately setin_block = 0andnext(lines 62-63), never checking for the trailing//comment.
Consider restructuring the logic to check for violations on lines that end block comments, and handle same-line block comments more carefully.
| # Track block comment state | |
| # Check for block comment start (/* or /**) | |
| if (line ~ /\/\*/) { | |
| in_block = 1 | |
| } | |
| # Check for block comment end | |
| if (line ~ /\*\//) { | |
| in_block = 0 | |
| next # This line ends a block comment, skip further checks | |
| } | |
| # If we are inside a block comment, skip all checks for this line | |
| if (in_block) { next } | |
| # Track block comment state and handle lines with block comment start/end | |
| # If currently inside a block comment, check for end | |
| if (in_block) { | |
| # Check if this line ends the block comment | |
| if (line ~ /\*\//) { | |
| in_block = 0 | |
| # Remove everything up to and including the block comment end | |
| sub(/^.*\*\//, "", line) | |
| # Continue processing the remainder of the line | |
| } else { | |
| # Still inside block comment, skip line | |
| next | |
| } | |
| } | |
| # Check for block comment start and end on the same line (single-line block comment) | |
| if (line ~ /\/\*/ && line ~ /\*\//) { | |
| # Remove block comment portion | |
| # This handles cases like: /** foo */ // bad | |
| # Remove everything from block comment start to end | |
| sub(/\/\*.*\*\//, "", line) | |
| # Continue processing the remainder of the line | |
| } else if (line ~ /\/\*/) { | |
| # Block comment starts here, set state and remove everything from start of block comment | |
| in_block = 1 | |
| sub(/\/\*.*/, "", line) | |
| # Continue processing the remainder of the line | |
| } |
| echo "Solidity comment style check failed: only block comments (/* */ or /** */) are allowed." | ||
| echo "Allowed exceptions: SPDX identifier (SPDX-License-Identifier:) anywhere, URLs (http:// or https://), and inline comments after code." | ||
| echo "Please replace single-line // comments at the start of lines with /* */ or /** */ block comments." |
Copilot
AI
Nov 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error messages at lines 86-88 incorrectly state that both /* */ and /** */ block comments are allowed. According to the repository coding guidelines in .github/copilot-instructions.md lines 318-331, only /** */ (documentation block comments) should be allowed, not plain /* */ comments.
The message should be updated to reflect that only /** */ style block comments are permitted, aligning with the documented coding standards.
| echo "Solidity comment style check failed: only block comments (/* */ or /** */) are allowed." | |
| echo "Allowed exceptions: SPDX identifier (SPDX-License-Identifier:) anywhere, URLs (http:// or https://), and inline comments after code." | |
| echo "Please replace single-line // comments at the start of lines with /* */ or /** */ block comments." | |
| echo "Solidity comment style check failed: only documentation block comments (/** */) are allowed." | |
| echo "Allowed exceptions: SPDX identifier (SPDX-License-Identifier:) anywhere, URLs (http:// or https://), and inline comments after code." | |
| echo "Please replace single-line // comments at the start of lines with /** */ documentation block comments." |
| # - Any occurrence of SPDX-License-Identifier: (regardless of comment delimiters) | ||
| # - URLs containing http:// or https:// (to avoid flagging links) | ||
| # - Inline comments after code (e.g., `uint x = 1; // comment`) | ||
| # - Block comments '/* */' and '/** */' (both styles allowed) |
Copilot
AI
Nov 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR description states "Both /* */ and /** */ block comment styles are allowed" in the "Changes Made" section, but this contradicts the repository's coding guidelines defined in .github/copilot-instructions.md lines 318-331, which explicitly require: "Use /** */ block comment style for all documentation and inline comments."
The description should be corrected to reflect that only /** */ style comments are permitted, not both styles.
| # - Block comments '/* */' and '/** */' (both styles allowed) | |
| # - Block comments '/** */' (only this style allowed for documentation and inline comments; plain '/* */' is not permitted for documentation) |
| pull_request: | ||
| types: [opened, synchronize, reopened, edited] | ||
|
|
Copilot
AI
Nov 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The workflow is configured to run on all pushes and pull requests (branches: - '**'), even when only documentation or non-Solidity files are changed. This could lead to unnecessary CI runs.
Consider adding a paths filter to only trigger when .sol files or the check script itself are modified, similar to how other workflows in this repository use paths-ignore for efficiency. For example:
on:
push:
branches:
- '**'
paths:
- '**.sol'
- '.github/scripts/check-solidity-comments.sh'
- '.github/workflows/enforce-solidity-comments.yml'
pull_request:
types: [opened, synchronize, reopened, edited]
paths:
- '**.sol'
- '.github/scripts/check-solidity-comments.sh'
- '.github/workflows/enforce-solidity-comments.yml'| pull_request: | |
| types: [opened, synchronize, reopened, edited] | |
| paths: | |
| - '**.sol' | |
| - '.github/scripts/check-solidity-comments.sh' | |
| - '.github/workflows/enforce-solidity-comments.yml' | |
| pull_request: | |
| types: [opened, synchronize, reopened, edited] | |
| paths: | |
| - '**.sol' | |
| - '.github/scripts/check-solidity-comments.sh' | |
| - '.github/workflows/enforce-solidity-comments.yml' |
Coverage Report
Last updated: Sun, 30 Nov 2025 02:06:07 GMT for commit |
Gas ReportNo gas usage changes detected between All functions maintain the same gas costs. ✅ Last updated: Sun, 30 Nov 2025 02:06:45 GMT for commit |
Summary
Adds CI enforcement for the Compose Solidity style guide: only block comments (
/* */and/** */) are allowed. SPDX identifiers, URLs, inline comments after code, and//inside block comments are exempted.Changes Made
.github/workflows/enforce-solidity-comments.yml— GitHub Actions workflow triggered on push/PR to run the style check.github/scripts/check-solidity-comments.sh— Bash script that:.solfilesSPDX-License-Identifier:in any comment formhttp://orhttps://(URLs)uint96 feeNumerator = 500; // 5%)/* */and/** */block comment styles//inside block comments (e.g., section header style/*//...//*/)//comments at the start of lines outside of block commentsExample violation output:
Checklist
Before submitting this PR, please ensure:
Code follows the Solidity feature ban - No inheritance, constructors, modifiers, public/private variables, external library functions,
using fordirectives, orselfdestructCode follows Design Principles - Readable, uses diamond storage, favors composition over inheritance
Code matches the codebase style - Consistent formatting, documentation, and patterns (e.g. ERC20Facet.sol)
Code is formatted with
forge fmtExisting tests pass - Run tests to be sure existing tests pass.
New tests are optional - If you don't provide tests for new functionality or changes then please create a new issue so this can be assigned to someone.
All tests pass - Run
forge testand ensure everything worksDocumentation updated - If applicable, update relevant documentation
Make sure to follow the contributing guidelines.
Additional Notes
git ls-filesso only tracked files are checked; ignores deleted/binary filespermissions: contents: readper security best practicesuint96 feeNumerator = 500; // 5%/* */and/** */block comment styles are allowed//inside block comments are allowed (e.g.,/*//...//*/)Original prompt
This pull request was created as a result of the following prompt from Copilot chat.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.