Capture of a robot comment:

There are a lot of details on the task to have SonarQube to report straight to Gerrit T217008 and an implementation at https://github.com/kostajh/sonarqubebot

Similar code could be used to consume checkstyle / linters log reports

With Robot Comments, you can also provide a proposed fix which the reviewer/author can accept by pressing a button in the Gerrit UI. This would be nice for simple lint issues. The problem is getting tools (eslint, stylelint, phpcs, etc) to generate the format needed for fix suggestion info.

sonarqubebot is probably helpful as an example but I think this should be a separate tool.

@hashar @Tgr @awight I was sitting down to look at Quibble to see how we could add support for the Gerrit reporting / fix format that @Tgr has added in https://gerrit.wikimedia.org/r/c/mediawiki/tools/codesniffer/+/647595

Currently Quibble knows about a composer-test stage and if that is set as a stage to run in CI, Quibble runs composer test.

I assume composer test is always going to include phpcs for extensions/skins.

How should we distribute the fix reporting code across core/extensions, mediawiki/tools/codesniffer, and quibble?

There's a few parts here (some manual instructions here):

1. Checking if phpcs failed, and if so re-running with --report='MediaWiki\Reports\GerritRobotComments'. Note that we should run phpcs the first time with the cache option enabled, if it isn't already set.
2. Preparing the POST body for Gerrit
3. POSTing the data

I assume the last step should be done via code in Quibble.

I guess the other two parts should probably also be done via Python code in Quibble, rather than attempting to add other entry points to composer.json (e.g. "composer phpcs-generate-fixes")

We could also make a report which inherits from the default one and outputs the POST body into a file as a side effect.

Note that GerritRobotComments disables caching in some cases (I copied the code from the Diff report, no idea when/if it is actually needed).

Change 647594 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/tools/codesniffer@master] Add Gerrit report format

https://gerrit.wikimedia.org/r/647594

Change 647595 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/tools/codesniffer@master] Add fix reporting to Gerrit robot comment reporter

https://gerrit.wikimedia.org/r/647595

Thoughts about this:

