cmd/review-bot/main.go

@@ -257,56 +257,70 @@ func main() {
 		log.Printf("Attaching %d inline comments", len(inlineComments))
 	}
 
-	// Check if existing review is unchanged — skip to preserve conversation threads
+	// --- Review update strategy ---
+	// 1. No existing review → POST new
+	// 2. Existing review, same state → PATCH body in place (preserves threads)
+	// 3. Existing review, state change → PATCH old to "Superseded", POST new
 	if *updateExisting && *reviewerName != "" {
 		existingReviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber)
-		if err == nil && reviewUnchanged(existingReviews, reviewBody, event, sentinel) {
-			log.Printf("Review unchanged from previous run; skipping to preserve threads")
-			return
-		}
-	}
-
-	log.Printf("Posting review (event=%s)...", event)
-	posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, inlineComments)
-	if err != nil {
-		log.Fatalf("Failed to post review: %v", err)
-	}
-	log.Printf("Review posted (id=%d, user=%s)", posted.ID, posted.User.Login)
-
-	// Delete stale reviews from this bot using sentinel matching
-	if *updateExisting && *reviewerName != "" {
-		reviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber)
 		if err != nil {
 			log.Printf("Warning: could not list existing reviews: %v", err)
 		} else {
-			for _, r := range reviews {
-				if r.ID != posted.ID && r.User.Login == posted.User.Login && strings.Contains(r.Body, sentinel) {
-					if err := giteaClient.DeleteReview(ctx, owner, repoName, prNumber, r.ID); err != nil {
-						log.Printf("Warning: could not delete old review %d: %v", r.ID, err)
-					} else {
-						log.Printf("Deleted stale review %d", r.ID)
-					}
-				}
+			// Worst-wins: escalate if a sibling blocks (need own login from existing review)
+			ownLogin := ""
+			existing := findOwnReview(existingReviews, sentinel)
+			if existing != nil {
+				ownLogin = existing.User.Login
+			}
+			if event == "APPROVED" && shouldEscalate(existingReviews, 0, ownLogin, sentinel) {
+				log.Printf("Sibling review has REQUEST_CHANGES; escalating to REQUEST_CHANGES")
+				event = "REQUEST_CHANGES"
 			}
 
-			// Worst-wins: if we posted APPROVE but a sibling review from the
-			// same user (same token, different role) has REQUEST_CHANGES,
-			// delete ours and re-post as REQUEST_CHANGES to maintain the block.
-			if event == "APPROVED" && shouldEscalate(reviews, posted.ID, posted.User.Login, sentinel) {
-				log.Printf("Sibling review has REQUEST_CHANGES; escalating")
-				if err := giteaClient.DeleteReview(ctx, owner, repoName, prNumber, posted.ID); err != nil {
-					log.Printf("Warning: could not delete review for escalation: %v", err)
-				} else {
-					_, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, "REQUEST_CHANGES", reviewBody, inlineComments)
+			if existing != nil {
+				if reviewUnchanged(existingReviews, reviewBody, event, sentinel) {
+					log.Printf("Review unchanged from previous run; skipping to preserve threads")
+					return
+				}
+
+				// Same state → PATCH in place
+				if existing.State == event {
+					commentID, err := giteaClient.GetTimelineReviewCommentID(ctx, owner, repoName, prNumber, sentinel)
 					if err != nil {
-						log.Printf("Warning: could not re-post as REQUEST_CHANGES: %v", err)
+						log.Printf("Warning: could not find review comment ID, falling back to new post: %v", err)
 					} else {
-						log.Printf("Review escalated to REQUEST_CHANGES")
+						if err := giteaClient.EditComment(ctx, owner, repoName, commentID, reviewBody); err != nil {
+							log.Printf("Warning: could not edit review, falling back to new post: %v", err)
+						} else {
+							log.Printf("Review updated in place (comment_id=%d)", commentID)
+							return
+						}
+					}
+				} else {
+					// State change → mark old as superseded, post new below
+					commentID, err := giteaClient.GetTimelineReviewCommentID(ctx, owner, repoName, prNumber, sentinel)
+					if err != nil {
+						log.Printf("Warning: could not find old review comment ID: %v", err)
+					} else {
+						supersededBody := fmt.Sprintf("~~*This review has been superseded by a newer review below.*~~\n\n%s", sentinel)
+						if err := giteaClient.EditComment(ctx, owner, repoName, commentID, supersededBody); err != nil {
+							log.Printf("Warning: could not mark old review as superseded: %v", err)
+						} else {
+							log.Printf("Marked old review as superseded (state was %s, now %s)", existing.State, event)
+						}
 					}
 				}
 			}
 		}
 	}
+
+	// POST new review (first run, or state transition fallthrough)
+	log.Printf("Posting review (event=%s)...", event)
+	_, err = giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, inlineComments)
+	if err != nil {
+		log.Fatalf("Failed to post review: %v", err)
+	}
+	log.Printf("Review posted successfully")
 }
 
 // fetchFileContext fetches the full content of modified files from the PR branch.
