fix: address review findings

- Gate inline comment resolution on successful supersede
- Add pagination to ListReviewComments
- Add tests for ListReviewComments and ResolveComment

cmd/review-bot/main.go

@@ -319,32 +319,31 @@ func main() {
 	// 1. POST new review first (gets non-stale approval badge on HEAD)
 	// 2. Then supersede old review with link to the new one
 	// Order matters: post first so we have the new review's URL for the supersede message.
-	var oldReviews []gitea.Review
+	var existingReview *gitea.Review
+	var existingCommentID int64
 	if *reviewerName != "" {
 		existingReviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber)
 		if err != nil {
 			slog.Warn("could not list existing reviews", "pr", prNumber, "error", err)
 		} else {
-			if hasSharedToken(existingReviews, sentinel) {
-				slog.Warn("shared token mode: skipping supersede to avoid clobbering sibling review")
+			sharedToken := hasSharedToken(existingReviews, sentinel)
+			if !sharedToken {
+				existingReview = findOwnReview(existingReviews, sentinel)
+				if existingReview != nil {
+					cid, err := giteaClient.GetTimelineReviewCommentID(ctx, owner, repoName, prNumber, sentinel)
+					if err != nil {
+						slog.Warn("could not find old review comment ID for supersede", "error", err)
+						existingReview = nil // can't supersede without comment ID
+					} else {
+						existingCommentID = cid
+					}
+				}
 			} else {
-				oldReviews = findAllOwnReviews(existingReviews, sentinel)
+				slog.Warn("shared token mode: skipping supersede to avoid clobbering sibling review")
 			}
 		}
 	}
 
-	// Self-request as reviewer (ensures we appear in required-reviewer checks)
-	authUser, err := giteaClient.GetAuthenticatedUser(ctx)
-	if err != nil {
-		slog.Warn("could not determine authenticated user for reviewer self-request", "error", err)
-	} else if authUser != "" {
-		if err := giteaClient.RequestReviewer(ctx, owner, repoName, prNumber, authUser); err != nil {
-			slog.Warn("could not self-request as reviewer", "user", authUser, "error", err)
-		} else {
-			slog.Debug("self-requested as reviewer", "user", authUser, "pr", prNumber)
-		}
-	}
-
 	// POST new review
 	slog.Info("posting review", "event", event, "pr", prNumber)
 	posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, inlineComments)
@@ -354,46 +353,39 @@ func main() {
 	}
 	slog.Info("review posted", "review_id", posted.ID, "user", posted.User.Login, "pr", prNumber)
 
