Conversation opened by @github-code-scanning bot does not resolve/collapse properly

[amotl]

Hi there,

we recently reported https://github.com/github/codeql/issues/11407 and https://github.com/github/codeql/issues/11408, and now are trying to properly dismiss admonitions reported by @github-code-scanning bot.

On https://github.com/crate/crate-python/pull/474, we are observing a situation where we tried to Resolve the conversation started by @github-code-scanning, using the usual GitHub UI feature, after dismissing the Check notice as "false positive" beforehand, see https://github.com/crate/crate-python/security/code-scanning/55.

However, even if the conversation has apparently been flagged as resolved, the UI component does not collapse properly, even after reloading the page.

It would be sweet if you could look into this issue, or advise us how to properly handle the situation.

With kind regards, Andreas.

P.S.: Maybe related: #986, #1223

[vilacides]

Uneducated guess: this sounds a lot like https://github.com/github/code-scanning/issues/5262 so tagging @anaarmas to have a quick look when she comes in on Monday to prove me wrong 🙇‍♀️

[anaarmas]

Hi @amotl 👋 This is in fact the intended behaviour: since the alert was manually dismissed and not fixed, we keep it open so reviewers of the PR have a chance to weigh in on the decision to dismiss the alert - especially since that action also prevents the alert from being reported at all (in this or other branches) in the future. We are very grateful for the feedback, though! 🙇‍♀️ I can see how it can be a bit surprising that the alert stays expanded even after manually dismissing it. We will take this into account as we continue to refine the experience for this feature! 😉

[amotl]

Hi @anaarmas,

thank you for your response, we appreciate it very much. First things first: Kudos to you and your colleagues for working on the excellent features supported by the LGTM/CodeQL integration/migration.

We hear you on the rationale of designing the dismissal feature like that, but yes, a few details surprised us, and it would be sweet if you could take our feedback into your refinement process.

Let me show you another example at https://github.com/crate/crate-python/pull/481, where three notices are displayed. We would love to collapse them, because we all agree that they are false positives discovered by the scanning engine (https://github.com/github/codeql/issues/11407), and they just distract the reviewing process, thus decreasing the user experience.

It may not be your department, but improving this detail together with https://github.com/github/codeql/issues/11427 would be so awesome. I think both features would be crucial for the new CodeQL integration you are working on.

Thanks for listening and with kind regards, Andreas.

[amotl]

Hi again,

we have another PR which demonstrates how distractive the current implementation of CodeQL admonitions is: https://github.com/crate/crate-python/pull/498. The admonitions dominate the whole page and there is no chance to acknowledge or hide individual admonition sections.

It also looks like there is a bug that we can not dismiss those open items, which are actually false positives, in a second round of review, where there have been some rebasing and commits beforehand, and where also some admonition items have been dismissed, but re-opened.

With kind regards, Andreas.

[amotl]

Hi again,

at [1], there is another spot which I would like to draw your attention to. The first few admonitions are actually items which already have been fixed, contrary to the other ones which only have been dismissed.

With kind regards, Andreas.

[1] https://github.com/crate/crate-python/pull/485/files#diff-776e4a4f474895d9e175985c87b02dac29399974a0e3af684c9d4ae9f3f04b70

[amotl]

Hi,

another spot at [2] has been fixed with a subsequent push, but the admonition is still there. Following "Show more details" yields a 404 at [3].

With kind regards, Andreas.

[2] https://github.com/crate/crate-python/pull/488/files/b4582208817a671a0a68607077d9e57d26cbf210#diff-8d4d5446aad4c66a2236f9c83edd825550a1565f3190e3adb01ed761e457fa02 [3] https://github.com/crate/crate-python/security/code-scanning/84

[adityasharad]

Hi Andreas, thanks for the detailed feedback! We have limited coverage over this holiday period, but will take a look at your examples and get back to you in the new year.

[anaarmas]

Hi @amotl! Thanks for providing those examples 🙇‍♀️ they're priceless for debugging purposes! It looks to me like there's something that's not working correctly as a result of the force pushes. I don't think I'll be able to look into it this week but next week should be doable 😉

[anaarmas]

I started looking into this and even though I'm not finished with all the things I want to poke, I noticed that when you updated the workflow file you didn't teach the code scanning category param about the new matrix dimension - I think you'll need to make that category: "/language:${{ matrix.language }}/sqla-version:${{ matrix.sqla-version }}". This should makes things better 🤞

[amotl]

Thank you very much for discovering this on our CI configuration 🌻, I am just fixing it with https://github.com/crate/crate-python/pull/505 based on your suggestions.

Apologies for not reading the documentation properly on this detail. Many of the misleading admonitions from code scanning obviously have been caused by this misconfiguration, sorry for the noise about those.

[anaarmas]

Hi again @amotl! Last week I was finally able to also get to the bottom of the 404 problem you mentioned and can now confirm that it was also a result of the temporary codeql workflow misconfiguration, so you shouldn't be seeing any more of those! If you do, please let us now so we can investigate ;)

[amotl]

Hi again,

I think all the teething woes are gone with the @github-code-scanning bot improvements you have been bringing in at the beginning of the year. Thank you again for assisting us on a misconfiguration of one of our repositories. I also hope you are doing well in general.

Other than this, I would like to get back to the original matter if this discussion, as we just encountered another occasion where we would like to dearly resolve & collapse a conversation about a false positive reported by CodeQL.

• https://github.com/crate/crate-python/pull/555#pullrequestreview-1451144547

With kind regards, Andreas.

[anaarmas]

Hi @amotl 👋 Thanks a lot for taking the time to continue sending us feedback 🙇 I'm happy to inform you that we've decided to change the collapsing behaviour of dismissed alerts on PRs to address the pain point you describe. We now have to prioritise the work, but I will let you know when it's landed! Regards, Ana

[anaarmas]

Hi @amotl! I'm bringing you an early Christmas 🎄 present! Ok maybe I'm overhyping this... 😅 🤣 Today I deployed the changes to adjust the collapsing behaviour of dismissed code scanning alerts on PRs. I hope it'll make the experience better for you moving forward 🤞 please let us know if you spot any creases to iron - or close this issue if it turn out to be just perfect™! 😉 Cheers, Ana