PR commented on multiple times

[jhrozek]

Describe the issue

We sometimes add multiple comments that tell that minder has analysed a PR. We should only be adding one and there already should be a safeguard looking for a magic comment in the previous PR but that is apparently not working correctly.

To Reproduce

Unsure yet, but manifested at #1790 (review)

What version are you using?

today's master (stacklok staging)

[gregfurman]

Going to try and recreate this locally. Perhaps it has something to do with the force pushes?
cc @eleftherias

[eleftherias]

Thanks @gregfurman! Feel free to reach out if you have any questions.

[gregfurman]

Think I found the issue. The GitHub API returns reviews from earliest to latest. We iterate through all review objects and when we encounter the first whose body contains the reviewBodyMagicComment in the prefix, the loop breaks and minderReview is set to the first review encountered in the list which never has a corresponding SHA equal to the current HEAD SHA in the PR.

So if HEAD of the PR was 5e26642d64aec7bae43167d8ad20cb090d33141a (see last SHA below) then of all the minder reviews found in PR 1790, the loop would always break and return on the first SHA 94ea8a313784de6d65dccbe0fc815335bf417633.

"2023-12-16T18:25:52Z 94ea8a313784de6d65dccbe0fc815335bf417633"
"2023-12-16T18:26:57Z 0105b620f68582a3e20da211c20d72798ef40d77"
"2023-12-16T18:30:14Z 3410b66b103cbf1ed777462c7ebba8b249b74dce"
"2023-12-16T18:36:55Z 277a82af28ffdbe3ab3c1209e27ddcc0774d3da7"
"2023-12-16T18:41:37Z 5e26642d64aec7bae43167d8ad20cb090d33141a"
"2023-12-16T18:42:17Z 5e26642d64aec7bae43167d8ad20cb090d33141a"
"2023-12-16T18:42:55Z 5e26642d64aec7bae43167d8ad20cb090d33141a"
"2023-12-16T18:43:32Z 5e26642d64aec7bae43167d8ad20cb090d33141a"

See below snippet showing list of reviews that would return early:
https://github.com/stacklok/minder/blob/f126d697a6fc1f00f8bc1c2f88c9036dd2264eb6/internal/engine/eval/vulncheck/review.go#L308-L323

See Line 264 in below code snippet which would always be false after the first minder review was made thus the "previois review" is never kept and we always submit a new one:
https://github.com/stacklok/minder/blob/f126d697a6fc1f00f8bc1c2f88c9036dd2264eb6/internal/engine/eval/vulncheck/review.go#L258-L290

Going to write some tests to confirm this is the case and make a PR tomorrow sometime. The solution probably just requires us to iterate through reviews in reverse order from how the API currently returns them (so latest to earliest).

---

Full Results of GitHub API Calls

GitHub API Reviews for PR 1790

$ curl -sL \
  -H "Accept: application/vnd.github+json" \
  -H "Authorization: Bearer $GITHUB_ACCESS_TOKEN" \
  -H "X-GitHub-Api-Version: 2022-11-28" https://api.github.com/repos/stacklok/minder/pulls/1790/reviews | jq '.[] | "\(.submitted_at) \(.body) \(.commit_id)"' 

# RESULT in format review_time, review_body, state, commit_SHA
curl -sL \
  -H "Accept: application/vnd.github+json" \
  -H "Authorization: Bearer $GITHUB_ACCESS_TOKEN" \
  -H "X-GitHub-Api-Version: 2022-11-28" https://api.github.com/repos/stacklok/minder/pulls/1790/reviews | jq '.[] | "\(.submitted_at) \(.body) \(.state) \(.commit_id)"'
