fix: supersede ALL old reviews, not just the most recent #43
Reference in New Issue
Block a user
Delete Branch "fix/supersede-all-old-reviews"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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.
Original reviewSuperseded — see 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
d4bf13eeSecurity 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
gitea/client.goRecommendation
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@@ -426,6 +426,65 @@ func (c *Client) GetTimelineReviewCommentID(ctx context.Context, owner, repo strreturn 0, fmt.Errorf("no timeline event found with sentinel")[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.
Original reviewSuperseded — see 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
gitea/client.gocmd/review-bot/main.gogitea/client.gocmd/review-bot/main.goRecommendation
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@@ -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)[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}[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)}[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),[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.
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
a0fd882bSecurity 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
a0fd882bSonnet 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
gitea/client.gocmd/review-bot/main.goRecommendation
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@@ -646,2 +636,4 @@return best}// findAllOwnReviews returns all non-superseded reviews matching the sentinel.[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.Bodyif len(matchPrefix) > 200 {matchPrefix = matchPrefix[:200]}[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).