fix: consistent url.PathEscape across all Gitea client endpoints

Apply url.PathEscape to owner, repo, and sha path segments in all
methods that were previously interpolating raw values. Methods already
using PathEscape (ListReviews, DeleteReview, GetTimelineReviewCommentID,
EditComment) are unchanged.

This eliminates an inconsistency flagged in PRs #17, #20, and #22 and
prevents potential path-injection bugs for names with special characters.

Closes #24

cmd/review-bot/main.go

@@ -241,38 +241,103 @@ func main() {
 
 	sentinel := fmt.Sprintf("<!-- review-bot:%s -->", *reviewerName)
 
+	// Map findings to inline comments for lines present in the diff
+	diffRanges := gitea.ParseDiffNewLines(diff)
+	var inlineComments []gitea.ReviewComment
+	for _, f := range result.Findings {
+		if f.File != "" && f.Line > 0 && diffRanges.Contains(f.File, f.Line) {
+			inlineComments = append(inlineComments, gitea.ReviewComment{
+				Path:        f.File,
+				NewPosition: int64(f.Line),
+				Body:        fmt.Sprintf("**[%s]** %s", f.Severity, f.Finding),
+			})
+		}
+	}
+	if len(inlineComments) > 0 {
+		log.Printf("Attaching %d inline comments", len(inlineComments))
+	}
+
+	// --- 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 {
+			log.Printf("Warning: could not list existing reviews: %v", err)
+		} else {
+			// 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"
+			}
+
+			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 find review comment ID, falling back to new post: %v", err)
+					} else {
+						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)
-	posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody)
+	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
+	// Post-posting escalation: if we just posted APPROVED but a sibling
-	if *updateExisting && *reviewerName != "" {
+	// from the same user has REQUEST_CHANGES, mark ours as superseded and
+	// re-post as REQUEST_CHANGES. This handles the first-run case where
+	// we don't know our login until after posting.
+	if event == "APPROVED" && *updateExisting && *reviewerName != "" {
 		reviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber)
-		if err != nil {
+		if err == nil && shouldEscalate(reviews, posted.ID, posted.User.Login, sentinel) {
-			log.Printf("Warning: could not list existing reviews: %v", err)
+			log.Printf("Post-posting escalation: sibling has REQUEST_CHANGES")
-		} else {
+			// Mark our just-posted review as superseded
-			for _, r := range reviews {
+			commentID, err := giteaClient.GetTimelineReviewCommentID(ctx, owner, repoName, prNumber, sentinel)
-				if r.ID != posted.ID && r.User.Login == posted.User.Login && strings.Contains(r.Body, sentinel) {
+			if err == nil {
-					if err := giteaClient.DeleteReview(ctx, owner, repoName, prNumber, r.ID); err != nil {
+				supersededBody := fmt.Sprintf("~~*This review has been superseded by a newer review below.*~~\n\n%s", sentinel)
-						log.Printf("Warning: could not delete old review %d: %v", r.ID, err)
+				giteaClient.EditComment(ctx, owner, repoName, commentID, supersededBody)
-					} else {
-						log.Printf("Deleted stale review %d", r.ID)
 			}
-				}
+			// Re-post as REQUEST_CHANGES
-			}
+			_, err = giteaClient.PostReview(ctx, owner, repoName, prNumber, "REQUEST_CHANGES", reviewBody, inlineComments)
-
-			// 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)
 			if err != nil {
 				log.Printf("Warning: could not re-post as REQUEST_CHANGES: %v", err)
 			} else {
@@ -281,8 +346,6 @@ func main() {
 		}
 	}
 }
-	}
-}
 
 // fetchFileContext fetches the full content of modified files from the PR branch.
 func fetchFileContext(ctx context.Context, client *gitea.Client, owner, repo, ref string, files []gitea.ChangedFile) string {
@@ -438,14 +501,50 @@ func validateReviewerName(name string) error {
 	return nil
 }
 
-// shouldEscalate checks if the current APPROVED review should be escalated
+// shouldEscalate checks if any sibling bot review from the same user
-// to REQUEST_CHANGES because a sibling bot review (same user, different role)
+// (different sentinel, same token) has REQUEST_CHANGES.
-// already has REQUEST_CHANGES.
+// ownLogin is the bot user login; if empty, escalation check is skipped.
-func shouldEscalate(reviews []gitea.Review, postedID int64, postedLogin, ownSentinel string) bool {
+// 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
 		}
 	}
 	return false
 }