"2023-12-01T06:30:29Z  COMMENTED 219bb29de2dba0c02d5445192bc2698b21a4080c"
"2023-12-01T06:54:40Z  COMMENTED 219bb29de2dba0c02d5445192bc2698b21a4080c"
"2023-12-01T08:58:18Z  COMMENTED 219bb29de2dba0c02d5445192bc2698b21a4080c"
"2023-12-01T13:58:36Z  COMMENTED 219bb29de2dba0c02d5445192bc2698b21a4080c"
"2023-12-01T14:04:42Z  COMMENTED 219bb29de2dba0c02d5445192bc2698b21a4080c"
"2023-12-01T14:19:59Z  COMMENTED 219bb29de2dba0c02d5445192bc2698b21a4080c"
"2023-12-01T14:55:12Z  COMMENTED 219bb29de2dba0c02d5445192bc2698b21a4080c"
"2023-12-04T23:11:32Z  COMMENTED 219bb29de2dba0c02d5445192bc2698b21a4080c"
"2023-12-08T10:59:26Z Let's rename this directory to `authz` to denote it doesn't have to do with authentication.\r\n\r\nCan you add the license headers to the new files?\r\n\r\nOtherwise I think this is good to go! Thanks for researching this! CHANGES_REQUESTED a683906d8e6713b578c6ade5128cda460b523321"
"2023-12-16T18:25:52Z <!-- minder: pr-review-body -->\n\n\nMinder analyzed this PR and found no vulnerable dependencies.\n COMMENTED 94ea8a313784de6d65dccbe0fc815335bf417633"
"2023-12-16T18:26:57Z <!-- minder: pr-review-body -->\n\n\nMinder analyzed this PR and found no vulnerable dependencies.\n COMMENTED 0105b620f68582a3e20da211c20d72798ef40d77"
"2023-12-16T18:30:14Z <!-- minder: pr-review-body -->\n\n\nMinder analyzed this PR and found no vulnerable dependencies.\n COMMENTED 3410b66b103cbf1ed777462c7ebba8b249b74dce"
"2023-12-16T18:36:55Z <!-- minder: pr-review-body -->\n\n\nMinder analyzed this PR and found no vulnerable dependencies.\n COMMENTED 277a82af28ffdbe3ab3c1209e27ddcc0774d3da7"
"2023-12-16T18:41:37Z <!-- minder: pr-review-body -->\n\n\nMinder analyzed this PR and found no vulnerable dependencies.\n COMMENTED 5e26642d64aec7bae43167d8ad20cb090d33141a"
"2023-12-16T18:42:17Z <!-- minder: pr-review-body -->\n\n\nMinder analyzed this PR and found no vulnerable dependencies.\n COMMENTED 5e26642d64aec7bae43167d8ad20cb090d33141a"
"2023-12-16T18:42:55Z <!-- minder: pr-review-body -->\n\n\nMinder analyzed this PR and found no vulnerable dependencies.\n COMMENTED 5e26642d64aec7bae43167d8ad20cb090d33141a"
"2023-12-16T18:43:32Z <!-- minder: pr-review-body -->\n\n\nMinder analyzed this PR and found no vulnerable dependencies.\n COMMENTED 5e26642d64aec7bae43167d8ad20cb090d33141a"
"2023-12-16T18:47:28Z  COMMENTED 219bb29de2dba0c02d5445192bc2698b21a4080c"
"2023-12-18T06:23:15Z I really like how the model ended up. Looks quite understandable. COMMENTED 5e26642d64aec7bae43167d8ad20cb090d33141a"
"2023-12-18T19:10:31Z  COMMENTED 5e26642d64aec7bae43167d8ad20cb090d33141a"
"2023-12-18T19:11:12Z  COMMENTED 5e26642d64aec7bae43167d8ad20cb090d33141a"
"2023-12-18T19:11:34Z  COMMENTED 5e26642d64aec7bae43167d8ad20cb090d33141a"
"2023-12-18T19:18:51Z  COMMENTED 5e26642d64aec7bae43167d8ad20cb090d33141a"
"2023-12-18T19:32:11Z  COMMENTED 5e26642d64aec7bae43167d8ad20cb090d33141a"
"2023-12-18T19:32:56Z  COMMENTED 5e26642d64aec7bae43167d8ad20cb090d33141a"
"2023-12-18T19:35:33Z  COMMENTED 5e26642d64aec7bae43167d8ad20cb090d33141a"
"2023-12-18T19:51:19Z  COMMENTED 5e26642d64aec7bae43167d8ad20cb090d33141a"
"2023-12-18T19:57:55Z  COMMENTED 06edca97e62dc8c76ac27803a77d994bb2b568bf"
"2023-12-18T21:22:29Z  COMMENTED e45f39e2364f5800df90deb0d9916afb9b23dc45"
"2023-12-18T21:28:00Z  COMMENTED e45f39e2364f5800df90deb0d9916afb9b23dc45"

GitHub API Commits for PR 1790

$ curl -sL \
  -H "Accept: application/vnd.github+json" \
  -H "Authorization: Bearer $GITHUB_ACCESS_TOKEN" \
  -H "X-GitHub-Api-Version: 2022-11-28" https://api.github.com/repos/stacklok/minder/pulls/1790/commits | jq '.[] | "\(.commit.author.date) \(.sha)"'

# RESULT in format commit time, commit SHA
"2023-12-01T06:23:05Z 219bb29de2dba0c02d5445192bc2698b21a4080c"
"2023-12-04T23:10:46Z 598c0f4256007f51723afd7fc0ad25af7e53e710"
"2023-12-04T23:27:23Z a683906d8e6713b578c6ade5128cda460b523321"
"2023-12-16T17:27:24Z 07829fb885d1915b5f057f6b1d6a4e9a4a3d4413"
"2023-12-16T18:25:23Z 5e26642d64aec7bae43167d8ad20cb090d33141a"
"2023-12-18T19:30:07Z c260c6763e49a4b3d26f3ea633d5c5cd38221bcb"
"2023-12-18T19:53:04Z 079138b8018df8d88e33bf5c7cd2ad3b3a8f5b75"
"2023-12-18T21:20:42Z e45f39e2364f5800df90deb0d9916afb9b23dc45"
"2023-12-18T21:27:18Z d85b9e75987145eac37517165a24a17808693ed0"
"2023-12-18T21:28:53Z 1e1c4552a78021f738f4abad8b61652efc09ffea"
"2023-12-18T21:51:04Z c28af07ced54ea1815f0b5ec2b7a83c78d43b806"
"2023-12-19T14:00:34Z ed3a4866ee4f777f407446c99d2844a350f0860d"
"2023-12-19T14:35:42Z f042db6f4776c07312d47481494858880b550446"

[gregfurman]

@JAORMX should we close this in favour of #1862?

[JAORMX]

Sure!

Closing this issue in favor of #1862