@@ -463,12 +477,20 @@ func validateReviewerName(name string) error {
 	return nil
 }
 
-// shouldEscalate checks if the current APPROVED review should be escalated
-// to REQUEST_CHANGES because a sibling bot review (same user, different role)
-// already has REQUEST_CHANGES.
-func shouldEscalate(reviews []gitea.Review, postedID int64, postedLogin, ownSentinel string) bool {
+// shouldEscalate checks if any sibling bot review from the same user
+// (different sentinel, same token) has REQUEST_CHANGES.
+// ownLogin is the bot user login; if empty, escalation check is skipped.
+// postedID is excluded from consideration (0 means no exclusion needed).
+func shouldEscalate(reviews []gitea.Review, postedID int64, ownLogin, ownSentinel string) bool {
+	if ownLogin == "" {
+		return false
+	}
 	for _, r := range reviews {
-		if r.ID != postedID && !r.Stale && r.User.Login == postedLogin && r.State == "REQUEST_CHANGES" && strings.Contains(r.Body, "<!-- review-bot:") && !strings.Contains(r.Body, ownSentinel) {
+		if r.ID == postedID || r.Stale {
+			continue
+		}
+		// Sibling = same user, has a review-bot sentinel, but not OUR sentinel
+		if r.User.Login == ownLogin && r.State == "REQUEST_CHANGES" && strings.Contains(r.Body, "<!-- review-bot:") && !strings.Contains(r.Body, ownSentinel) {
 			return true
 		}
 	}
@@ -494,3 +516,13 @@ func reviewUnchanged(reviews []gitea.Review, newBody, newEvent, sentinel string)
 	}
 	return false
 }
+
+// findOwnReview locates a review matching the given sentinel in its body.
+func findOwnReview(reviews []gitea.Review, sentinel string) *gitea.Review {
+	for i := range reviews {
+		if strings.Contains(reviews[i].Body, sentinel) {
+			return &reviews[i]
+		}
+	}
+	return nil
+}

cmd/review-bot/main_test.go

@@ -55,7 +55,8 @@ func TestShouldEscalate(t *testing.T) {
 		name        string
 		reviews     []gitea.Review
 		postedID    int64
-		postedLogin string
+		ownLogin    string
+
 		ownSentinel string
 		want        bool
 	}{
@@ -63,7 +64,7 @@ func TestShouldEscalate(t *testing.T) {
 			name:        "no reviews",
 			reviews:     nil,
 			postedID:    100,
-			postedLogin: "bot",
+			ownLogin:    "bot",
 			ownSentinel: "<!-- review-bot:sonnet -->",
 			want:        false,
 		},
@@ -73,7 +74,7 @@ func TestShouldEscalate(t *testing.T) {
 				makeReview(101, "bot", "REQUEST_CHANGES", false, "bad\n<!-- review-bot:security -->"),
 			},
 			postedID:    100,
-			postedLogin: "bot",
+			ownLogin:    "bot",
 			ownSentinel: "<!-- review-bot:sonnet -->",
 			want:        true,
 		},
@@ -83,7 +84,7 @@ func TestShouldEscalate(t *testing.T) {
 				makeReview(101, "other-bot", "REQUEST_CHANGES", false, "bad\n<!-- review-bot:gpt -->"),
 			},
 			postedID:    100,
-			postedLogin: "bot",
+			ownLogin:    "bot",
 			ownSentinel: "<!-- review-bot:sonnet -->",
 			want:        false,
 		},
@@ -93,7 +94,7 @@ func TestShouldEscalate(t *testing.T) {
 				makeReview(101, "bot", "REQUEST_CHANGES", true, "old\n<!-- review-bot:security -->"),
 			},
 			postedID:    100,
-			postedLogin: "bot",
+			ownLogin:    "bot",
 			ownSentinel: "<!-- review-bot:sonnet -->",
 			want:        false,
 		},
@@ -103,7 +104,7 @@ func TestShouldEscalate(t *testing.T) {
 				makeReview(101, "bot", "REQUEST_CHANGES", false, "old\n<!-- review-bot:sonnet -->"),
 			},
 			postedID:    100,
-			postedLogin: "bot",
+			ownLogin:    "bot",
 			ownSentinel: "<!-- review-bot:sonnet -->",
 			want:        false,
 		},
