fix: supersede ALL old reviews, not just the most recent #43

Merged
rodin merged 2 commits from fix/supersede-all-old-reviews into main 2026-05-02 20:35:23 +00:00
Owner

Fixes the bug where only the most recent old review got superseded on force-push PRs. Now all non-superseded old reviews matching the sentinel are superseded and their inline comments resolved.

Reproduced on gargoyle PR #527 which had 6 stale unsuperseded reviews accumulating.

Fixes the bug where only the most recent old review got superseded on force-push PRs. Now all non-superseded old reviews matching the sentinel are superseded and their inline comments resolved. Reproduced on gargoyle PR #527 which had 6 stale unsuperseded reviews accumulating.
rodin added 1 commit 2026-05-02 20:28:11 +00:00
fix: supersede ALL old reviews, not just the most recent
CI / test (pull_request) Successful in 14s
CI / review (gpt-4.1, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 22s
CI / review (gpt-5, security, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 46s
CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 1m7s
d4bf13eeab
Previously findOwnReview returned only the single most-recent matching
review, so on PRs with multiple force-pushes only the latest old review
got superseded. The rest accumulated as unsuperseded stale reviews.

Changes:
- Add findAllOwnReviews() to collect all non-superseded matching reviews
- Loop over all old reviews in the supersede phase
- Add GetTimelineReviewCommentIDForReview() to find comment IDs by
  review ID (fetches review body, matches in timeline by prefix)
- Each old review gets independently superseded and its inline comments
  resolved

The old findOwnReview is kept for backward compat (tested, may be
useful as a utility).
gpt-review-bot approved these changes 2026-05-02 20:28:48 +00:00
Dismissed
gpt-review-bot left a comment
First-time contributor

Original review

Supersededsee current review for up-to-date findings.

Previous findings (commit d4bf13ee)

Gpt Review

Summary

The change correctly supersedes all existing non-superseded reviews matching the sentinel rather than just the most recent, preventing stale reviews from accumulating. Test coverage and helper logic are properly updated.

Recommendation

APPROVE — This change correctly generalizes the supersede logic to handle all matching past reviews rather than only the most recent, fixing the problem of accumulating outdated reviews. It handles errors on a per-review basis, avoids superseding in shared token scenarios, and preserves the logic for resolving associated inline comments. The supporting test and helper code are updated appropriately, improving test coverage and correctness. No issues found—merge as is.


Review by gpt


Evaluated against d4bf13ee

~~Original review~~ **Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/43#pullrequestreview-690) for up-to-date findings. <details><summary>Previous findings (commit d4bf13ee)</summary> # Gpt Review ## Summary The change correctly supersedes all existing non-superseded reviews matching the sentinel rather than just the most recent, preventing stale reviews from accumulating. Test coverage and helper logic are properly updated. ## Recommendation **APPROVE** — This change correctly generalizes the supersede logic to handle all matching past reviews rather than only the most recent, fixing the problem of accumulating outdated reviews. It handles errors on a per-review basis, avoids superseding in shared token scenarios, and preserves the logic for resolving associated inline comments. The supporting test and helper code are updated appropriately, improving test coverage and correctness. No issues found—merge as is. --- *Review by gpt* <!-- review-bot:gpt --> --- *Evaluated against d4bf13ee* </details> <!-- review-bot:gpt -->
security-review-bot requested review from security-review-bot 2026-05-02 20:29:13 +00:00
security-review-bot approved these changes 2026-05-02 20:29:13 +00:00
Dismissed
security-review-bot left a comment
Collaborator

Security Review

Summary

The changes broaden superseding to all prior bot reviews and add per-review timeline matching, which is a sensible and safer approach. No exploitable security issues are evident; CI passed and input handling remains guarded (e.g., reviewer-name validation, workspace path checks).

Findings

# Severity File Line Finding
1 [MINOR] gitea/client.go 426 GetTimelineReviewCommentIDForReview matches timeline events by checking strings.HasPrefix on the first ~200 bytes of a review body. In the unlikely case of two reviews with identical prefixes, this could target the wrong timeline comment for editing, potentially modifying another review's summary. Consider tightening the match (e.g., include a unique sentinel in the prefix or match full body with a fallback) to reduce mis-targeting risk.

Recommendation

APPROVE — Overall, the update to supersede all prior non-superseded reviews and resolve their inline comments is well-scoped and reduces stale review accumulation. The switch to identifying timeline comments per specific review ID is a security-hardening move compared to sentinel-only matching. To further improve robustness, strengthen the timeline event matching in GetTimelineReviewCommentIDForReview to reduce the chance of editing an unintended comment (e.g., by ensuring the unique sentinel or the full body is included in the comparison, or additionally verifying author/metadata). Otherwise, approve as is.


Review by security


Evaluated against d4bf13ee

# Security Review ## Summary The changes broaden superseding to all prior bot reviews and add per-review timeline matching, which is a sensible and safer approach. No exploitable security issues are evident; CI passed and input handling remains guarded (e.g., reviewer-name validation, workspace path checks). ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `gitea/client.go` | 426 | GetTimelineReviewCommentIDForReview matches timeline events by checking strings.HasPrefix on the first ~200 bytes of a review body. In the unlikely case of two reviews with identical prefixes, this could target the wrong timeline comment for editing, potentially modifying another review's summary. Consider tightening the match (e.g., include a unique sentinel in the prefix or match full body with a fallback) to reduce mis-targeting risk. | ## Recommendation **APPROVE** — Overall, the update to supersede all prior non-superseded reviews and resolve their inline comments is well-scoped and reduces stale review accumulation. The switch to identifying timeline comments per specific review ID is a security-hardening move compared to sentinel-only matching. To further improve robustness, strengthen the timeline event matching in GetTimelineReviewCommentIDForReview to reduce the chance of editing an unintended comment (e.g., by ensuring the unique sentinel or the full body is included in the comparison, or additionally verifying author/metadata). Otherwise, approve as is. --- *Review by security* <!-- review-bot:security --> --- *Evaluated against d4bf13ee*
@@ -426,6 +426,65 @@ func (c *Client) GetTimelineReviewCommentID(ctx context.Context, owner, repo str
return 0, fmt.Errorf("no timeline event found with sentinel")
Collaborator

[MINOR] GetTimelineReviewCommentIDForReview matches timeline events by checking strings.HasPrefix on the first ~200 bytes of a review body. In the unlikely case of two reviews with identical prefixes, this could target the wrong timeline comment for editing, potentially modifying another review's summary. Consider tightening the match (e.g., include a unique sentinel in the prefix or match full body with a fallback) to reduce mis-targeting risk.

**[MINOR]** GetTimelineReviewCommentIDForReview matches timeline events by checking strings.HasPrefix on the first ~200 bytes of a review body. In the unlikely case of two reviews with identical prefixes, this could target the wrong timeline comment for editing, potentially modifying another review's summary. Consider tightening the match (e.g., include a unique sentinel in the prefix or match full body with a fallback) to reduce mis-targeting risk.
sonnet-review-bot approved these changes 2026-05-02 20:29:35 +00:00
sonnet-review-bot left a comment
First-time contributor

Original review

Supersededsee current review for up-to-date findings.

Previous findings (commit d4bf13ee)

Sonnet Review

Summary

The changes correctly extend superseding to all prior non-superseded reviews and add a robust method to map specific reviews to their timeline comments. Tests cover the new helper, and CI is passing. A couple of minor robustness and efficiency improvements are suggested.

Findings

# Severity File Line Finding
1 [MINOR] gitea/client.go 463 GetTimelineReviewCommentIDForReview matches timeline events using only a HasPrefix on the review body. If multiple reviews share identical leading content, this could match the wrong event. Consider also verifying the event's user (ev.User.Login) and/or checking for the sentinel substring to reduce false positives.
2 [NIT] cmd/review-bot/main.go 327 sharedTokenMode is only used to gate collection of oldReviews and to log a warning; the variable itself isn't used later. You could simplify by inlining the condition or removing the variable to reduce local state.
3 [NIT] gitea/client.go 452 For each old review, the code scans the entire issue timeline to find the matching comment ID. If many stale reviews accumulate, this results in repeated timeline fetches. Consider fetching the timeline once and mapping bodies/prefixes to IDs to reduce API calls.
4 [NIT] cmd/review-bot/main.go 385 When resolving old inline comments, failures are only logged at debug level per comment and no summary of failures is emitted. Consider aggregating and logging a warning if some resolutions failed to aid observability.

Recommendation

APPROVE — Overall, the implementation achieves the stated goal: all prior non-superseded reviews are now superseded, and their inline comments resolved. The new client method to locate a specific review's timeline comment is a solid improvement over the sentinel-only approach. To further harden the solution, consider tightening the timeline match by also checking the event's user (and/or sentinel presence) to avoid ambiguous prefix matches, and reduce repeated timeline scans by caching or prefetching when many old reviews exist. Minor logging and small simplifications can improve readability and observability, but they are not blockers. With CI passing and no major issues found, this is good to merge.


Review by sonnet


Evaluated against d4bf13ee

~~Original review~~ **Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/43#pullrequestreview-693) for up-to-date findings. <details><summary>Previous findings (commit d4bf13ee)</summary> # Sonnet Review ## Summary The changes correctly extend superseding to all prior non-superseded reviews and add a robust method to map specific reviews to their timeline comments. Tests cover the new helper, and CI is passing. A couple of minor robustness and efficiency improvements are suggested. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `gitea/client.go` | 463 | GetTimelineReviewCommentIDForReview matches timeline events using only a HasPrefix on the review body. If multiple reviews share identical leading content, this could match the wrong event. Consider also verifying the event's user (ev.User.Login) and/or checking for the sentinel substring to reduce false positives. | | 2 | [NIT] | `cmd/review-bot/main.go` | 327 | sharedTokenMode is only used to gate collection of oldReviews and to log a warning; the variable itself isn't used later. You could simplify by inlining the condition or removing the variable to reduce local state. | | 3 | [NIT] | `gitea/client.go` | 452 | For each old review, the code scans the entire issue timeline to find the matching comment ID. If many stale reviews accumulate, this results in repeated timeline fetches. Consider fetching the timeline once and mapping bodies/prefixes to IDs to reduce API calls. | | 4 | [NIT] | `cmd/review-bot/main.go` | 385 | When resolving old inline comments, failures are only logged at debug level per comment and no summary of failures is emitted. Consider aggregating and logging a warning if some resolutions failed to aid observability. | ## Recommendation **APPROVE** — Overall, the implementation achieves the stated goal: all prior non-superseded reviews are now superseded, and their inline comments resolved. The new client method to locate a specific review's timeline comment is a solid improvement over the sentinel-only approach. To further harden the solution, consider tightening the timeline match by also checking the event's user (and/or sentinel presence) to avoid ambiguous prefix matches, and reduce repeated timeline scans by caching or prefetching when many old reviews exist. Minor logging and small simplifications can improve readability and observability, but they are not blockers. With CI passing and no major issues found, this is good to merge. --- *Review by sonnet* <!-- review-bot:sonnet --> --- *Evaluated against d4bf13ee* </details> <!-- review-bot:sonnet -->
@@ -324,4 +324,4 @@
if *reviewerName != "" {
existingReviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber)
if err != nil {
slog.Warn("could not list existing reviews", "pr", prNumber, "error", err)
First-time contributor

[NIT] sharedTokenMode is only used to gate collection of oldReviews and to log a warning; the variable itself isn't used later. You could simplify by inlining the condition or removing the variable to reduce local state.

**[NIT]** sharedTokenMode is only used to gate collection of oldReviews and to log a warning; the variable itself isn't used later. You could simplify by inlining the condition or removing the variable to reduce local state.
@@ -397,1 +382,4 @@
for _, c := range oldComments {
if c.ID == 0 {
continue
}
First-time contributor

[NIT] When resolving old inline comments, failures are only logged at debug level per comment and no summary of failures is emitted. Consider aggregating and logging a warning if some resolutions failed to aid observability.

**[NIT]** When resolving old inline comments, failures are only logged at debug level per comment and no summary of failures is emitted. Consider aggregating and logging a warning if some resolutions failed to aid observability.
@@ -429,0 +449,4 @@
if review.Body == "" {
return 0, fmt.Errorf("review %d has empty body", reviewID)
}
First-time contributor

[NIT] For each old review, the code scans the entire issue timeline to find the matching comment ID. If many stale reviews accumulate, this results in repeated timeline fetches. Consider fetching the timeline once and mapping bodies/prefixes to IDs to reduce API calls.

**[NIT]** For each old review, the code scans the entire issue timeline to find the matching comment ID. If many stale reviews accumulate, this results in repeated timeline fetches. Consider fetching the timeline once and mapping bodies/prefixes to IDs to reduce API calls.
@@ -429,0 +460,4 @@
for page := 1; ; page++ {
timelineURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/issues/%d/timeline?limit=%d&page=%d",
c.baseURL,
url.PathEscape(owner),
First-time contributor

[MINOR] GetTimelineReviewCommentIDForReview matches timeline events using only a HasPrefix on the review body. If multiple reviews share identical leading content, this could match the wrong event. Consider also verifying the event's user (ev.User.Login) and/or checking for the sentinel substring to reduce false positives.

**[MINOR]** GetTimelineReviewCommentIDForReview matches timeline events using only a HasPrefix on the review body. If multiple reviews share identical leading content, this could match the wrong event. Consider also verifying the event's user (ev.User.Login) and/or checking for the sentinel substring to reduce false positives.
rodin added 1 commit 2026-05-02 20:32:01 +00:00
fix: address review findings
CI / test (pull_request) Successful in 14s
CI / review (gpt-4.1, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 24s
CI / review (gpt-5, security, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 37s
CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 1m4s
a0fd882b0d
- Tighten timeline matching: also check ev.User.Login matches
  the review author (prevents collision on identical body prefix)
- Remove unused sharedTokenMode variable (inline condition)
- Aggregate resolution failures with warn-level summary
gpt-review-bot approved these changes 2026-05-02 20:32:39 +00:00
gpt-review-bot left a comment
First-time contributor

Gpt Review

Summary

The change fixes an issue where stale reviews accumulate by ensuring all non-superseded reviews are superseded rather than just the most recent one. The update is correct, thorough, and well-tested.

Recommendation

APPROVE — The patch improves the review-bot logic to supersede all existing non-superseded reviews, which aligns with the intended behavior and resolves an accumulation/staleness bug. The new and changed helper function is clear and covered by tests. No regressions or correctness concerns found. CI passes. Recommend merge.


Review by gpt


Evaluated against a0fd882b

# Gpt Review ## Summary The change fixes an issue where stale reviews accumulate by ensuring all non-superseded reviews are superseded rather than just the most recent one. The update is correct, thorough, and well-tested. ## Recommendation **APPROVE** — The patch improves the review-bot logic to supersede all existing non-superseded reviews, which aligns with the intended behavior and resolves an accumulation/staleness bug. The new and changed helper function is clear and covered by tests. No regressions or correctness concerns found. CI passes. Recommend merge. --- *Review by gpt* <!-- review-bot:gpt --> --- *Evaluated against a0fd882b*
security-review-bot requested review from security-review-bot 2026-05-02 20:32:53 +00:00
security-review-bot approved these changes 2026-05-02 20:32:53 +00:00
security-review-bot left a comment
Collaborator

Security Review

Summary

The changes broaden superseding to all prior matching reviews and add a robust method to locate specific review timeline comments. No new security risks are introduced, and input handling and API usage remain safe and consistent with prior code.

Recommendation

APPROVE — The implementation safely iterates and supersedes all previous matching reviews, resolving prior inline comments as intended. The added client method to identify timeline comment IDs for specific reviews uses authenticated requests and conservative matching and does not expose sensitive data. With CI passing and no security concerns arising from these changes, this PR is good to merge.


Review by security


Evaluated against a0fd882b

# Security Review ## Summary The changes broaden superseding to all prior matching reviews and add a robust method to locate specific review timeline comments. No new security risks are introduced, and input handling and API usage remain safe and consistent with prior code. ## Recommendation **APPROVE** — The implementation safely iterates and supersedes all previous matching reviews, resolving prior inline comments as intended. The added client method to identify timeline comment IDs for specific reviews uses authenticated requests and conservative matching and does not expose sensitive data. With CI passing and no security concerns arising from these changes, this PR is good to merge. --- *Review by security* <!-- review-bot:security --> --- *Evaluated against a0fd882b*
sonnet-review-bot approved these changes 2026-05-02 20:33:19 +00:00
sonnet-review-bot left a comment
First-time contributor

Sonnet Review

Summary

Solid fix that supersedes all prior bot reviews instead of only the most recent, and resolves their inline comments. The implementation is careful about shared-token scenarios and adds focused tests; CI is green.

Findings

# Severity File Line Finding
1 [MINOR] gitea/client.go 460 GetTimelineReviewCommentIDForReview matches timeline events by user and a 200-char prefix of the review body. If multiple historical reviews have identical beginnings (common for templated bot reviews), this can select the wrong timeline event, leading to editing one review's timeline comment while resolving another review's inline comments. Consider strengthening the match (e.g., also require the presence of the short commit SHA in ev.Body, compare a longer/hash-based signature, or disambiguate among multiple matches deterministically).
2 [NIT] cmd/review-bot/main.go 639 findAllOwnReviews returns a slice of gitea.Review (by value), which copies potentially large Body strings. Returning []*gitea.Review would avoid copying and align with findOwnReview's pointer return; not critical but can reduce allocations for large sets of reviews.

Recommendation

APPROVE — The change correctly expands superseding to all previous non-superseded reviews for the given sentinel and resolves their inline comments. The added client method to locate the specific timeline comment improves precision over a pure sentinel search, and the tests cover the new helper. Consider tightening the timeline matching logic in GetTimelineReviewCommentIDForReview to avoid ambiguity when bodies share identical prefixes (for example, include the short commit SHA in the match or use a stronger signature). The current approach is acceptable, and with CI passing, this is good to merge.


Review by sonnet


Evaluated against a0fd882b

# Sonnet Review ## Summary Solid fix that supersedes all prior bot reviews instead of only the most recent, and resolves their inline comments. The implementation is careful about shared-token scenarios and adds focused tests; CI is green. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `gitea/client.go` | 460 | GetTimelineReviewCommentIDForReview matches timeline events by user and a 200-char prefix of the review body. If multiple historical reviews have identical beginnings (common for templated bot reviews), this can select the wrong timeline event, leading to editing one review's timeline comment while resolving another review's inline comments. Consider strengthening the match (e.g., also require the presence of the short commit SHA in ev.Body, compare a longer/hash-based signature, or disambiguate among multiple matches deterministically). | | 2 | [NIT] | `cmd/review-bot/main.go` | 639 | findAllOwnReviews returns a slice of gitea.Review (by value), which copies potentially large Body strings. Returning []*gitea.Review would avoid copying and align with findOwnReview's pointer return; not critical but can reduce allocations for large sets of reviews. | ## Recommendation **APPROVE** — The change correctly expands superseding to all previous non-superseded reviews for the given sentinel and resolves their inline comments. The added client method to locate the specific timeline comment improves precision over a pure sentinel search, and the tests cover the new helper. Consider tightening the timeline matching logic in GetTimelineReviewCommentIDForReview to avoid ambiguity when bodies share identical prefixes (for example, include the short commit SHA in the match or use a stronger signature). The current approach is acceptable, and with CI passing, this is good to merge. --- *Review by sonnet* <!-- review-bot:sonnet --> --- *Evaluated against a0fd882b*
@@ -646,2 +636,4 @@
return best
}
// findAllOwnReviews returns all non-superseded reviews matching the sentinel.
First-time contributor

[NIT] findAllOwnReviews returns a slice of gitea.Review (by value), which copies potentially large Body strings. Returning []*gitea.Review would avoid copying and align with findOwnReview's pointer return; not critical but can reduce allocations for large sets of reviews.

**[NIT]** findAllOwnReviews returns a slice of gitea.Review (by value), which copies potentially large Body strings. Returning []*gitea.Review would avoid copying and align with findOwnReview's pointer return; not critical but can reduce allocations for large sets of reviews.
@@ -429,0 +457,4 @@
matchPrefix := review.Body
if len(matchPrefix) > 200 {
matchPrefix = matchPrefix[:200]
}
First-time contributor

[MINOR] GetTimelineReviewCommentIDForReview matches timeline events by user and a 200-char prefix of the review body. If multiple historical reviews have identical beginnings (common for templated bot reviews), this can select the wrong timeline event, leading to editing one review's timeline comment while resolving another review's inline comments. Consider strengthening the match (e.g., also require the presence of the short commit SHA in ev.Body, compare a longer/hash-based signature, or disambiguate among multiple matches deterministically).

**[MINOR]** GetTimelineReviewCommentIDForReview matches timeline events by user and a 200-char prefix of the review body. If multiple historical reviews have identical beginnings (common for templated bot reviews), this can select the wrong timeline event, leading to editing one review's timeline comment while resolving another review's inline comments. Consider strengthening the match (e.g., also require the presence of the short commit SHA in ev.Body, compare a longer/hash-based signature, or disambiguate among multiple matches deterministically).
rodin merged commit 8d8a249481 into main 2026-05-02 20:35:23 +00:00
rodin deleted branch fix/supersede-all-old-reviews 2026-05-02 20:35:23 +00:00
Sign in to join this conversation.
4 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: rodin/review-bot#43