Move coding-style check to GitHub actions #16266

r0qs · 2025-10-21T10:18:49Z

Part of CI refactor suggested by #16136 and #16253 (comment). This PR replaces our current code-style setup by a github action. After merging we can completely remove the need of a bot account and remove the GITHUB_ACCESS_TOKEN from CircleCI.

r0qs · 2025-10-27T21:23:29Z

Note that ci/circleci: chk_coding_style will need to be replaced by github chk_coding_style in the branch protection setting as a required check.

.github/workflows/code-style.yml

Co-authored-by: Heath Dutton <[email protected]>

heathdutton · 2025-12-23T16:29:18Z

The main scenario is when a file is modified but the style error is on a line that's not part of the diff hunk. GitHub's createReviewComment API only allows comments on lines within the diff context (changed lines plus surrounding context lines).

For example, if a PR modifies lines 10-20 of a file, but there's a pre-existing style error on line 500, the API will reject the comment with a 422 error because line 500 isn't in the diff.

This could happen if:

The style checker finds errors in parts of a modified file that weren't touched by this specific PR
A rebase brings in new code that has style issues in non-diff lines
The script runs against files that are in the PR but where the specific error line isn't in the diff context

Since the codebase should be clean as you noted, this is an edge case, but the try/catch prevents a noisy failure when it does happen.

r0qs · 2025-12-23T16:38:10Z

The main scenario is when a file is modified but the style error is on a line that's not part of the diff hunk. GitHub's createReviewComment API only allows comments on lines within the diff context (changed lines plus surrounding context lines).

For example, if a PR modifies lines 10-20 of a file, but there's a pre-existing style error on line 500, the API will reject the comment with a 422 error because line 500 isn't in the diff.

This could happen if:
* The style checker finds errors in parts of a modified file that weren't touched by this specific PR

* A rebase brings in new code that has style issues in non-diff lines

* The script runs against files that are in the PR but where the specific error line isn't in the diff context
Since the codebase should be clean as you noted, this is an edge case, but the try/catch prevents a noisy failure when it does happen.

Thanks for the clarification! While these should be rare given our CI enforcement, I agree that it is better to be defensive here, because changes in the code style script can also change what will be detected. So, I've added the try-catch ;)

r0qs · 2025-12-23T17:31:22Z

Thanks for the clarification! While this should be rare given our CI enforcement, I agree that it is better to be defensive here. I've added the try-catch ;)

A test triggering the validation error (422) when there is no try-catch can be seen here: https://github.com/r0qs/solidity/actions/runs/20467384783/job/58814376600?pr=14

r0qs · 2025-12-23T18:09:53Z

@heathdutton motivated me to improve this PR further. I've added error descriptions to our code style checker and pushed the bc25670 with intentional style errors for easier review of the behavior (the file will be removed before merge). That said, I think we should rethink about moving to clang-format as proposed in #2856 for more robust style checking.

github-actions · 2025-12-23T18:18:49Z

There was an error when running Code Style Check for commit 9e45e2b0eeb4a6a465878f3b2bed599536bd9c0a:

grep: warning: * at start of expression
grep: warning: * at start of expression
Coding style error:
[Use angle brackets for includes] libsolutil/TestStyleViolations.cpp:2:#include "libsolutil/Common.h"
[Missing space after keyword] libsolutil/TestStyleViolations.cpp:12:	if(true) {
[Opening brace on same line as if] libsolutil/TestStyleViolations.cpp:12:	if(true) {
[Missing space before opening brace in namespace] libsolutil/TestStyleViolations.cpp:6:namespace solidity::util {
[const should be on the right side of the type] libsolutil/TestStyleViolations.cpp:8:const int BAD_CONST = 42;
[Use std::move] libsolutil/TestStyleViolations.cpp:14:		auto x = move(badRef);
[Do not use 'using namespace std'] libsolutil/TestStyleViolations.cpp:4:using namespace std;

Please check that your changes are working as intended.

github-actions · 2025-12-23T18:18:49Z