@@ -113,7 +114,7 @@ func TestShouldEscalate(t *testing.T) {
 				makeReview(101, "bot", "APPROVED", false, "good\n<!-- review-bot:security -->"),
 			},
 			postedID:    100,
-			postedLogin: "bot",
+			ownLogin:    "bot",
 			ownSentinel: "<!-- review-bot:sonnet -->",
 			want:        false,
 		},
@@ -123,7 +124,7 @@ func TestShouldEscalate(t *testing.T) {
 				makeReview(101, "bot", "REQUEST_CHANGES", false, "please fix this"),
 			},
 			postedID:    100,
-			postedLogin: "bot",
+			ownLogin:    "bot",
 			ownSentinel: "<!-- review-bot:sonnet -->",
 			want:        false,
 		},
@@ -133,7 +134,7 @@ func TestShouldEscalate(t *testing.T) {
 				makeReview(100, "bot", "REQUEST_CHANGES", false, "x\n<!-- review-bot:security -->"),
 			},
 			postedID:    100,
-			postedLogin: "bot",
+			ownLogin:    "bot",
 			ownSentinel: "<!-- review-bot:sonnet -->",
 			want:        false,
 		},
@@ -141,7 +142,7 @@ func TestShouldEscalate(t *testing.T) {
 
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
-			got := shouldEscalate(tc.reviews, tc.postedID, tc.postedLogin, tc.ownSentinel)
+			got := shouldEscalate(tc.reviews, tc.postedID, tc.ownLogin, tc.ownSentinel)
 			if got != tc.want {
 				t.Errorf("shouldEscalate() = %v, want %v", got, tc.want)
 			}
@@ -227,3 +228,63 @@ func TestReviewUnchanged(t *testing.T) {
 		})
 	}
 }
+
+func TestFindOwnReview(t *testing.T) {
+	tests := []struct {
+		name     string
+		reviews  []gitea.Review
+		sentinel string
+		wantID   int64
+		wantNil  bool
+	}{
+		{
+			name:     "no reviews",
+			reviews:  nil,
+			sentinel: "<!-- review-bot:sonnet -->",
+			wantNil:  true,
+		},
+		{
+			name: "found by sentinel",
+			reviews: []gitea.Review{
+				makeReview(42, "bot", "APPROVED", false, "review body\n<!-- review-bot:sonnet -->"),
+			},
+			sentinel: "<!-- review-bot:sonnet -->",
+			wantID:   42,
+		},
+		{
+			name: "wrong sentinel",
+			reviews: []gitea.Review{
+				makeReview(42, "bot", "APPROVED", false, "body\n<!-- review-bot:gpt -->"),
+			},
+			sentinel: "<!-- review-bot:sonnet -->",
+			wantNil:  true,
+		},
+		{
+			name: "multiple reviews, returns first match",
+			reviews: []gitea.Review{
+				makeReview(10, "bot", "APPROVED", false, "old\n<!-- review-bot:gpt -->"),
+				makeReview(20, "bot", "APPROVED", false, "new\n<!-- review-bot:sonnet -->"),
+			},
+			sentinel: "<!-- review-bot:sonnet -->",
+			wantID:   20,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			got := findOwnReview(tc.reviews, tc.sentinel)
+			if tc.wantNil {
+				if got != nil {
+					t.Errorf("findOwnReview() = %v, want nil", got)
+				}
+			} else {
+				if got == nil {
+					t.Fatal("findOwnReview() = nil, want non-nil")
+				}
+				if got.ID != tc.wantID {
+					t.Errorf("findOwnReview().ID = %d, want %d", got.ID, tc.wantID)
+				}
+			}
+		})
+	}
+}

gitea/client.go

@@ -353,3 +353,78 @@ func (c *Client) DeleteReview(ctx context.Context, owner, repo string, number in
 	}
 	return nil
 }