• Why does phpcs have to be run twice? Can we run in reporting mode the first time? Is there some performance penalty, etc.?
• As mentioned, ideally phpcs is run under composer. If there is a good reason to, we can run from Quibble however.
• Publishing should be done by a generic script, in a post-build step. This step would look for e.g. log/checkstyle-reports/*.xml and upload everything in that directory to gerrit, associated it with the current repo and patchset.

In T209149#7125536, @awight wrote:

Thoughts about this:

• Why does phpcs have to be run twice? Can we run in reporting mode the first time? Is there some performance penalty, etc.?

I don't think there's a way to run phpcs in "fix" mode and have it also report an error. We want phpcs to report an error so the CI build fails because we are not making the changes ourselves, just providing the robot comments with the suggested fixes. So we need it first to fail, and then a second time with the fix. If we pass the --cache flag to phpcs, then re-running to generate fixes will take a few seconds, at most.

• As mentioned, ideally phpcs is run under composer. If there is a good reason to, we can run from Quibble however.
• Publishing should be done by a generic script, in a post-build step. This step would look for e.g. log/checkstyle-reports/*.xml and upload everything in that directory to gerrit, associated it with the current repo and patchset.

Ack.

Change 703399 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[mediawiki/core@master] phpcs: Use cache flag

https://gerrit.wikimedia.org/r/703399

Change 703400 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[mediawiki/core@master] composer: Add gerrit-fix command

https://gerrit.wikimedia.org/r/703400

Change 703401 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[integration/quibble@master] gerrit: Generate fix comments for use with robot comments

https://gerrit.wikimedia.org/r/703401

Change 703405 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[integration/quibble@master] gerrit: Post the generated fix suggestions to sonarqubebot

https://gerrit.wikimedia.org/r/703405

Change 703430 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[labs/tools/sonarqubebot@master] Add support for Gerrit fix suggestions

https://gerrit.wikimedia.org/r/703430

I've posted a few (mostly untested) patches today, here's the summary (cc @Tgr, @hashar, @Jdforrester-WMF, @awight):

• phpcs: Use cache flag -- use --cache when running phpcs in composer.json.
• composer: Add gerrit-fix command -- re-uses the cached run from previous patch. Adds a gerrit-fix command to composer.json, which uses the GerritRobotComment report format that @Tgr created; that command dumps the report into cache/gerrit-fix-suggestions.json
• gerrit: Generate fix comments for use with robot comments -- now in Quibble, check if _run_composer_test() failed; if it did, run the new composer gerrit:fix command if it exists. Then re-raise the process error.
• gerrit: Post the generated fix suggestions to sonarqubebot -- Take the output file and issue a POST request to sonarqubebot, which we could probably rename to "codehealthbot" or something if we want to avoid mixing concerns :)
• Add support for Gerrit fix suggestions -- update sonarqubebot to listen for incoming POSTs from Quibble, construct a URL for the relevant Gerrit change, and post the fix suggestions JSON. This patch should probably also check if the request was already handled once, probably by calling Gerrit API to see if any robot comments already exist for the patch and if so exiting early. Otherwise if something in CI calls composer test-some more than once, we'd end up with duplicate fix suggestions (or maybe Gerrit would be clever enough to discard duplicates?). And I suppose we'd want to guard against abuse of this commenting vector, so maybe Quibble should use a secret in Jenkins and that secret should be stored in sonarqubebot's .env file on toolforge.

Output from the above patches (where possible anyway) can be seen here: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/703464

I made my phpcs violation edits in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/703464, then ran git review -x 703400 to download the composer gerrit-fix command, then I modified composer.json to point to my local checkout of mediawiki/tools/codesniffer with the GerritRobotComment.php class.

Then I ran:

GERRIT_ROBOT_ID=sonarqubebot GERRIT_ROBOT_RUN_ID=manualtest GERRIT_URL=https://gerrit.wikimedia.org/TBD PHPCS_ROOT_DIR=/Users/kostajh/src/mediawiki/w composer gerrit-fix -- index.php includes/Message/Converter.php

I started up sonarqubebot locally with php -S localhost:8888 public/index.php, then I ran curl -X POST -H 'accept: application/json' -H 'content-type: application/json' -d @cache/gerrit-fix-suggestions.json -H 'user-agent: quibble/gerrit-fix-suggestions' -H 'zuul-change: 703464' -H 'zuul-patchset: 1' -H 'zuul-project: mediawiki/core' http://localhost:8888 and the fix suggestions showed up in gerrit.

Change 703405 abandoned by Kosta Harlan:

[integration/quibble@master] gerrit: Post the generated fix suggestions to sonarqubebot

Reason:

squashed

https://gerrit.wikimedia.org/r/703405

Change 647594 merged by jenkins-bot:

[mediawiki/tools/codesniffer@master] Add Gerrit report format

https://gerrit.wikimedia.org/r/647594

Change 647595 merged by jenkins-bot:

[mediawiki/tools/codesniffer@master] Add fix reporting to Gerrit robot comment reporter

https://gerrit.wikimedia.org/r/647595

Change 703399 merged by jenkins-bot:

[mediawiki/core@master] phpcs: Use cache flag

https://gerrit.wikimedia.org/r/703399

Release-Engineering-Team, could one of you please add an environment variable secret in Jenkins for SONARQUBEBOT_API_KEY and send me the value privately, so that I can add it to the envfile on https://sonarqubebot.toolforge.org ? For context, please see https://gerrit.wikimedia.org/r/c/integration/quibble/+/703401/9/quibble/commands.py#426

In T209149#7297030, @kostajh wrote:

Release-Engineering-Team, could one of you please add an environment variable secret in Jenkins for SONARQUBEBOT_API_KEY and send me the value privately, so that I can add it to the envfile on https://sonarqubebot.toolforge.org ? For context, please see https://gerrit.wikimedia.org/r/c/integration/quibble/+/703401/9/quibble/commands.py#426

@dancy has volunteered to take care of this

Hi @kostajh. I'd like to help you wrap this up while hashar is out. I took a look at Jenkins and I see that there is already a SONAR_API_KEY credential established. Would that work for you or do you definitely need a new one for Sonarqube?

In T209149#7302747, @dancy wrote:

Hi @kostajh. I'd like to help you wrap this up while hashar is out. I took a look at Jenkins and I see that there is already a SONAR_API_KEY credential established. Would that work for you or do you definitely need a new one for Sonarqube?

Thanks @dancy. After comments on the existing set of patches, I'm exploring a different approach, to use a Toolforge bot (fixsuggesterbot) that looks at the event streams in Gerrit and listens for new patchsets on core/extensions/skins (like the "Welcome, new contributor" bot does) and then handles posting to Gerrit as a bot user, perhaps also with a -1 when there are fix suggestions that we know for sure would cause CI to fail. I think this will provide faster feedback on patches while also allowing Quibble to do less.

Anyway, @dancy, my new request is to ask if fixsuggesterbot could be added to stream events global group. Could you please help me with that?

I'll work with my team to get that arranged.

Apologies for the delay. fixsuggesterbot has been added to the stream-events Gerrit group.

In T209149#7303060, @kostajh wrote:

After comments on the existing set of patches, I'm exploring a different approach, to use a Toolforge bot (fixsuggesterbot) that looks at the event streams in Gerrit and listens for new patchsets on core/extensions/skins (like the "Welcome, new contributor" bot does) and then handles posting to Gerrit as a bot user, perhaps also with a -1 when there are fix suggestions that we know for sure would cause CI to fail. I think this will provide faster feedback on patches while also allowing Quibble to do less.

Thanks, moving the publishing step out of Quibble seems wise!

In T209149#7306324, @dancy wrote:

Apologies for the delay. fixsuggesterbot has been added to the stream-events Gerrit group.

🙏 thank you @dancy, confirmed that this is working.

GerritRobotComments seems to add an empty line whenever something is removed:

Seems like Gerrit is interpreting line numbers differently from how I expected and the range end numbers should be one larger.

(The examples are from c717122. The robot comment is here (apparently you can inspect it via refs/changes/.../robot-comments, which is nice).

In T209149#7332532, @Tgr wrote:

GerritRobotComments seems to add an empty line whenever something is removed:

When it is trying to remove an empty line, it ends up as a no-op:

Seems like Gerrit is interpreting line numbers differently from how I expected and the range end numbers should be one larger.

(The examples are from c717122. The robot comment is here (apparently you can inspect it via refs/changes/.../robot-comments, which is nice).

I've hopefully fixed that in https://gitlab.wikimedia.org/KostaHarlan/fix-suggestions/-/merge_requests/5.

Update from previous comments. I ended up not using Quibble but instead implementing this as a bot running on Toolforge, listening to patchset-created stream events from Gerrit. The documentation is here https://wikitech.wikimedia.org/wiki/Tool:Fix_Suggester_Bot and I'll be adding tasks in Fix-Suggester-Bot. (Of course if folks would prefer a different approach, please say so.) For now it's just providing suggestion for patches to GrowthExperiments but once some issues are ironed out we could roll this out to more extensions/skins & mediawiki/core as well as other PHP libraries. I think adding support for fixes to JavaScript code should be doable as well but not sure how to best support other languages used in the ecosystem with this setup.

Change 703400 abandoned by Kosta Harlan:

[mediawiki/core@master] composer: Add gerrit-fix command

Reason:

https://phabricator.wikimedia.org/T209149#7333699

https://gerrit.wikimedia.org/r/703400

Change 703401 abandoned by Kosta Harlan:

[integration/quibble@master] gerrit: Generate and submit fix suggestions

Reason:

https://phabricator.wikimedia.org/T209149#7333699

https://gerrit.wikimedia.org/r/703401

Change 703430 abandoned by Kosta Harlan:

[labs/tools/sonarqubebot@master] Add support for Gerrit fix suggestions

Reason:

https://phabricator.wikimedia.org/T209149#7333699

https://gerrit.wikimedia.org/r/703430

In T209149#7333699, @kostajh wrote:

I think adding support for fixes to JavaScript code should be doable as well but not sure how to best support other languages used in the ecosystem with this setup.

I wonder if it would be possible to run the linters via normal CI and have them store the output as a public artifact file that the bot can then download.

The part of GerritRobotComments that converts a diff to a robot comment could be run by the bot centrally, so the various linter plugins that would have to be written would only need to output 1) a JSON file with filename -> line number -> error message information, and 2) a diff file with the fixes.

In T209149#7335003, @Tgr wrote:

In T209149#7333699, @kostajh wrote:

I think adding support for fixes to JavaScript code should be doable as well but not sure how to best support other languages used in the ecosystem with this setup.

I wonder if it would be possible to run the linters via normal CI and have them store the output as a public artifact file that the bot can then download.

I think we could work on that in T331996: Quibble: Generate structured output for command success/failures.

Gerrit is going to remove the robot comments entirely: https://groups.google.com/g/repo-discuss/c/fIgEGcB2IAs

In short the rationale is two fold:

• comments made to Gerrit changes end up being stored infinitely in the git repositories which will make them grow and slow operations.
• bot usually keep a state of the results which can then be fetched via the JavaScript Checks API

Thus linters/testers should output some artifacts result file which is stored by Jenkins, then some home made JavaScript code would be added to fetch the results from Jenkins and display them in Gerrit. I think the devil is figuring out configurations for each of them (eslint, phpcs, phpunit, wdio etc) then either always make them write the report (even when run locally) or figure out logic so the configuration only applies when in CI context (eg CI environment variable is set).

Gerrit 3.10 introduces a more structured way for suggestions: https://www.gerritcodereview.com/3.10.html#allow-fixes-in-human-comments-via-rest-api

Robot Comments have been officially deprecated for some time and the checks API framework is recommended since Gerrit 3.6. This is great as it provides a way of greatly reducing the size of the repository. However, unlike fixes suggested in Robot-Comments, Human Suggested fixes could not be applied programmatically, until now.

Gerrit 3.10 introduces a way of suggesting fixes that could then be programmatically applied with the the Apply Stored Fixes endpoint. This is done by adding an extra field fix_suggestions in CommentInfo that will be stored separately on NoteDB.

(...)

The experiment can be enabled in gerrit.config like so

[experiments]
          enabled = GerritBackendFeature__allow_fix_suggestions_in_comments

(In 3.9 the bot would instead have to make a normal comment and wrap the code snippet in ```...```. Not sure what the advantages / disadvantages of the new method are.)

In T209149#9175757, @kostajh wrote:

https://wikitech.wikimedia.org/wiki/Tool:Fix_Suggester_Bot got some of the way there, but I don't have the time or interest to continue it.

@Tgr and I talked at the Hackathon in Tallinn about picking this up again. It should not be too difficult to hammer it into shape. And especially now that the suggestion interface is more polished, I think this would be a useful change to implement. If anyone's interested to collaborate (cc @Pppery) please let me or @Tgr know.