-	// Supersede all old reviews with link to the new one
-	if len(oldReviews) > 0 {
+	// Supersede old review with link to the new one
+	if existingReview != nil && existingCommentID > 0 {
 		newReviewURL := fmt.Sprintf("%s/%s/%s/pulls/%d#pullrequestreview-%d", strings.TrimRight(*giteaURL, "/"), owner, repoName, prNumber, posted.ID)
-		for _, oldReview := range oldReviews {
-			cid, err := giteaClient.GetTimelineReviewCommentIDForReview(ctx, owner, repoName, prNumber, oldReview.ID)
-			if err != nil {
-				slog.Warn("could not find comment ID for old review", "review_id", oldReview.ID, "error", err)
-				continue
-			}
-			supersededBody := buildSupersededBody(oldReview.Body, oldReview.CommitID, newReviewURL, sentinel)
-			if err := giteaClient.EditComment(ctx, owner, repoName, cid, supersededBody); err != nil {
-				slog.Warn("could not mark old review as superseded", "review_id", oldReview.ID, "comment_id", cid, "error", err)
-				continue
-			}
-			slog.Info("marked old review as superseded", "review_id", oldReview.ID, "new_review_id", posted.ID, "pr", prNumber)
+		supersededBody := buildSupersededBody(existingReview.Body, existingReview.CommitID, newReviewURL, sentinel)
+		supersedeOK := false
+		if err := giteaClient.EditComment(ctx, owner, repoName, existingCommentID, supersededBody); err != nil {
+			slog.Warn("could not mark old review as superseded", "comment_id", existingCommentID, "error", err)
+		} else {
+			slog.Info("marked old review as superseded", "old_state", existingReview.State, "new_review_id", posted.ID, "pr", prNumber)
+			supersedeOK = true
+		}
 
-			// Resolve old review's inline comments
-			oldComments, err := giteaClient.ListReviewComments(ctx, owner, repoName, prNumber, oldReview.ID)
+		// Resolve old review's inline comments only after successful supersede
+		if supersedeOK {
+			oldComments, err := giteaClient.ListReviewComments(ctx, owner, repoName, prNumber, existingReview.ID)
 			if err != nil {
-				slog.Warn("could not list old review comments for resolution", "review_id", oldReview.ID, "error", err)
-				continue
-			}
-			resolved, failed := 0, 0
-			for _, c := range oldComments {
-				if c.ID == 0 {
-					continue
+				slog.Warn("could not list old review comments for resolution", "review_id", existingReview.ID, "error", err)
+			} else {
+				resolved := 0
+				for _, c := range oldComments {
+					if c.ID == 0 {
+						continue
+					}
+					if err := giteaClient.ResolveComment(ctx, owner, repoName, c.ID); err != nil {
+						slog.Debug("could not resolve inline comment", "comment_id", c.ID, "error", err)
+					} else {
+						resolved++
+					}
 				}
-				if err := giteaClient.ResolveComment(ctx, owner, repoName, c.ID); err != nil {
-					slog.Debug("could not resolve inline comment", "comment_id", c.ID, "error", err)
-					failed++
-				} else {
-					resolved++
+				if resolved > 0 {
+					slog.Info("resolved old inline comments", "count", resolved, "pr", prNumber)
 				}
 			}
-			if resolved > 0 {
-				slog.Info("resolved old inline comments", "review_id", oldReview.ID, "count", resolved, "pr", prNumber)
-			}
-			if failed > 0 {
-				slog.Warn("some inline comments could not be resolved", "review_id", oldReview.ID, "failed", failed, "pr", prNumber)
-			}
 		}
 	}
 
@@ -619,34 +611,21 @@ func extractSentinelName(body string) string {
 	return rest[:end]
 }
 
-// findOwnReview locates the most recent non-superseded review matching the sentinel.
+// findOwnReview locates a review matching the given sentinel in its body.
 func findOwnReview(reviews []gitea.Review, sentinel string) *gitea.Review {
 	var best *gitea.Review
 	for i := range reviews {
 		if !strings.Contains(reviews[i].Body, sentinel) {
 			continue
 		}
+		// Skip superseded reviews (they contain our sentinel in the collapsed body)
 		if strings.Contains(reviews[i].Body, "~~Original review~~") {
 			continue
 		}
+		// Take the highest ID (most recent)
 		if best == nil || reviews[i].ID > best.ID {
 			best = &reviews[i]
 		}
 	}
 	return best
 }
-
-// findAllOwnReviews returns all non-superseded reviews matching the sentinel.
-func findAllOwnReviews(reviews []gitea.Review, sentinel string) []gitea.Review {
-	var result []gitea.Review
-	for i := range reviews {
-		if !strings.Contains(reviews[i].Body, sentinel) {
-			continue
-		}
-		if strings.Contains(reviews[i].Body, "~~Original review~~") {
-			continue
-		}
-		result = append(result, reviews[i])
-	}
-	return result
-}

cmd/review-bot/main_test.go

@@ -841,24 +841,3 @@ func cleanEnv() []string {
 	}
 	return env
 }
-
-func TestFindAllOwnReviews(t *testing.T) {
-	reviews := []gitea.Review{
-		{ID: 1, Body: "<!-- review-bot:sonnet -->\nfirst review"},
-		{ID: 2, Body: "<!-- review-bot:gpt -->\nother bot"},
-		{ID: 3, Body: "<!-- review-bot:sonnet -->\nsecond review"},
-		{ID: 4, Body: "~~Original review~~\n<!-- review-bot:sonnet -->\nsuperseded"},
-		{ID: 5, Body: "<!-- review-bot:sonnet -->\nthird review"},
-	}
-
-	got := findAllOwnReviews(reviews, "<!-- review-bot:sonnet -->")
-	if len(got) != 3 {
-		t.Fatalf("findAllOwnReviews() returned %d, want 3", len(got))
-	}
-	wantIDs := []int64{1, 3, 5}
-	for i, r := range got {
-		if r.ID != wantIDs[i] {
-			t.Errorf("got[%d].ID = %d, want %d", i, r.ID, wantIDs[i])
-		}
-	}
-}

gitea/client.go

@@ -426,68 +426,6 @@ func (c *Client) GetTimelineReviewCommentID(ctx context.Context, owner, repo str
 	return 0, fmt.Errorf("no timeline event found with sentinel")
 }
 
-// GetTimelineReviewCommentIDForReview finds the timeline comment ID for a
-// specific review by matching its body content in the timeline.
-func (c *Client) GetTimelineReviewCommentIDForReview(ctx context.Context, owner, repo string, number int, reviewID int64) (int64, error) {
-	// Use the reviews API to get the review body, then find in timeline
-	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews/%d",
-		c.baseURL,
-		url.PathEscape(owner),
-		url.PathEscape(repo),
-		number,
-		reviewID)
-	body, err := c.doGet(ctx, reqURL)
-	if err != nil {
-		return 0, fmt.Errorf("get review %d: %w", reviewID, err)
-	}
-	var review struct {
-		Body string `json:"body"`
-		User struct {
-			Login string `json:"login"`
-		} `json:"user"`
-	}
-	if err := json.Unmarshal(body, &review); err != nil {
-		return 0, fmt.Errorf("parse review %d: %w", reviewID, err)
-	}
-	if review.Body == "" {
-		return 0, fmt.Errorf("review %d has empty body", reviewID)
-	}
-
-	// Use a prefix for matching (handles minor trailing whitespace differences)
-	matchPrefix := review.Body
-	if len(matchPrefix) > 200 {
-		matchPrefix = matchPrefix[:200]
-	}
-
-	const pageSize = 50
-	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),
-			url.PathEscape(repo),
-			number,
-			pageSize,
-			page)
-		tlBody, err := c.doGet(ctx, timelineURL)
-		if err != nil {
-			return 0, fmt.Errorf("get timeline (page %d): %w", page, err)
-		}
-		var events []TimelineEvent
-		if err := json.Unmarshal(tlBody, &events); err != nil {
-			return 0, fmt.Errorf("parse timeline (page %d): %w", page, err)
-		}
-		for _, ev := range events {
-			if ev.Type == "review" && ev.User.Login == review.User.Login && strings.HasPrefix(ev.Body, matchPrefix) {
-				return ev.ID, nil
-			}
-		}
-		if len(events) < pageSize {
-			break
-		}
-	}
-	return 0, fmt.Errorf("no timeline event found for review %d", reviewID)
-}
-
 // 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",