+
+// TimelineEvent represents an entry from the issue timeline API.
+type TimelineEvent struct {
+	ID   int64  `json:"id"`
+	Type string `json:"type"`
+	Body string `json:"body"`
+	User struct {
+		Login string `json:"login"`
+	} `json:"user"`
+}
+
+// GetTimelineReviewCommentID finds the comment ID for a review body by
+// scanning the issue timeline for a review event containing the sentinel.
+func (c *Client) GetTimelineReviewCommentID(ctx context.Context, owner, repo string, number int, sentinel string) (int64, error) {
+	const pageSize = 50
+	for page := 1; ; page++ {
+		reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/issues/%d/timeline?limit=%d&page=%d",
+			c.baseURL,
+			url.PathEscape(owner),
+			url.PathEscape(repo),
+			number,
+			pageSize,
+			page)
+		body, err := c.doGet(ctx, reqURL)
+		if err != nil {
+			return 0, fmt.Errorf("get timeline (page %d): %w", page, err)
+		}
+		var events []TimelineEvent
+		if err := json.Unmarshal(body, &events); err != nil {
+			return 0, fmt.Errorf("parse timeline (page %d): %w", page, err)
+		}
+		for _, ev := range events {
+			if ev.Type == "review" && strings.Contains(ev.Body, sentinel) {
+				return ev.ID, nil
+			}
+		}
+		if len(events) < pageSize {
+			break
+		}
+	}
+	return 0, fmt.Errorf("no timeline event found with sentinel")
+}
+
+// EditComment updates the body of an issue/review comment.
+func (c *Client) EditComment(ctx context.Context, owner, repo string, commentID int64, newBody string) error {
+	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/issues/comments/%d",
+		c.baseURL,
+		url.PathEscape(owner),
+		url.PathEscape(repo),
+		commentID)
+
+	payload := struct {
+		Body string `json:"body"`
+	}{Body: newBody}
+	data, _ := json.Marshal(payload)
+
+	req, err := http.NewRequestWithContext(ctx, http.MethodPatch, reqURL, bytes.NewReader(data))
+	if err != nil {
+		return fmt.Errorf("create edit request: %w", err)
+	}
+	req.Header.Set("Authorization", "token "+c.token)
+	req.Header.Set("Content-Type", "application/json")
+
+	resp, err := c.http.Do(req)
+	if err != nil {
+		return fmt.Errorf("edit comment: %w", err)
+	}
+	defer resp.Body.Close()
+
+	if resp.StatusCode != http.StatusOK {
+		body, _ := io.ReadAll(resp.Body)
+		return fmt.Errorf("edit comment failed (status %d): %s", resp.StatusCode, body)
+	}
+	return nil
+}

gitea/client_test.go

@@ -426,3 +426,82 @@ func TestDeleteReview_Forbidden(t *testing.T) {
 		t.Fatal("expected error for 403, got nil")
 	}
 }
+
+func TestEditComment(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.Method != http.MethodPatch {
+			t.Errorf("expected PATCH, got %s", r.Method)
+		}
+		if r.URL.Path != "/api/v1/repos/owner/repo/issues/comments/42" {
+			t.Errorf("unexpected path: %s", r.URL.Path)
+		}
+
+		var payload struct {
+			Body string `json:"body"`
+		}
+		json.NewDecoder(r.Body).Decode(&payload)
+		if payload.Body != "updated body" {
+			t.Errorf("unexpected body: %s", payload.Body)
+		}
+
+		w.WriteHeader(http.StatusOK)
+		w.Write([]byte(`{"id": 42, "body": "updated body"}`))
+	}))
+	defer server.Close()
+
+	client := NewClient(server.URL, "test-token")
+	err := client.EditComment(context.Background(), "owner", "repo", 42, "updated body")
+	if err != nil {
+		t.Fatalf("EditComment() error = %v", err)
+	}
+}
+
+func TestEditComment_Forbidden(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusForbidden)
+		w.Write([]byte(`{"message": "not allowed"}`))
+	}))
+	defer server.Close()
+
+	client := NewClient(server.URL, "test-token")
+	err := client.EditComment(context.Background(), "owner", "repo", 42, "new body")
+	if err == nil {
+		t.Fatal("expected error for 403 response")
+	}
+}
+
+func TestGetTimelineReviewCommentID(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.URL.Path != "/api/v1/repos/owner/repo/issues/5/timeline" {
+			t.Errorf("unexpected path: %s", r.URL.Path)
+		}
+		w.Write([]byte(`[
+			{"id": 100, "type": "comment", "body": "random"},
+			{"id": 200, "type": "review", "body": "other review <!-- review-bot:gpt -->"},
+			{"id": 300, "type": "review", "body": "our review <!-- review-bot:sonnet -->"}
+		]`))
+	}))
+	defer server.Close()
+
+	client := NewClient(server.URL, "test-token")
+	id, err := client.GetTimelineReviewCommentID(context.Background(), "owner", "repo", 5, "<!-- review-bot:sonnet -->")
+	if err != nil {
+		t.Fatalf("GetTimelineReviewCommentID() error = %v", err)
+	}
+	if id != 300 {
+		t.Errorf("got id=%d, want 300", id)
+	}
+}
+
+func TestGetTimelineReviewCommentID_NotFound(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Write([]byte(`[{"id": 100, "type": "review", "body": "no match"}]`))
+	}))
+	defer server.Close()
+
+	client := NewClient(server.URL, "test-token")
+	_, err := client.GetTimelineReviewCommentID(context.Background(), "owner", "repo", 5, "<!-- review-bot:sonnet -->")
+	if err == nil {
+		t.Fatal("expected error when sentinel not found")
+	}
+}