Blog

Notes on the .Net ecosystem, Angular, Software Architecture, Machine Learning and CyberSecurity

All posts
Commenting on the pull request and holding the merge
  • NeuralGuard
  • GitHub Actions
  • CI

Commenting on the pull request and holding the merge

The previous article left the analysis running at every pull request. The result sits in a file inside a virtual machine that is destroyed shortly after.

Two things are missing: showing this to people, and making it count.

What is worth writing in a comment

Before the code, worth thinking about what the comment needs to have, because that decides whether it gets read.

An automated tool comment competes for the attention of somebody who wants to finish their task. If it is long, generic or repetitive, the person learns to scroll past it.

Four things make the difference.

Where the problem is. File and line, so the person does not have to go looking.

What exactly was flagged. The snippet of code, right there in the comment. Nobody should have to open another tab.

What the verdict is, separating confirmed from dismissed. That is what makes this tool different from an ordinary scanner. Showing the dismissals alongside has an important effect: the person sees that the tool thought before accusing.

Why. One or two sentences explaining. A warning with no explanation forces the person to either trust it or ignore it, and they will choose to ignore it.

NeuralGuard's report comes out like this:

## NeuralGuard

Analyzed 12 changed snippets. The classifier raised 3 suspicions,
and 1 was confirmed by the review.

### Insecure Deserialization in src/ReportsController.cs (line 31)

Classifier confidence: 99%

...code of the snippet...

**Confirmed**: the payload comes from the request and BinaryFormatter
rebuilds whatever type the data names, so whoever controls that
payload controls which class gets instantiated.

### Insecure Deserialization in src/ReportsController.cs (line 58)

Classifier confidence: 92%

...code of the snippet...

**False positive**: the type is fixed at compile time by typeof(Report),
so the data cannot choose which class is built.

The summary line at the top exists for a reason: it shows the work that was avoided. Three suspicions, one confirmed, means two people were not interrupted for nothing.

Publishing the comment

To write on the pull request you need to talk to the GitHub API. You could assemble the request by hand, but the virtual machine already comes with a tool that does it: the GitHub CLI, the gh command.

Three details matter in that step.

Running even after a failure. GitHub Actions skips the remaining steps as soon as one step goes wrong. And the case where the analysis finds a vulnerability is precisely a "went wrong" case from the pipeline's point of view. Without telling the step to always run, the tool would go silent exactly when it has something to say.

Checking the file exists. This handles the case where the analysis never even ran, for example if the model download failed. Then there is no report, and the step ends quietly instead of erroring on a missing file.

Authenticating. The gh command uses the workflow's own token, which already has permission to write on pull requests because that was declared in the permissions block.

Avoiding a pile of repeated comments

A naive version publishes a new comment on every run. In a pull request that receives five pushes, five comments appear, and the first four are already out of date.

That is annoying in a specific way: the person has to work out which of the five is the current one, and the pull request conversation gets buried under robot text.

The fix is for the tool to find the comment it left before and edit that one, instead of creating another.

For that it needs to recognize its own comment. The trick is leaving an invisible mark in the text. Markdown accepts HTML comments, and they do not show up on screen:

public const string Marker = "<!-- neuralguard -->";

That line is the first one of the report. Whoever reads the pull request sees nothing. Whoever reads the raw text of the comment finds the signature.

With the mark in place, the step searches before publishing. It asks the API for the list of comments on the pull request, keeps only the ones containing the mark, and takes the identifier of the last one. If it finds none, the variable comes back empty.

Paging matters here: the GitHub API returns comments in pages, and without asking for all of them, a pull request with a long discussion could hide the old comment on a later page, and the tool would create a duplicate thinking it was the first.

Then it is a matter of deciding between editing and creating. To edit a comment you have to send the text inside a JSON, and the report has line breaks, quotes and code blocks. Assembling that JSON by hand would break on the first snippet with quotes.

The jq command solves it in one line: it reads the whole file as a single piece of text and writes a JSON with that text in the body field, escaping everything that needs escaping.

The result is that the pull request always has one NeuralGuard comment, and it reflects the last push.

Making the result count

Commenting is informing. Blocking is something else.

The final step reads the exit code that was stored earlier and decides: if the detector answered anything other than zero, this step fails on purpose. A step that fails makes the job fail, and a job that fails shows up as a red check on the pull request.

The order matters and is not accidental: comment first, fail afterwards. The person receives the explanation together with the block, and not a red check with no context.

A red check still does not prevent a merge

Here is the part that tends to go unnoticed, and without it none of this holds anything.

A failing check shows up on the pull request, but by default the merge button stays clickable. GitHub shows the red and lets the person carry on.

For the check to become required, you have to configure that in the branch protection rules, in the repository settings. You choose the branch (usually main) and mark the check as required.

From then on, the merge is unavailable while that check does not pass.

Two things are worth knowing when switching this on. The first is that the check only appears in the list of options after it has run at least once, so you need to open a pull request before configuring it. The second is that repository administrators can usually override the rule, which is useful in an emergency and is good for the team to know exists.

The parameter that switches blocking off

Not every team wants to start out blocking, and the action has the block parameter for that.

With blocking off, the detector keeps analyzing and commenting, but answers zero even when it confirms something. The check stays green and nobody is stopped.

That mode has a concrete use: running for a few weeks just observing. The team sees the comments, judges whether the verdicts make sense in that project's real code, and adjusts the threshold if needed. Then they switch blocking on with some confidence that it will not get in the way.

Switching on a new tool that already blocks everyone's work is the fastest way to have it removed on the first bad Friday.

What is already standing

Adding up the engineering articles: the detector became a command, the command became an action, the action runs at every pull request, comments the result and can hold the merge.

The next article closes the series by putting everything to run for real against the example repository, including the false positive showing up and being dismissed.

Sources