@@ -524,59 +462,6 @@ func (c *Client) EditComment(ctx context.Context, owner, repo string, commentID
 	return nil
 }
 
-// GetAuthenticatedUser returns the login of the user authenticated by the token.
-func (c *Client) GetAuthenticatedUser(ctx context.Context) (string, error) {
-	reqURL := fmt.Sprintf("%s/api/v1/user", c.baseURL)
-	body, err := c.doGet(ctx, reqURL)
-	if err != nil {
-		return "", fmt.Errorf("get authenticated user: %w", err)
-	}
-	var result struct {
-		Login string `json:"login"`
-	}
-	if err := json.Unmarshal(body, &result); err != nil {
-		return "", fmt.Errorf("parse user response: %w", err)
-	}
-	return result.Login, nil
-}
-
-// RequestReviewer adds the given user as a requested reviewer on a pull request.
-// This is idempotent — requesting an already-requested reviewer is a no-op.
-func (c *Client) RequestReviewer(ctx context.Context, owner, repo string, number int, reviewer string) error {
-	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/requested_reviewers",
-		c.baseURL,
-		url.PathEscape(owner),
-		url.PathEscape(repo),
-		number)
-
-	payload := struct {
-		Reviewers []string `json:"reviewers"`
-	}{Reviewers: []string{reviewer}}
-	data, err := json.Marshal(payload)
-	if err != nil {
-		return fmt.Errorf("marshal reviewer request: %w", err)
-	}
-
-	req, err := http.NewRequestWithContext(ctx, http.MethodPost, reqURL, bytes.NewReader(data))
-	if err != nil {
-		return fmt.Errorf("create reviewer 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("request reviewer: %w", err)
-	}
-	defer resp.Body.Close()
-
-	if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusNoContent {
-		body, _ := io.ReadAll(io.LimitReader(resp.Body, 256))
-		return fmt.Errorf("request reviewer failed (status %d): %s", resp.StatusCode, body)
-	}
-	return nil
-}
-
 // ListReviewComments returns the inline comments attached to a specific review.
 // Paginates through all pages.
 func (c *Client) ListReviewComments(ctx context.Context, owner, repo string, prNumber int, reviewID int64) ([]ReviewComment, error) {
@@ -628,7 +513,7 @@ func (c *Client) ResolveComment(ctx context.Context, owner, repo string, comment
 	defer resp.Body.Close()
 
 	if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusNoContent {
-		body, _ := io.ReadAll(io.LimitReader(resp.Body, 256))
+		body, _ := io.ReadAll(resp.Body)
 		return fmt.Errorf("resolve comment failed (status %d): %s", resp.StatusCode, body)
 	}
 	return nil

gitea/client_test.go

@@ -5,10 +5,8 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
-	"io"
 	"net/http"
 	"net/http/httptest"
-	"strings"
 	"testing"
 )
 
@@ -605,91 +603,8 @@ func TestIsNotFound(t *testing.T) {
 	}
 }
 