+
+// reviewUnchanged checks if an existing review with the same sentinel
+// already has identical body and state. Returns true if a re-post would
+// produce the same result (skip to preserve conversation threads).
+func reviewUnchanged(reviews []gitea.Review, newBody, newEvent, sentinel string) bool {
+	for _, r := range reviews {
+		if r.Stale {
+			continue
+		}
+		if !strings.Contains(r.Body, sentinel) {
+			continue
+		}
+		if r.State == newEvent && r.Body == newBody {
+			return true
+		}
+	}
+	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,10 +142,149 @@ 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)
 			}
 		})
 	}
 }
+
+func TestReviewUnchanged(t *testing.T) {
+	tests := []struct {
+		name     string
+		existing []gitea.Review
+		newBody  string
+		newEvent string
+		sentinel string
+		want     bool
+	}{
+		{
+			name:     "no existing review",
+			existing: nil,
+			newBody:  "new review",
+			newEvent: "APPROVED",
+			sentinel: "<!-- review-bot:sonnet -->",
+			want:     false,
+		},
+		{
+			name: "identical body and state",
+			existing: []gitea.Review{
+				makeReview(100, "bot", "APPROVED", false, "same body\n<!-- review-bot:sonnet -->"),
+			},
+			newBody:  "same body\n<!-- review-bot:sonnet -->",
+			newEvent: "APPROVED",
+			sentinel: "<!-- review-bot:sonnet -->",
+			want:     true,
+		},
+		{
+			name: "same body but different state",
+			existing: []gitea.Review{
+				makeReview(100, "bot", "APPROVED", false, "body\n<!-- review-bot:sonnet -->"),
+			},
+			newBody:  "body\n<!-- review-bot:sonnet -->",
+			newEvent: "REQUEST_CHANGES",
+			sentinel: "<!-- review-bot:sonnet -->",
+			want:     false,
+		},
+		{
+			name: "different body same state",
+			existing: []gitea.Review{
+				makeReview(100, "bot", "APPROVED", false, "old body\n<!-- review-bot:sonnet -->"),
+			},
+			newBody:  "new body\n<!-- review-bot:sonnet -->",
+			newEvent: "APPROVED",
+			sentinel: "<!-- review-bot:sonnet -->",
+			want:     false,
+		},
+		{
+			name: "stale review with same body (should still post)",
+			existing: []gitea.Review{
+				makeReview(100, "bot", "APPROVED", true, "same\n<!-- review-bot:sonnet -->"),
+			},
+			newBody:  "same\n<!-- review-bot:sonnet -->",
+			newEvent: "APPROVED",
+			sentinel: "<!-- review-bot:sonnet -->",
+			want:     false,
+		},
+		{
+			name: "different sentinel (not our review)",
+			existing: []gitea.Review{
+				makeReview(100, "bot", "APPROVED", false, "body\n<!-- review-bot:gpt -->"),
+			},
+			newBody:  "body\n<!-- review-bot:sonnet -->",
+			newEvent: "APPROVED",
+			sentinel: "<!-- review-bot:sonnet -->",
+			want:     false,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			got := reviewUnchanged(tc.existing, tc.newBody, tc.newEvent, tc.sentinel)
+			if got != tc.want {
+				t.Errorf("reviewUnchanged() = %v, want %v", got, tc.want)
+			}
+		})
+	}
+}
+
+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)
+				}
+			}
+		})
+	}
+}

docs/REVIEW_STRATEGY.md

