Repository navigation
Conversation
…cussions api Resolve still-open review threads on GitLab merge requests whose only comments are outdated inline comments authored by the review agent. On GitLab, when a review agent runs on subsequent commits of an MR, earlier inline review comments created by the review bot remained unresolved, even after newer commits were pushed, because forge_resolve_outdated_review_threads was stubbed as a no-op for GitLab. Query GitLab discussions via GET /projects/:id/merge_requests/:mr_iid/discussions, identifying resolvable, unresolved threads where all notes were authored by the review bot and whose position.head_sha does not match the current MR HEAD SHA. Resolve each candidate thread via PUT .../discussions/:disc_id?resolved=true. Operations remain best-effort and log warnings on failure without aborting the post-review pipeline. Signed-off-by: Jeffery Bagirimvano <jefferyb@redhat.com>
PR Summary by QodoResolve outdated bot review threads on GitLab merge requests
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only comments. One finding is inline. The other is on a file this PR does not change, so it is here.
MEDIUM: docs/review.md still says only GitHub resolves outdated review-agent threads (docs/review.md:368)
docs/review.md:368 still says "On GitHub, it also resolves still-open review threads whose only comments are outdated inline comments authored by the review agent". This PR makes post-review resolve GitLab discussions too. On GitLab, "outdated" means the first note's position.head_sha differs from the MR head, which is not GitHub's isOutdated test. The page that documents this step is now out of date and does not say how GitLab decides a thread is outdated.
Suggestion: update step 4 in docs/review.md to cover GitLab. Say that a thread counts as outdated when the first note's position.head_sha differs from the current MR diff_refs.head_sha, and that only threads where every note is by the review identity are resolved.
| GITLAB_OUTDATED_DISC='[{"id":"disc_outdated","notes":[{"id":10,"author":{"id":100,"username":"review-bot"},"resolvable":true,"resolved":false,"position":{"head_sha":"older_sha"}}]}]' | ||
| GITLAB_SYSTEM_NOTE_DISC='[{"id":"disc_sys","notes":[{"id":14,"author":{"id":100,"username":"review-bot"},"resolvable":true,"resolved":false,"position":{"head_sha":"older_sha"}},{"id":15,"system":true,"author":{"id":1,"username":"system"},"body":"added 1 commit"}]}]' | ||
| GITLAB_RESOLVED_DISC='[{"id":"disc_resolved","notes":[{"id":11,"author":{"id":100,"username":"review-bot"},"resolvable":true,"resolved":true,"position":{"head_sha":"older_sha"}}]}]' | ||
| GITLAB_HUMAN_DISC='[{"id":"disc_human","notes":[{"id":12,"author":{"id":200,"username":"alice"},"resolvable":true,"resolved":false,"position":{"head_sha":"older_sha"}}]}]' |
There was a problem hiding this comment.
MEDIUM: Tests skip the mixed bot/human authorship guard and the discussions-fetch failure path that the PR body says are covered
The PR body says tests cover "Preserving human comments and mixed bot/human threads" and "Resiliency to API error responses". At head c640ccb, the only GitLab non-bot fixture is GITLAB_HUMAN_DISC (this line), which holds a single human note. No discussion mixes bot and human notes, so the all(...) authorship check in forge_resolve_outdated_review_threads has no test for the mixed case. That check is the line that keeps human threads open. The GitHub side does test it (select-skips-mixed-authors, lines 4474 and 4503).
The mock adds MOCK_GITLAB_DISCUSSIONS_FAIL at line 700, but no test sets it, so a failed discussions GET is never run. Nothing tests the /user-missing fallback either. Pagination cannot be tested, because the mock returns the same body for every page.
Suggestion: add a GitLab discussion with one bot note and one human note, and assert that no PUT .../discussions call is made. Add a case with MOCK_GITLAB_DISCUSSIONS_FAIL=1 that expects exit 0 and no PUT. Add a case where /user returns no id. If these are not added, correct the PR body.
There was a problem hiding this comment.
Fixed in 0ec8635: added tests covering mixed bot+human threads (gitlab-mixed-bot-human-no-mutation), MOCK_GITLAB_DISCUSSIONS_FAIL=1 non-fatal warning and no-PUT verification (gitlab-discussions-fail-warning, gitlab-discussions-fail-no-mutation), and /user-missing fallback handling (gitlab-user-fail-marker-fallback, gitlab-user-fail-human-no-mutation).
…error, and expand tests Signed-off-by: Jeffery Bagirimvano <jefferyb@redhat.com>
|
Addressed |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only comments: two inline findings on the authorship filter in forge_resolve_outdated_review_threads.
| | select(.notes | type == "array" and length > 0) | ||
| | select(.notes | any(.resolvable == true and .resolved == false)) | ||
| | select( | ||
| if ($bot_id != "") then |
There was a problem hiding this comment.
MEDIUM: Username check is OR'd with the id check even when the id is known
When GET /user returns an id, the authorship filter is still (.author.id|tostring) == $bot_id or (.author.username // "" | ascii_downcase) == ($bot_user | ascii_downcase). The author id is immutable and is enough to identify the review account. GitLab usernames can change, and a freed username can be claimed by another account. If that happens, notes from the account that now holds the bot's old username pass the "authored by the review identity" check, so their outdated threads get resolved.
A second edge case: if /user returns an id but no username, $bot_user is empty, and any note whose author.username is missing matches "" == "".
The username comparison adds nothing once the id is known. docs/review.md describes the rule as "authored by the review identity", which the id alone already meets. (This is separate from the earlier thread on line 225 about a human PAT owner being treated as the bot.)
Suggestion: in the $bot_id != "" branch, compare on (.author.id | tostring) == $bot_id only and drop the username comparison. Optionally add a test where a note has the bot's username but a different id, and check that it is not resolved.
There was a problem hiding this comment.
Fixed in 8a8b736 (8a8b736): removed bot_username and --arg bot_user from forge_resolve_outdated_review_threads in scripts/lib/gitlab-review-ops.lib.sh so when GET /user returns an ID ($bot_id != ""), authorship compares (.author.id | tostring) == $bot_id only. Added gitlab-different-id-same-username-no-mutation coverage in scripts/post-review-test.sh and rebuilt scripts/post-review.sh.
| | select( | ||
| if ($bot_id != "") then | ||
| .notes | all((.author.id | tostring) == $bot_id or (.author.username // "" | ascii_downcase) == ($bot_user | ascii_downcase)) | ||
| else |
There was a problem hiding this comment.
MEDIUM: The fallback username regex matches other bots, so their threads get resolved when GET /user fails
If GET /user returns no id, the fallback accepts any note whose author.username matches (^|[-_])bot([-_]|$)|\[bot\]$|fullsend. That matches unrelated automation accounts, for example renovate-bot, and every GitLab project or group access token bot (usernames like project_<id>_bot_<hash> or group_<id>_bot_<hash>, which contain _bot_). Every note in such a thread passes all(...), so fullsend resolves outdated diff threads opened by SAST, lint or dependency bots it does not own. That contradicts docs/review.md, which says only threads "authored by the review identity" are resolved. The GitHub path uses only viewerDidAuthor and has no heuristic.
The earlier thread on line 228 was about humans adding body markers, and was declined because all(...) keeps mixed human and bot threads open. That reasoning does not cover threads written entirely by third-party bots, which this regex makes eligible. The gitlab-user-fail-marker-fallback test pins the fallback, and no test checks that another bot's thread is left alone.
Suggestion: fail closed. When GET /user returns no id, emit a ::warning:: and return 0. This matches the GitHub path and the _gitlab_bot_username helpers in gitlab-prioritize/triage. If you want to keep a fallback, at least drop the generic bot username pattern, and add a test where a project_1_bot_abc or renovate-bot thread is not resolved.
There was a problem hiding this comment.
Fixed in 8a8b736 (8a8b736): removed the generic bot username regex from the $bot_id == "" fallback in scripts/lib/gitlab-review-ops.lib.sh so fallback resolution requires the Fullsend body signature (<!-- fullsend / 🤖 Fullsend) on every non-system note. Added gitlab-user-fail-other-bot-no-mutation coverage (renovate-bot / project_1_bot_abc) in scripts/post-review-test.sh and rebuilt scripts/post-review.sh.
Signed-off-by: Jeffery Bagirimvano <jefferyb@redhat.com>
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only comment: one inline finding. The committed scripts/pre-review.sh bundle was not rebuilt after the authorship fix.
| return 0 | ||
| fi | ||
|
|
||
| local current_user bot_user_id bot_username |
There was a problem hiding this comment.
HIGH: The committed scripts/pre-review.sh bundle is stale, so make check-bundle will fail
Commit 8a8b736 ("tighten authorship") updated scripts/lib/gitlab-review-ops.lib.sh and scripts/post-review.sh, but scripts/pre-review.sh was not rebuilt. It still bundles the old forge_resolve_outdated_review_threads. Lines 738-771 still have:
bot_usernameand--arg bot_user- the
.author.usernamematch OR'd with the id check - the fallback regex
(^|[-_])bot([-_]|$)|\[bot\]$|fullsend
Running scripts/bundle-sh.sh on scripts/pre-review.src.sh and diffing the output against the committed file shows these differences. The Makefile check-bundle target runs the same diff and exits 1 with Bundled script stale: scripts/pre-review.sh. The Script tests workflow has not run on this PR (only the dispatch and DCO checks show), so CI has not caught this yet. The PR body says check-bundle showed 0 diff, which is not true at this head.
Pre-review does not call this function today, so this is dead code. It still ships the looser authorship check that 8a8b736 was meant to remove.
Suggestion: Run make script-build, commit the regenerated scripts/pre-review.sh, and confirm make check-bundle passes locally. Get the Script tests workflow to run on the PR before merge.
There was a problem hiding this comment.
Regenerated scripts/pre-review.sh in 87204a22 (87204a22902468487a7ac42df71ff2ac46135bd4).
Verified locally with make -B script-build, make check-bundle (0 diff across all bundled scripts), shellcheck -S warning scripts/lib/gitlab-review-ops.lib.sh scripts/post-review.sh scripts/pre-review.sh scripts/post-review-test.sh, and scripts/post-review-test.sh in both source and bundled modes (all tests passed).
Note: fork PR workflows beyond dispatch and DCO (such as Script tests) appear to require maintainer approval / ok-to-test on fullsend-ai/agents.
Signed-off-by: Jeffery Bagirimvano <jefferyb@redhat.com>
| fi | ||
|
|
||
| # Resolve still-open GitHub review threads whose only comments are | ||
| # Resolve still-open GitHub/GitLab review threads whose only comments are |
There was a problem hiding this comment.
if the agent produces no review this can still close the old GitLab threads. I wouldd move the cleanup to after the new review is posted. @waynesun09
There was a problem hiding this comment.
Good catch — fixed in 1f8995de (1f8995de21c664308619bc46b1400796095ce668):
- Moved
forge_resolve_outdated_review_threadsinscripts/post-review.src.shto run immediately afterforge_post_reviewcompletes and after theACTION = "failure"check, right before outcome labels are applied. Missing agent output, stale-head re-dispatches, failed posts, and explicitaction: "failure"runs now exit without closing existing unresolved GitHub/GitLab threads. - Updated the
forge_resolve_outdated_review_threadscomment inscripts/lib/gitlab-review-ops.lib.shand regeneratedscripts/post-review.sh/scripts/pre-review.sh. - Added regression tests in
scripts/post-review-test.sh(github-missing-result-no-resolve,github-action-failure-no-resolve,gitlab-missing-result-no-resolve,gitlab-action-failure-no-resolve).
Signed-off-by: Jeffery Bagirimvano <jefferyb@redhat.com>
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only comments: two inline findings on forge_resolve_outdated_review_threads.
|
|
||
| local mr_info current_sha | ||
| mr_info=$(_gitlab_api GET "/projects/${REPO_ENCODED}/merge_requests/${PR_NUMBER}" 2>/dev/null) || mr_info="{}" | ||
| current_sha=$(echo "${mr_info}" | jq -r '.diff_refs.head_sha // .sha // empty' 2>/dev/null) || current_sha="" |
There was a problem hiding this comment.
MEDIUM: Outdated check reads the live MR head, so a push after posting resolves the comments this run just posted
forge_resolve_outdated_review_threads reads current_sha from a fresh GET of diff_refs.head_sha. It then resolves any bot thread whose notes[0].position.head_sha differs from it. In scripts/post-review.src.sh the call now runs after forge_post_review (where the stale-head exit 10 is checked) and after the label removals. The fullsend Go post-review places new discussions on the reviewed head.
- If a push lands between the stale-head check and this GET, every inline comment this run just posted carries the old
head_shaand is resolved at once. The review summary still asks for changes. - GitLab rewrites the positions of still-current discussions in its async
ReloadDiffsServiceonly after it has changeddiff_refs. A GET inside that window treats still-current bot threads as outdated.
This is not the declined thread about using a SHA mismatch as the test. That thread said the definition is wrong. This one is a timing race that appears even with the current definition of outdated. Every test fixture sets the mock MR head equal to the reviewed head, so no test covers it.
Suggestion: Compare positions against the head this run reviewed (the head_sha already passed to fullsend post-review), not a fresh diff_refs read. Read the live diff_refs.head_sha only as a guard: if it differs from the reviewed head, log that the head moved and return 0 without resolving anything, because the push will trigger a new review. Add a test where MOCK_MR_HEAD_SHA differs from the result head_sha and assert that no PUT is made.
| if ($bot_id != "") then | ||
| .notes | all((.author.id | tostring) == $bot_id) | ||
| else | ||
| .notes | all((.body // "" | test("<!--[ ]*fullsend|🤖 Fullsend"; "i"))) |
There was a problem hiding this comment.
MEDIUM: The /user-failure fallback is silent, never matches real review comments, and its test uses a made-up marker
When GET /user fails, the function prints no warning and decides authorship by matching each note body against <!--[ ]*fullsend|🤖 Fullsend.
- Real inline review comments never match.
formatFindingCommentin fullsendinternal/cli/postreview.gowrites<!-- finding:<id> -->followed by**[sev]** category.... No code emits🤖 Fullsend. - The fixture behind
gitlab-user-fail-marker-fallbackinscripts/post-review-test.shuses<!-- fullsend:review:id=abc -->, a format nothing produces. The test passes only on invented data. The earlier decline of the quoted-marker thread rested on that test and on consistency with the review scripts. - So the fallback resolves none of the bot's real threads. It does resolve any thread whose notes all quote a fullsend HTML marker, such as
<!-- fullsend:review-agent -->, whoever wrote them. That is the forgeable path raised earlier. - Other GitLab identity lookups in this repo fail closed with a warning (
_gitlab_bot_usernameingitlab-triage-ops.lib.sh).
Suggestion: Drop the body-marker branch. If /user fails or returns no .id, print ::warning::Could not determine review identity — skipping outdated-thread resolution and return 0. Change gitlab-user-fail-marker-fallback to assert that no PUT is made. If a fallback must stay, match the real <!-- finding:f_... --> stamp and copy the fixture from a real comment body.
Summary
Implement
forge_resolve_outdated_review_threadsfor GitLab using the GitLab Discussions REST API, bringing GitLab review thread lifecycle parity with GitHub.Problem
When the review agent runs across iterative commits on a pull/merge request, previously opened inline review comments should be automatically resolved if they pertain to older commits that have since been superseded by new push events.
While GitHub implements this via GraphQL thread resolution, the GitLab implementation in
scripts/lib/gitlab-review-ops.lib.shwas previously an empty stub:As a result, outdated review comments from previous bot review cycles remained open indefinitely on GitLab merge requests, cluttering the MR diff view and requiring human reviewers to manually resolve stale bot threads.
Solution
forge_resolve_outdated_review_threadsinscripts/lib/gitlab-review-ops.lib.sh:GET /projects/:id/merge_requests/:mr_iid/discussions.BOT_USERNAME/BOT_NAME).position.head_shaagainst the current MR HEAD SHA (FORGE_HEAD_SHA).PUT /projects/:id/merge_requests/:mr_iid/discussions/:discussion_id?resolved=true.forge_resolve_outdated_review_threadsintoscripts/post-review.src.shafter review comments are submitted.scripts/post-review-test.sh:make script-build) and verify bundle parity (make check-bundle).Verification
scripts/post-review-test.sh— all 300+ assertions passed, including all new GitLab discussion resolution test cases.make check-bundle— verified 0 diff and clean bundle parity across all bundled targets (pre-review.sh,post-review.sh).