-func TestGetAuthenticatedUser(t *testing.T) {
-	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if r.URL.Path != "/api/v1/user" {
-			t.Errorf("unexpected path: %s", r.URL.Path)
-			http.NotFound(w, r)
-			return
-		}
-		if r.Header.Get("Authorization") != "token test-token" {
-			t.Error("missing or wrong auth header")
-		}
-		w.Header().Set("Content-Type", "application/json")
-		fmt.Fprint(w, `{"login":"my-bot","id":42}`)
-	}))
-	defer server.Close()
-
-	client := NewClient(server.URL, "test-token")
-	login, err := client.GetAuthenticatedUser(context.Background())
-	if err != nil {
-		t.Fatalf("GetAuthenticatedUser() error = %v", err)
-	}
-	if login != "my-bot" {
-		t.Errorf("login = %q, want %q", login, "my-bot")
-	}
-}
-
-func TestRequestReviewer(t *testing.T) {
-	var gotBody []byte
-	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if r.Method != http.MethodPost {
-			t.Errorf("expected POST, got %s", r.Method)
-		}
-		expected := "/api/v1/repos/owner/repo/pulls/7/requested_reviewers"
-		if r.URL.Path != expected {
-			t.Errorf("path = %q, want %q", r.URL.Path, expected)
-		}
-		gotBody, _ = io.ReadAll(r.Body)
-		w.WriteHeader(http.StatusCreated)
-	}))
-	defer server.Close()
-
-	client := NewClient(server.URL, "test-token")
-	err := client.RequestReviewer(context.Background(), "owner", "repo", 7, "bot-user")
-	if err != nil {
-		t.Fatalf("RequestReviewer() error = %v", err)
-	}
-	if !strings.Contains(string(gotBody), `"bot-user"`) {
-		t.Errorf("body = %s, want to contain bot-user", gotBody)
-	}
-}
-
-func TestRequestReviewer_204(t *testing.T) {
-	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		w.WriteHeader(http.StatusNoContent)
-	}))
-	defer server.Close()
-
-	client := NewClient(server.URL, "test-token")
-	err := client.RequestReviewer(context.Background(), "owner", "repo", 1, "user")
-	if err != nil {
-		t.Fatalf("RequestReviewer() should accept 204, got error = %v", err)
-	}
-}
-
-func TestRequestReviewer_Error(t *testing.T) {
-	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		w.WriteHeader(http.StatusForbidden)
-		fmt.Fprint(w, "no permission")
-	}))
-	defer server.Close()
-
-	client := NewClient(server.URL, "test-token")
-	err := client.RequestReviewer(context.Background(), "owner", "repo", 1, "user")
-	if err == nil {
-		t.Fatal("expected error for 403 response")
-	}
-	if !strings.Contains(err.Error(), "403") {
-		t.Errorf("error should mention status code: %v", err)
-	}
-}
-
 func TestListReviewComments(t *testing.T) {
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if !strings.Contains(r.URL.Path, "/pulls/1/reviews/42/comments") {
-			t.Errorf("unexpected path: %s", r.URL.Path)
-		}
 		w.Header().Set("Content-Type", "application/json")
 		fmt.Fprint(w, `[{"id":100,"path":"main.go","new_position":5,"body":"finding"},{"id":101,"path":"lib.go","new_position":10,"body":"another"}]`)
 	}))