@@ -0,0 +1,97 @@
+# Review Update Strategy
+
+review-bot uses an **edit-in-place** strategy for updating reviews. Reviews are never deleted — this preserves conversation threads on inline comments.
+
+## State Transition Diagram
+
+```mermaid
+stateDiagram-v2
+    [*] --> NoExistingReview: First run
+
+    NoExistingReview --> POST_Review: Generate findings + event
+    POST_Review --> PostEscalationCheck: event == APPROVED?
+
+    PostEscalationCheck --> Done: No sibling blocks
+    PostEscalationCheck --> Supersede_And_Repost: Sibling has REQUEST_CHANGES
+    Supersede_And_Repost --> Done: Posted as REQUEST_CHANGES
+
+    [*] --> ExistingReviewFound: Subsequent run (sentinel match)
+
+    ExistingReviewFound --> CheckEscalation: Determine final event
+    CheckEscalation --> CompareState: Apply worst-wins if needed
+
+    CompareState --> SameState: existing.state == new event
+    CompareState --> StateChange: existing.state != new event
+
+    SameState --> Skip: Body unchanged
+    SameState --> PatchBody: Body changed → PATCH in place
+
+    StateChange --> Escalate: APPROVED → REQUEST_CHANGES
+    StateChange --> Downgrade: REQUEST_CHANGES → APPROVED
+
+    Escalate --> Supersede: PATCH old body → "Superseded"
+    Supersede --> POST_New_RC: POST new REQUEST_CHANGES
+
+    Downgrade --> POST_New_Approve: POST new APPROVED (old stays intact)
+
+    Skip --> Done
+    PatchBody --> Done
+    POST_New_RC --> Done
+    POST_New_Approve --> Done
+```
+
+## Rules
+
+| Scenario | Action | Reason |
+|----------|--------|--------|
+| No existing review | POST new | First run |
+| Same state, same body | Skip | Nothing changed — preserve threads |
+| Same state, body changed | PATCH body | Update findings without losing threads |
+| APPROVED → REQUEST_CHANGES | Supersede old + POST new | Can always escalate; old APPROVED is no longer valid |
+| REQUEST_CHANGES → APPROVED | POST new APPROVED | Can't edit state; old REQUEST_CHANGES stays as historical record |
+| Sibling has REQUEST_CHANGES (worst-wins) | Escalate to REQUEST_CHANGES | PR must stay blocked if ANY reviewer blocks |
+
+## Key Constraints
+
+1. **Review state is immutable after POST** — Gitea has no API to change APPROVED ↔ REQUEST_CHANGES
+2. **Never delete reviews** — Deleting cascades to inline comments and reply threads
+3. **"Last review per user" wins** — Gitea uses the most recent review from a user for merge decisions
+4. **REQUEST_CHANGES reviews are never touched** — Their inline comments and threads are preserved as historical record
+5. **APPROVED reviews can be superseded** — When escalation is needed, mark old as superseded and POST new
+
+## Worst-Wins (Shared Token)
+
+When multiple reviewer roles share a token (e.g., `sonnet` and `security` both use `sonnet-review-bot`):
+
+```
+CI Matrix Run:
+  sonnet  → REQUEST_CHANGES (findings)
+  security → APPROVED (no security issues)
+                ↓
+  security sees sibling REQUEST_CHANGES
+                ↓
+  security escalates → REQUEST_CHANGES
+                ↓
+  PR stays blocked ✓
+```
+
+The **first-run case** (no existing review to read login from) uses a post-posting fallback:
+POST APPROVED → check siblings → if blocked, supersede own APPROVED → re-POST as REQUEST_CHANGES.
+
+## Edit Mechanism
+
+Reviews are edited via `PATCH /repos/{owner}/{repo}/issues/comments/{id}`:
+
+- **Review body**: ID obtained from the timeline API (`/issues/{index}/timeline`, type `"review"`)
+- **Inline comments**: IDs obtained from `/pulls/{index}/reviews/{id}/comments`
+- **Both are editable** by the token that created them
+- **ListReviews always returns the original body** (reads from review table, not comment table) — sentinel matching works regardless of edits
+
+## Inline Comments Lifecycle
+
+| Event | Inline comments behavior |
+|-------|--------------------------|
+| First POST | Created on specific diff lines |
+| PATCH body (same state) | Unchanged — still current findings |
+| Supersede (state change) | Old inline comments stay (readable but on outdated code) |
+| New POST after supersede | Fresh inline comments on current diff |

