What this is about
We have been steadily adding automated checks to this repository — phplint on 8.4/8.5,
php-cs-fixer, and more recently a PHPUnit suite that CI runs on every pull request. All of
those answer "does it parse, is it formatted, does the narrow part we can unit test still
behave". None of them read the change and ask whether it is correct.
There is a category of tooling that does, and one of them is free for projects like ours:
CodeRabbit. Their pricing page puts it plainly — "install
CodeRabbit on a public repository, and receive free reviews forever for public repositories".
Public repos get the Pro feature set, with no seat limit, so every contributor here is covered.
I am opening this as an issue rather than a pull request because it cannot be done from a
pull request. The GitHub App has to be installed on the SimpleMachines organisation by an
org owner. Nobody outside that group can set it up, so this needs a decision from someone who
can.
What it would actually add
Two things we do not have today.
A review on every PR. It posts a walkthrough of what changed, and inline comments on the
specific lines it has doubts about, each tagged with a category and a severity. Not a rubber
stamp — the comments are specific enough to act on or dismiss. Two examples of it working on
PHP, from The Events Calendar:
event-tickets#4385 — on
src/Tribe/RSVP.php, flagged Major: "Account for reusable attendees before the stock check.
Line 2829 loads reusable attendees after Lines 2803-2812 validate the full requested quantity.
If an attendee is already yes and stock is zero, a resubmission to no fails the stock check
before this code can release the attendee's stock." That is an ordering bug in the change,
described in terms of the code in front of it.event-tickets#4303 — a
large PR, where it found dead code left behind by a __serialize/__unserialize refactor,
a test that resets the current user to 0 instead of restoring it, and a reducer field with
no default. Mixed severities, all of them things a human reviewer would have had to spot by
reading carefully.
The example that prompted this was vuejs/core#15297,
where it pushed back on the test rather than the fix: "expect(code).toContain(`return`)
only proves that the render-function wrapper was generated. It can pass if code generation skips
the invalid node or emits an empty return." A test that passes for the wrong reason is exactly
the kind of thing that survives review here too.
Static analysis we have never run. It drives PHPStan, PHPMD and PHPCS over changed files,
plus Stylelint for the theme CSS, ESLint for our JS, YAMLlint and actionlint for the workflow
files, ShellCheck for shell, and secret scanning. Adding PHPStan to CI on its own would mean a
baseline file and an argument about the level; here it arrives scoped to the diff, which is the
only place we can act on it anyway.
The case for it here
We have 64 open pull requests and 135 open issues. Review bandwidth is the constraint, not
contribution. A bot that reads every PR the day it lands does not replace a maintainer's review,
but it does mean a PR that has been sitting for months has had something look at it, and that
the reviewer who eventually picks it up starts from a summary of what changed rather than from
scratch. It also gives contributors feedback in minutes instead of weeks, which is where we lose
people.
What it costs us
- Permissions. The app asks for read on actions, checks, discussions, members and metadata,
and read/write on code, commit statuses, issues and pull requests. The write scopes are what
let it post reviews and commit suggested fixes when someone accepts one. CodeRabbit states it
does not retain source code after review unless caching is enabled.
- Noise. It is a real risk and worth being honest about. It is also tunable: behaviour is
controlled by a .coderabbit.yaml file in the repository, so once an owner has installed the
app, every subsequent adjustment — tone, which paths to ignore, which linters to run, whether
it comments at all on translation-only PRs — is a normal pull request that we control and
review like any other.
- A settings detail worth getting right up front. Auto review is limited to the default
branch by default. Ours is release-3.0, so the normal flow is covered, but anything targeting
release-2.1 would be silently skipped unless we list it in .coderabbit.yaml. Draft PRs are
skipped by default too, which is probably what we want.
What it is not
It is not a replacement for human review, and it should not gate merges — no required check, no
blocking. It is a reviewer that is always available and never tired, and it is wrong often enough
that its comments are input, not verdicts. The proposal is only to install it and see, with the
understanding that uninstalling is a click if it turns out to be more noise than signal.
Ask
Would an org owner be willing to install it on SimpleMachines/SMF? If there is appetite for a
trial but not a blanket yes, it can be installed on this repository alone rather than the whole
organisation, and I am happy to send the initial .coderabbit.yaml as a PR so the configuration
is reviewed here before it does anything.
What this is about
We have been steadily adding automated checks to this repository —
phplinton 8.4/8.5,php-cs-fixer, and more recently a PHPUnit suite that CI runs on every pull request. All ofthose answer "does it parse, is it formatted, does the narrow part we can unit test still
behave". None of them read the change and ask whether it is correct.
There is a category of tooling that does, and one of them is free for projects like ours:
CodeRabbit. Their pricing page puts it plainly — "install
CodeRabbit on a public repository, and receive free reviews forever for public repositories".
Public repos get the Pro feature set, with no seat limit, so every contributor here is covered.
I am opening this as an issue rather than a pull request because it cannot be done from a
pull request. The GitHub App has to be installed on the
SimpleMachinesorganisation by anorg owner. Nobody outside that group can set it up, so this needs a decision from someone who
can.
What it would actually add
Two things we do not have today.
A review on every PR. It posts a walkthrough of what changed, and inline comments on the
specific lines it has doubts about, each tagged with a category and a severity. Not a rubber
stamp — the comments are specific enough to act on or dismiss. Two examples of it working on
PHP, from The Events Calendar:
event-tickets#4385— onsrc/Tribe/RSVP.php, flagged Major: "Account for reusable attendees before the stock check.Line 2829 loads reusable attendees after Lines 2803-2812 validate the full requested quantity.
If an attendee is already
yesand stock is zero, a resubmission tonofails the stock checkbefore this code can release the attendee's stock." That is an ordering bug in the change,
described in terms of the code in front of it.
event-tickets#4303— alarge PR, where it found dead code left behind by a
__serialize/__unserializerefactor,a test that resets the current user to
0instead of restoring it, and a reducer field withno default. Mixed severities, all of them things a human reviewer would have had to spot by
reading carefully.
The example that prompted this was
vuejs/core#15297,where it pushed back on the test rather than the fix: "
expect(code).toContain(`return`)only proves that the render-function wrapper was generated. It can pass if code generation skips
the invalid node or emits an empty
return." A test that passes for the wrong reason is exactlythe kind of thing that survives review here too.
Static analysis we have never run. It drives PHPStan, PHPMD and PHPCS over changed files,
plus Stylelint for the theme CSS, ESLint for our JS, YAMLlint and
actionlintfor the workflowfiles, ShellCheck for shell, and secret scanning. Adding PHPStan to CI on its own would mean a
baseline file and an argument about the level; here it arrives scoped to the diff, which is the
only place we can act on it anyway.
The case for it here
We have 64 open pull requests and 135 open issues. Review bandwidth is the constraint, not
contribution. A bot that reads every PR the day it lands does not replace a maintainer's review,
but it does mean a PR that has been sitting for months has had something look at it, and that
the reviewer who eventually picks it up starts from a summary of what changed rather than from
scratch. It also gives contributors feedback in minutes instead of weeks, which is where we lose
people.
What it costs us
and read/write on code, commit statuses, issues and pull requests. The write scopes are what
let it post reviews and commit suggested fixes when someone accepts one. CodeRabbit states it
does not retain source code after review unless caching is enabled.
controlled by a
.coderabbit.yamlfile in the repository, so once an owner has installed theapp, every subsequent adjustment — tone, which paths to ignore, which linters to run, whether
it comments at all on translation-only PRs — is a normal pull request that we control and
review like any other.
branch by default. Ours is
release-3.0, so the normal flow is covered, but anything targetingrelease-2.1would be silently skipped unless we list it in.coderabbit.yaml. Draft PRs areskipped by default too, which is probably what we want.
What it is not
It is not a replacement for human review, and it should not gate merges — no required check, no
blocking. It is a reviewer that is always available and never tired, and it is wrong often enough
that its comments are input, not verdicts. The proposal is only to install it and see, with the
understanding that uninstalling is a click if it turns out to be more noise than signal.
Ask
Would an org owner be willing to install it on
SimpleMachines/SMF? If there is appetite for atrial but not a blanket yes, it can be installed on this repository alone rather than the whole
organisation, and I am happy to send the initial
.coderabbit.yamlas a PR so the configurationis reviewed here before it does anything.