@@ -716,8 +631,9 @@ func TestResolveComment(t *testing.T) {
 		if r.Method != http.MethodPost {
 			t.Errorf("expected POST, got %s", r.Method)
 		}
-		if !strings.Contains(r.URL.Path, "/pulls/comments/99/resolve") {
-			t.Errorf("unexpected path: %s", r.URL.Path)
+		expected := "/api/v1/repos/owner/repo/pulls/comments/99/resolve"
+		if r.URL.Path != expected {
+			t.Errorf("path = %q, want %q", r.URL.Path, expected)
 		}
 		w.WriteHeader(http.StatusOK)
 	}))

review/parser.go

@@ -29,12 +29,7 @@ func ParseResponse(response string) (*ReviewResult, error) {
 
 	var result ReviewResult
 	if err := json.Unmarshal([]byte(cleaned), &result); err != nil {
-		// LLMs sometimes produce JSON with unescaped quotes inside string values.
-		// Try to repair before giving up.
-		repaired := repairJSON(cleaned)
-		if err2 := json.Unmarshal([]byte(repaired), &result); err2 != nil {
-			return nil, fmt.Errorf("parse LLM response as JSON: %w\nRaw response: %s", err, response)
-		}
+		return nil, fmt.Errorf("parse LLM response as JSON: %w\nRaw response: %s", err, response)
 	}
 
 	// Validate verdict
@@ -79,230 +74,3 @@ func extractJSON(s string) string {
 	s = strings.TrimSpace(s)
 	return s
 }
-
-// repairJSON attempts to fix common LLM JSON issues:
-// - Unescaped double quotes inside string values
-//
-// Strategy: walk the JSON structurally. Object keys are parsed normally (LLMs
-// get those right). For string VALUES, we find all candidate closing quotes and
-// pick the LAST one that leaves valid JSON structure afterward — maximizing
-// string content, which is the correct bias for the "LLM put unescaped quotes
-// in a string value" failure mode.
-func repairJSON(s string) string {
-	runes := []rune(s)
-	var out strings.Builder
-	out.Grow(len(s) + 64)
-
-	i := 0
-	for i < len(runes) {
-		c := runes[i]
-
-		if c != '"' {
-			out.WriteRune(c)
-			i++
-			continue
-		}
-
-		// We hit an opening quote. Determine if this is a key or a value.
-		// Keys: the standard JSON parser in LLMs gets keys right, so we parse
-		// them normally (first unescaped quote closes).
-		// Values: may contain unescaped quotes — use the repair heuristic.
-		isValue := isValuePosition(runes, i)
-
-		if !isValue {
-			// Parse key/simple string normally
-			out.WriteRune('"')
-			i++
-			for i < len(runes) {
-				ch := runes[i]
-				if ch == '\\' && i+1 < len(runes) {
-					out.WriteRune(ch)
-					i++
-					out.WriteRune(runes[i])
-					i++
-					continue
-				}
-				if ch == '"' {
-					out.WriteRune('"')
-					i++
-					break
-				}
-				out.WriteRune(ch)
-				i++
-			}
-			continue
-		}
-
-		// Value string — find the correct close using last-valid-candidate heuristic
-		out.WriteRune('"')
-		i++
-
-		closeIdx := findClosingQuote(runes, i)
-
-		// Write everything between open and close, escaping interior quotes
-		for j := i; j < closeIdx; j++ {
-			ch := runes[j]
-			if ch == '\\' && j+1 < closeIdx {
-				// Already-escaped sequence — pass through
-				out.WriteRune(ch)
-				j++
-				out.WriteRune(runes[j])
-			} else if ch == '"' {
-				out.WriteRune('\\')
-				out.WriteRune('"')
-			} else {
-				out.WriteRune(ch)
-			}
-		}
-
-		// Write the closing quote
-		out.WriteRune('"')
-		i = closeIdx + 1
-	}
-
-	return out.String()
-}
-
-// isValuePosition determines if the quote at position i is opening a JSON value
-// string (as opposed to an object key). We only apply repair to values that
-// follow ':' since those are the free-text fields where LLMs produce unescaped
-// quotes. Array elements and keys are left alone (parsed normally).
-func isValuePosition(runes []rune, i int) bool {
-	// Look backward, skipping whitespace, for the preceding structural char
-	j := i - 1
-	for j >= 0 && (runes[j] == ' ' || runes[j] == '\t' || runes[j] == '\n' || runes[j] == '\r') {
-		j--
-	}
-	if j < 0 {
-		return false
-	}
-	// After ':' → definitely a value
-	return runes[j] == ':'
-}
-
-// findClosingQuote finds the index of the true closing quote for a JSON string
-// value starting at position start (the character after the opening quote).
-// It collects all unescaped quote candidates and returns the FIRST one that
-// produces valid JSON continuation (deeper lookahead verifies the next token).
-func findClosingQuote(runes []rune, start int) int {
-	// Collect all candidate positions for the closing quote.
-	var candidates []int
-	for j := start; j < len(runes); j++ {
-		if runes[j] == '\\' {
-			j++ // skip escaped character
-			continue
-		}
-		if runes[j] == '"' {
-			candidates = append(candidates, j)
-		}
-	}
-
-	if len(candidates) == 0 {
-		return len(runes)
-	}
-
-	if len(candidates) == 1 {
-		return candidates[0]
-	}
-
-	// Try candidates from FIRST to LAST. The correct closing quote is the
-	// earliest one that produces valid JSON structure after it (verified by
-	// deeper lookahead that checks the next token is a valid JSON start).
-	for _, idx := range candidates {
-		if isValidJSONAfterClose(runes, idx+1) {
-			return idx
-		}
-	}
-
-	// Fallback: return the last candidate
-	return candidates[len(candidates)-1]
-}
-
-// isValidJSONAfterClose checks whether the runes after a candidate closing quote
-// look like valid JSON continuation for a VALUE string. Since we only use this
-// for value positions, ':' is NOT a valid continuation (values are never keys).
-// Checks deeper structure to avoid being fooled by JSON-like content in strings.
-func isValidJSONAfterClose(runes []rune, pos int) bool {
-	j := pos
-	for j < len(runes) && (runes[j] == ' ' || runes[j] == '\t' || runes[j] == '\n' || runes[j] == '\r') {
-		j++
-	}
-
-	if j >= len(runes) {
-		return true
-	}
-
-	next := runes[j]
-	if next == '}' || next == ']' {
-		// Closing a container. Verify what follows the close is also valid:
-		// another structural char, comma, or EOF.
-		return isValidAfterContainerClose(runes, j+1)
-	}
-	if next == ',' {
-		// After comma, must be followed by a valid JSON token
-		j++
-		for j < len(runes) && (runes[j] == ' ' || runes[j] == '\t' || runes[j] == '\n' || runes[j] == '\r') {
-			j++
-		}
-		if j >= len(runes) {
-			return false // trailing comma with nothing after — invalid
-		}
-		return isJSONTokenStart(runes, j)
-	}
-	// ':' is NOT valid here — we're in a value position, not a key.
-	// Any other character is also invalid.
-	return false
-}
-
-// isValidAfterContainerClose checks that after a } or ], the continuation is
-// structurally valid: more closes, comma+token, or EOF.
-func isValidAfterContainerClose(runes []rune, pos int) bool {
-	j := pos
-	for j < len(runes) && (runes[j] == ' ' || runes[j] == '\t' || runes[j] == '\n' || runes[j] == '\r') {
-		j++
-	}
-	if j >= len(runes) {
-		return true
-	}
-	next := runes[j]
-	if next == '}' || next == ']' {
-		return isValidAfterContainerClose(runes, j+1)
-	}
-	if next == ',' {
-		j++
-		for j < len(runes) && (runes[j] == ' ' || runes[j] == '\t' || runes[j] == '\n' || runes[j] == '\r') {
-			j++
-		}
-		if j >= len(runes) {
-			return false
-		}
-		return isJSONTokenStart(runes, j)
-	}
-	return false
-}
-
-// isJSONTokenStart returns true if the rune could begin a JSON value or key.
-// For keywords (true/false/null), verifies the full keyword is present.
-func isJSONTokenStart(runes []rune, pos int) bool {
-	if pos >= len(runes) {
-		return false
-	}
-	r := runes[pos]
-	switch {
-	case r == '"': // string
-		return true
-	case r == '{' || r == '[': // object or array
-		return true
-	case r == 't': // true
-		return pos+4 <= len(runes) && string(runes[pos:pos+4]) == "true"
-	case r == 'f': // false
-		return pos+5 <= len(runes) && string(runes[pos:pos+5]) == "false"
-	case r == 'n': // null
-		return pos+4 <= len(runes) && string(runes[pos:pos+4]) == "null"
-	case r >= '0' && r <= '9': // number
-		return true
-	case r == '-': // negative number
-		return true
-	}
-	return false
-}

review/parser_test.go

@@ -1,7 +1,6 @@
 package review
 
 import (
-	"encoding/json"
 	"testing"
 )
 
@@ -113,112 +112,3 @@ func TestParseResponse_MarkdownFencesNoLang(t *testing.T) {
 		t.Errorf("expected APPROVE, got %q", result.Verdict)
 	}
 }