gitea/client.go

@@ -57,9 +57,16 @@ type ChangedFile struct {
 	Status   string `json:"status"`
 }
 
+// ReviewComment represents an inline comment to attach to a review.
+type ReviewComment struct {
+	Path        string `json:"path"`
+	NewPosition int64  `json:"new_position"`
+	Body        string `json:"body"`
+}
+
 // GetPullRequest fetches PR metadata.
 func (c *Client) GetPullRequest(ctx context.Context, owner, repo string, number int) (*PullRequest, error) {
-	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d", c.baseURL, owner, repo, number)
+	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
 	body, err := c.doGet(ctx, reqURL)
 	if err != nil {
 		return nil, fmt.Errorf("fetch PR: %w", err)
@@ -73,7 +80,7 @@ func (c *Client) GetPullRequest(ctx context.Context, owner, repo string, number
 
 // GetPullRequestDiff fetches the unified diff for a PR.
 func (c *Client) GetPullRequestDiff(ctx context.Context, owner, repo string, number int) (string, error) {
-	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d.diff", c.baseURL, owner, repo, number)
+	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d.diff", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
 	body, err := c.doGet(ctx, reqURL)
 	if err != nil {
 		return "", fmt.Errorf("fetch diff: %w", err)
@@ -83,7 +90,7 @@ func (c *Client) GetPullRequestDiff(ctx context.Context, owner, repo string, num
 
 // GetPullRequestFiles fetches the list of files changed in a PR.
 func (c *Client) GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]ChangedFile, error) {
-	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/files", c.baseURL, owner, repo, number)
+	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/files", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
 	body, err := c.doGet(ctx, reqURL)
 	if err != nil {
 		return nil, fmt.Errorf("fetch PR files: %w", err)
@@ -97,7 +104,7 @@ func (c *Client) GetPullRequestFiles(ctx context.Context, owner, repo string, nu
 
 // GetCommitStatuses fetches CI statuses for a commit SHA.
 func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string) ([]CommitStatus, error) {
-	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/commits/%s/statuses", c.baseURL, owner, repo, sha)
+	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/commits/%s/statuses", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), url.PathEscape(sha))
 	body, err := c.doGet(ctx, reqURL)
 	if err != nil {
 		return nil, fmt.Errorf("fetch commit statuses: %w", err)
@@ -111,7 +118,7 @@ func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string)
 
 // GetFileContent fetches a file from the default branch of a repo.
 func (c *Client) GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) {
-	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/raw/%s", c.baseURL, owner, repo, escapePath(filepath))
+	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/raw/%s", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), escapePath(filepath))
 	body, err := c.doGet(ctx, reqURL)
 	if err != nil {
 		return "", fmt.Errorf("fetch file %s: %w", filepath, err)
@@ -121,7 +128,7 @@ func (c *Client) GetFileContent(ctx context.Context, owner, repo, filepath strin
 
 // GetFileContentRef fetches a file from a specific ref (branch/tag/sha) in a repo.
 func (c *Client) GetFileContentRef(ctx context.Context, owner, repo, filepath, ref string) (string, error) {
-	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/raw/%s?ref=%s", c.baseURL, owner, repo, escapePath(filepath), url.QueryEscape(ref))
+	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/raw/%s?ref=%s", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), escapePath(filepath), url.QueryEscape(ref))
 	body, err := c.doGet(ctx, reqURL)
 	if err != nil {
 		return "", fmt.Errorf("fetch file %s@%s: %w", filepath, ref, err)
@@ -131,15 +138,18 @@ func (c *Client) GetFileContentRef(ctx context.Context, owner, repo, filepath, r
 
 // PostReview submits a review to a PR and returns the created review.
 // event should be "APPROVED" or "REQUEST_CHANGES".
-func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, event, body string) (*Review, error) {
+// comments are optional inline comments attached to specific lines.
-	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews", c.baseURL, owner, repo, number)
+func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, event, body string, comments []ReviewComment) (*Review, error) {
+	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
 
 	payload := struct {
 		Body     string          `json:"body"`
 		Event    string          `json:"event"`
+		Comments []ReviewComment `json:"comments,omitempty"`
 	}{
 		Body:     body,
 		Event:    event,
+		Comments: comments,
 	}
 
 	data, err := json.Marshal(payload)
@@ -220,9 +230,9 @@ type ContentEntry struct {
 func (c *Client) ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error) {
 	var reqURL string
 	if path == "" {
-		reqURL = fmt.Sprintf("%s/api/v1/repos/%s/%s/contents", c.baseURL, owner, repo)
+		reqURL = fmt.Sprintf("%s/api/v1/repos/%s/%s/contents", c.baseURL, url.PathEscape(owner), url.PathEscape(repo))
 	} else {
-		reqURL = fmt.Sprintf("%s/api/v1/repos/%s/%s/contents/%s", c.baseURL, owner, repo, escapePath(path))
+		reqURL = fmt.Sprintf("%s/api/v1/repos/%s/%s/contents/%s", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), escapePath(path))
 	}
 	body, err := c.doGet(ctx, reqURL)
 	if err != nil {
@@ -343,3 +353,81 @@ 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, err := json.Marshal(payload)
+	if err != nil {
+		return fmt.Errorf("marshal edit payload: %w", err)
+	}
+
+	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

@@ -128,7 +128,7 @@ func TestPostReview(t *testing.T) {
 	defer server.Close()
 
 	client := NewClient(server.URL, "test-token")
-	review, err := client.PostReview(context.Background(), "owner", "repo", 3, "APPROVED", "LGTM")
+	review, err := client.PostReview(context.Background(), "owner", "repo", 3, "APPROVED", "LGTM", nil)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
@@ -175,7 +175,7 @@ func TestPostReview_Non200(t *testing.T) {
 	defer server.Close()
 
 	client := NewClient(server.URL, "test-token")
-	_, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "test")
+	_, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "test", nil)
 	if err == nil {
 		t.Fatal("expected error for 403, got nil")
 	}
@@ -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")
+	}
+}

gitea/diff.go

@@ -0,0 +1,85 @@
+package gitea
+
+import (
+	"strconv"
+	"strings"
+)
+
+// DiffLineRanges maps filenames to the set of new-file line numbers present in the diff.
+type DiffLineRanges struct {
+	files map[string]map[int]bool
+}
+
+// Contains reports whether the given file+line is within the diff hunks.
+func (d *DiffLineRanges) Contains(file string, line int) bool {
+	if d == nil || d.files == nil {
+		return false
+	}
+	lines, ok := d.files[file]
+	if !ok {
+		return false
+	}
+	return lines[line]
+}
+
+// ParseDiffNewLines parses a unified diff and extracts the new-file line numbers
+// that appear in each hunk (both added and context lines).
+func ParseDiffNewLines(diff string) *DiffLineRanges {
+	result := &DiffLineRanges{files: make(map[string]map[int]bool)}
+
+	var currentFile string
+	var newLine int
+
+	for _, line := range strings.Split(diff, "\n") {
+		// Track current file from +++ header
+		if strings.HasPrefix(line, "+++ b/") {
+			currentFile = strings.TrimPrefix(line, "+++ b/")
+			if result.files[currentFile] == nil {
+				result.files[currentFile] = make(map[int]bool)
+			}
+			continue
+		}
+		if strings.HasPrefix(line, "+++ /dev/null") {
+			currentFile = ""
+			continue
+		}
+
+		// Parse hunk header: @@ -old,count +new,count @@ or @@ -old +new @@
+		if strings.HasPrefix(line, "@@") && currentFile != "" {
+			// Extract the +N part — handle both "+10,8" and "+1" forms
+			parts := strings.Split(line, "+")
+			if len(parts) >= 2 {
+				// Take everything before comma or space
+				numStr := parts[1]
+				if idx := strings.IndexAny(numStr, ", "); idx != -1 {
+					numStr = numStr[:idx]
+				}
+				n, err := strconv.Atoi(numStr)
+				if err == nil {
+					newLine = n
+				}
+			}
+			continue
+		}
+
+		if currentFile == "" {
+			continue
+		}
+
+		// Skip diff metadata lines
+		if strings.HasPrefix(line, "\\") {
+			continue
+		}
+
+		// Count lines in hunk
+		if strings.HasPrefix(line, "+") || strings.HasPrefix(line, " ") {
+			result.files[currentFile][newLine] = true
+			newLine++
+		} else if strings.HasPrefix(line, "-") {
+			// Removed lines don't advance new line counter
+			continue
+		}
+	}
+
+	return result
+}

gitea/diff_test.go

@@ -0,0 +1,115 @@
+package gitea
+
+import (
+	"testing"
+)
+
+func TestParseDiffLineRanges(t *testing.T) {
+	diff := `diff --git a/main.go b/main.go
+index abc1234..def5678 100644
+--- a/main.go
++++ b/main.go
+@@ -10,6 +10,8 @@ func main() {
+ 	fmt.Println("hello")
++	fmt.Println("new line 11")
++	fmt.Println("new line 12")
+ 	fmt.Println("existing")
+ }
+@@ -30,4 +32,5 @@ func other() {
+ 	return nil
++	// added at line 33
+ }
+diff --git a/util.go b/util.go
+new file mode 100644
+--- /dev/null
++++ b/util.go
+@@ -0,0 +1,5 @@
++package main
++
++func helper() string {
++	return "hi"
++}
+`
+
+	ranges := ParseDiffNewLines(diff)
+
+	// main.go should have lines 10-17 (first hunk) and 32-36 (second hunk)
+	if !ranges.Contains("main.go", 11) {
+		t.Error("expected main.go:11 to be in diff")
+	}
+	if !ranges.Contains("main.go", 12) {
+		t.Error("expected main.go:12 to be in diff")
+	}
+	if !ranges.Contains("main.go", 10) {
+		t.Error("expected main.go:10 to be in diff (context line)")
+	}
+	if !ranges.Contains("main.go", 33) {
+		t.Error("expected main.go:33 to be in diff")
+	}
+	if ranges.Contains("main.go", 25) {
+		t.Error("main.go:25 should NOT be in diff")
+	}
+
+	// util.go is entirely new, lines 1-5
+	if !ranges.Contains("util.go", 1) {
+		t.Error("expected util.go:1 to be in diff")
+	}
+	if !ranges.Contains("util.go", 5) {
+		t.Error("expected util.go:5 to be in diff")
+	}
+	if ranges.Contains("util.go", 6) {
+		t.Error("util.go:6 should NOT be in diff")
+	}
+
+	// Unknown file
+	if ranges.Contains("unknown.go", 1) {
+		t.Error("unknown.go should not be in diff")
+	}
+}
+
+func TestParseDiffNewLines_Empty(t *testing.T) {
+	ranges := ParseDiffNewLines("")
+	if ranges.Contains("any.go", 1) {
+		t.Error("empty diff should contain nothing")
+	}
+}
+
+func TestParseDiffNewLines_NoCommaHunk(t *testing.T) {
+	// Single-line hunks omit the comma: @@ -1 +1 @@
+	diff := `diff --git a/single.go b/single.go
+--- a/single.go
++++ b/single.go
+@@ -1 +1 @@
+-old line
++new line
+`
+	ranges := ParseDiffNewLines(diff)
+	if !ranges.Contains("single.go", 1) {
+		t.Error("expected single.go:1 to be in diff (no-comma hunk)")
+	}
+	if ranges.Contains("single.go", 2) {
+		t.Error("single.go:2 should NOT be in diff")
+	}
+}
+
+func TestParseDiffNewLines_NoNewlineMarker(t *testing.T) {
+	// "\ No newline at end of file" should not advance line counter
+	diff := `diff --git a/noeof.go b/noeof.go
+--- a/noeof.go
++++ b/noeof.go
+@@ -1,2 +1,2 @@
++line one
++line two
+\ No newline at end of file
+`
+	ranges := ParseDiffNewLines(diff)
+	if !ranges.Contains("noeof.go", 1) {
+		t.Error("expected noeof.go:1")
+	}
+	if !ranges.Contains("noeof.go", 2) {
+		t.Error("expected noeof.go:2")
+	}
+	if ranges.Contains("noeof.go", 3) {
+		t.Error("noeof.go:3 should NOT be in diff (no-newline marker)")
+	}
+}

gitea/post_review_comments_test.go

@@ -0,0 +1,88 @@
+package gitea
+
+import (
+	"context"
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+)
+
+func TestPostReview_WithComments(t *testing.T) {
+	var gotPayload struct {
+		Body     string `json:"body"`
+		Event    string `json:"event"`
+		Comments []struct {
+			Path        string `json:"path"`
+			NewPosition int64  `json:"new_position"`
+			Body        string `json:"body"`
+		} `json:"comments"`
+	}
+
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		json.NewDecoder(r.Body).Decode(&gotPayload)
+		w.Header().Set("Content-Type", "application/json")
+		w.WriteHeader(200)
+		json.NewEncoder(w).Encode(map[string]any{
+			"id":   99,
+			"body": gotPayload.Body,
+			"user": map[string]any{"login": "bot"},
+		})
+	}))
+	defer server.Close()
+
+	client := NewClient(server.URL, "test-token")
+	comments := []ReviewComment{
+		{Path: "main.go", NewPosition: 42, Body: "[MAJOR] Something bad"},
+		{Path: "util.go", NewPosition: 10, Body: "[MINOR] Style issue"},
+	}
+
+	_, err := client.PostReview(context.Background(), "owner", "repo", 1, "REQUEST_CHANGES", "summary", comments)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	if len(gotPayload.Comments) != 2 {
+		t.Fatalf("expected 2 comments, got %d", len(gotPayload.Comments))
+	}
+	if gotPayload.Comments[0].Path != "main.go" {
+		t.Errorf("expected path main.go, got %s", gotPayload.Comments[0].Path)
+	}
+	if gotPayload.Comments[0].NewPosition != 42 {
+		t.Errorf("expected new_position 42, got %d", gotPayload.Comments[0].NewPosition)
+	}
+	if gotPayload.Comments[1].Body != "[MINOR] Style issue" {
+		t.Errorf("unexpected body: %s", gotPayload.Comments[1].Body)
+	}
+}
+
+func TestPostReview_NilComments(t *testing.T) {
+	var gotPayload map[string]any
+
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		json.NewDecoder(r.Body).Decode(&gotPayload)
+		w.Header().Set("Content-Type", "application/json")
+		w.WriteHeader(200)
+		json.NewEncoder(w).Encode(map[string]any{
+			"id":   100,
+			"body": "test",
+			"user": map[string]any{"login": "bot"},
+		})
+	}))
+	defer server.Close()
+
+	client := NewClient(server.URL, "test-token")
+	_, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "all good", nil)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	// With nil comments, the field should be omitted (omitempty)
+	comments, ok := gotPayload["comments"]
+	if ok && comments != nil {
+		arr, isArr := comments.([]any)
+		if isArr && len(arr) > 0 {
+			t.Error("expected no comments in payload when nil passed")
+		}
+	}
+}