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

[amotl]

Hi there,

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

On crate/crate-python#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 crate/crate-python#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 (github/codeql#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 github/codeql#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: crate/crate-python#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 🤞