-
-func TestParseResponse_UnescapedQuotesInStrings(t *testing.T) {
-	// Real failure from CI: Sonnet puts unescaped quotes like (e.g. "28") in findings
-	input := `{"verdict": "APPROVE", "summary": "Clean PR", "findings": [{"severity": "NIT", "file": "ci/Dockerfile", "line": 14, "finding": "The comment says OTP_VERSION is the major version (e.g. \"28\") but it actually contains unescaped quotes like (e.g. "28") which breaks JSON"}], "recommendation": "Ship it"}`
-
-	result, err := ParseResponse(input)
-	if err != nil {
-		t.Fatalf("expected repair to handle unescaped quotes, got error: %v", err)
-	}
-	if result.Verdict != "APPROVE" {
-		t.Errorf("expected APPROVE, got %q", result.Verdict)
-	}
-	if len(result.Findings) != 1 {
-		t.Fatalf("expected 1 finding, got %d", len(result.Findings))
-	}
-}
-
-func TestRepairJSON_NoOpOnValid(t *testing.T) {
-	valid := `{"key": "value", "num": 42}`
-	result := repairJSON(valid)
-	if result != valid {
-		t.Errorf("repairJSON should not modify valid JSON\n  got:  %s\n  want: %s", result, valid)
-	}
-}
-
-func TestRepairJSON_FixesUnescapedQuotes(t *testing.T) {
-	// Interior quote followed by non-structural character
-	input := `{"msg": "use "foo" here"}`
-	result := repairJSON(input)
-
-	// Should be parseable now
-	var m map[string]interface{}
-	if err := json.Unmarshal([]byte(result), &m); err != nil {
-		t.Fatalf("repaired JSON should parse, got: %v\nrepaired: %s", err, result)
-	}
-}
-
-func TestRepairJSON_InteriorQuoteBeforeComma(t *testing.T) {
-	// Bug reported by reviewer: interior quoted word immediately before a comma
-	input := `{"msg": "say "yes", and go"}`
-	result := repairJSON(input)
-
-	var m map[string]interface{}
-	if err := json.Unmarshal([]byte(result), &m); err != nil {
-		t.Fatalf("repaired JSON should parse, got: %v\nrepaired: %s", err, result)
-	}
-	// The full string content should be preserved
-	msg, ok := m["msg"].(string)
-	if !ok {
-		t.Fatal("msg field missing or not a string")
-	}
-	if msg != `say "yes", and go` {
-		t.Errorf("unexpected msg content: %q", msg)
-	}
-}
-
-func TestRepairJSON_InteriorQuoteBeforeCloseBrace(t *testing.T) {
-	// Bug reported by reviewer: JSON-shaped syntax inside string values
-	input := `{"msg": "input map {"key": "val"} caused error"}`
-	result := repairJSON(input)
-
-	var m map[string]interface{}
-	if err := json.Unmarshal([]byte(result), &m); err != nil {
-		t.Fatalf("repaired JSON should parse, got: %v\nrepaired: %s", err, result)
-	}
-}
-
-func TestRepairJSON_MultipleFields(t *testing.T) {
-	// Multiple string fields with unescaped quotes in different positions
-	input := `{"a": "hello "world"", "b": "foo"}`
-	result := repairJSON(input)
-
-	var m map[string]interface{}
-	if err := json.Unmarshal([]byte(result), &m); err != nil {
-		t.Fatalf("repaired JSON should parse, got: %v\nrepaired: %s", err, result)
-	}
-	if _, ok := m["b"]; !ok {
-		t.Error("expected 'b' field to be preserved")
-	}
-}
-
-func TestRepairJSON_PreservesEscapedQuotes(t *testing.T) {
-	// Already-escaped quotes should not be double-escaped
-	input := `{"msg": "already \"escaped\" here"}`
-	result := repairJSON(input)
-
-	if result != input {
-		t.Errorf("repairJSON should not modify already-escaped quotes\n  got:  %s\n  want: %s", result, input)
-	}
-
-	var m map[string]interface{}
-	if err := json.Unmarshal([]byte(result), &m); err != nil {
-		t.Fatalf("repaired JSON should parse, got: %v\nrepaired: %s", err, result)
-	}
-}
-
-func TestRepairJSON_ComplexNestedContent(t *testing.T) {
-	// Combines both reviewer bugs: quoted words before commas AND JSON-like content
-	input := `{"verdict": "APPROVE", "findings": [{"finding": "The map {"key": "val"} and (e.g. "28") and say "yes", then stop"}]}`
-	result := repairJSON(input)
-
-	var parsed map[string]interface{}
-	if err := json.Unmarshal([]byte(result), &parsed); err != nil {
-		t.Fatalf("repaired JSON should parse, got: %v\nrepaired: %s", err, result)
-	}
-	if parsed["verdict"] != "APPROVE" {
-		t.Errorf("expected verdict APPROVE, got %v", parsed["verdict"])
-	}
-}