Allow PR review just with suggestions without requesting to change or approving

[Alexolut]

With option like format-review I can enable posting suggestion by bot what and how should be changed, i.e. diff to be applied to satisfy the review. At the same time this option adds review result in terms of "needs work/approved". More over for dismissing the results of previous "needs work" additional permissions should be set for a bot.

I just want to see the suggestions (like normal comments) without "approve" or "request changes".

[2bndy5]

Interesting idea. Technically speaking, we could change the posted review to be a COMMENT (GitHub REST API doc ref - see event parameter).

I think this could be controlled via a new input option with a boolean value. Off the top of my head, I think the name passive-reviews would be fitting. The default value false would incite the current behavior, but a true value would post all reviews as comments (not approving nor demanding changes).

[2bndy5]

[...] additional permissions should be set for a bot.

This cannot be avoided when posting reviews as comments too. The REST API endpoint that Github offers (to post PR reviews) requires extra permission to be set. There is a security concern about posting comments. To be clear, we cannot work around the required permission as that is mandated by github.

FWIW, posting thread comments on a PR (not as a PR review) poses the same permission obstacle. Posting comments in a PR that originates from a fork is practically impossible.

[Alexolut]

I'm not very familiar with permission restrictions on GH, but I can day that I faced with the situation that posting comments with format-review works fine, but it is not possible to dismiss the previously set "changes requested" set by bot. So merging such PR became impossible even after addressing the issue with code formatting. Hence from that point of view permissions needed to post comments are differ from that to dissmiss last PR review results.

[2bndy5]

You should be able to dismiss an entire review if your account has the permission to do so for the repo in question.

I wrote a doc about some of the PR review feature's caveats. Maybe that could help in some way.

impossible even after addressing the issue with code formatting

If the bot did not dismiss the outdated review, then I would suggest you look at the CI triggers (the on: [...] part of the yaml). If the CI only triggers cpp-linter-action on a PR open event, then any PR sync events will not be triggered. A PR sync event is needed for cpp-linter-action to:

1. understand that the event is specific to a PR.
2. fetch a list of PR reviews and dismiss any outdated review that cpp-linter-action had previously posted.

As for the comments posted within the diff, GH allows these diff comments to be deleted or collapsed (marked as "resolved") provided the user has the right permission.

[Alexolut]

In my case bot is trying to dismiss but ends with an error:

ERROR:CPP Linter:response returned 403 from PUT
https://api.github.com/.../dismissals
with message: {"message":"Branch protections do not permit dismissing this review","documentation_url":"
https://docs.github.com/rest/pulls/reviews#dismiss-a-review-for-a-pull-request
"}

I can workaround this by disabling "Restrict who can dismiss pull request reviews" option in branch protection rules, but this is not what I would like to change.

[2bndy5]

Ah, I forgot about the branch protection rules... Well, posting reviews as a comment will still yield the same dismissal problem.

Why each PR event posts a new review

It would be a nightmare to track which comments are outdated and which comments need updating/deleting. And thread comments can be collapsed when resolved, but there is no way to do that via GitHub REST API. Furthermore, altering an existing review via the REST API is not intuitive at all.

[Alexolut]

Unfortunately for some reason on 2.12 the following options don't lead to post comment with diff needed to apply to satisfy code formatting.

          step-summary: true
          lines-changed-only: true
          format-review: true
          passive-reviews: true
          thread-comments: update

I see only summary comment on conversation tag like this without diff:

Additionally on action summary I see annotation like this:

File <filename> does not conform to Custom style guidelines. (lines 19, 48)

But I expecting to see comment with actual diff (clang-format suggestions) as stated for format-review option.

[2bndy5]

1. thread-comments have a limited capacity. Github disallows posting comments that are larger than 65535 bytes. So, we have to omit feedback that won't fit within that constraint.
2. File <filename> does not conform to Custom style guidelines. (lines 19, 48) is from our file-annotations feature (enabled by default). It is not possible to post multiple lines of text (like a fenced code block) when creating file annotations.
3. the format-review feature is for posting PR reviews which is slightly different from thread-comments and file-annotations. For more detail about what to expect from that feature, see our doc about PR review caveats.

[Alexolut]

Sorry, I completly forget that it doesn't work for draft PR. It works as expected after I marked PR ready to review. IDK but maybe it would be good to allow diff suggestions on draft PR as well regulated by some additional option.

[2bndy5]

By definition a drafted PR is "not ready for review". So, no it isn't a good idea. Also, I explicitly prevent posting reviews on closed or drafted PRs to avoid thread pollution/noise.

[Alexolut]

All of your thoughts are correct, but is it OK that we don't have a way to get a diff from cpp-linter during PR draft stage? From my point of view diff is important information how the formatting issues can be addressed by PR's author before real users (not bots) start reviewing PR.

[2bndy5]

I would argue that cpp-linter isn't the only way to use clang-format and clang-tidy. In fact, cpp-linter's role has always been one of review only. Our suggestion snippets is an experimental frontier for us, but I'm struggling to keep the idea not intrusive (thread pollution/noise). The review implementation we have may also be subject to change when seeking to satisfy cpp-linter/cpp-linter#82.

The bot vs user review argument doesn't hold much weight since repo admins can specify a custom PAT (personal access token) instead of the inherent github.token (making posted comments/reviews originate from whatever account owns the custom PAT -- which could also be a custom bot account).

If you want to fix formatting errors in an automated way during the draft stage of a PR, you could use pre-commit. We have cpp-linter pre-commit hooks that doesn't use the same cpp-linter python package, but it requires the version of clang-format/tidy be installed separately & added to PATH env var. There's also a clang-format pre-commit hook. Either of those hooks don't support only scanning line changes. Either way, pre-commit hooks could be enforced locally (thus the name) or in CI (if any contributors don't want to locally use git hooks).

---

TBH, it wouldn't be hard to implement a special env var that allows reviews of a drafted PR. But it would incur tech debt to maintain such functionality. I just don't see the trade off worthy enough.