Copilot PR reviews

[rugk]

I don't know how you all (especially other maintainers) feel about this, but for a test I have enabled Copilot reviews here: https://github.com/PrivateBin/PrivateBin/settings/rules/9263756 (docs: https://docs.github.com/en/copilot/how-tos/use-copilot-agents/request-a-code-review/use-code-review)

It's for the default branch only and should only trigger in such cases.

Let's use this discussion to see whether it's worth it. I know all the problematic aspects, but after all, if it is a tool that saves work and increases the code quality (and security, ideally!) by providing good code reviews, well… then IMHO we can give it a try.

@PrivateBin/maintainers

[elrido]

I'm happy to give it a spin.

As long as it doesn't block us from doing progress, I'm all for hearing more feedback and if an AI agent is highlighting a certain line in the code, it may not be wrong, but at least not follow conventional patterns that are easily understood by other (human) collaborators, and so may be worthwhile of reviewing.

My own experience with AI code suggests that we should review it's claims at least as carefully as those of very junior human collaborator. We need to consider that it may very stubbornly insist on there being something (an issue in our code or an API or language feature) that it has made up or doesn't apply to our use case (i.e. I've been suggested nodejs-only functions that wont work in the browser). The AI seems to struggle with our project using browser JS, but running the unit tests within a nodejs context, so the test code can use nodejs-isms, but the main logic can't. And then there is legacy.js for feature detection, where we can't use any modern JS for it to still do it's job (un-hide a warning) in a legacy browser. :-)

[cryptomilk]

FYI: We are using sourcery.ai for libssh. Nice are the rules you can add. We do it for certain coding styles we want.

[rugk]

it's claims at least as carefully as those of very junior human collaborator.

Absolutely. After al, we live in a world nowadays, where every PR some contributor mades can be made by AI (at least partially), too, so proper human review is critical here. We can anyway not use the Copilot agent (as it's a paid feature) feature, so it's only about PR reviews by the AI, which is absolutely fine, IMHO.

The AI seems to struggle with our project using browser JS, but running the unit tests within a nodejs context, so the test code can use nodejs-isms, but the main logic can't. And then there is legacy.js for feature detection, where we can't use any modern JS for it to still do it's job (un-hide a warning) in a legacy browser. :-)

Yeah for this to improve we can add some agent Markdown to explain these facts to "the AI": https://docs.github.com/copilot/how-tos/configure-custom-instructions/add-repository-instructions

[cryptomilk]

We have this in use: https://gitlab.com/libssh/libssh-mirror/-/merge_requests/647/diffs?commit_id=20db339f084552be2edc6e53f1ff5d96e10f27ee

[rugk]

Gave it a shot and here you go: #1698

[rugk]

Okay, also PR reviews need billing?

I looked why I cannot assign copilot to a PR or that in general does not trigger any reviews and actually it seems to be the case that, it also needs billing information to enable this at org level?

Found the answer here

[elrido]

Well, I guess those models are rather expensive to run. But I hadn't quite understood that from all those copilot news emails we have been getting recently. :-(

Anyhow, I assume that instruction document is also useful for prompting local or third-party LLMs.