fix: address self-review findings on PR #90

- Remove unused error return from BuildPositionToLineMap (always nil)
- Add comment explaining intentional CommitID drop in PostReview
- Refactor TestAdapter_PostReview_WithComments to route by URL path
- Add TestAdapter_GetFileContent_RefRouting test
- Acknowledge maxPosition O(n) with code comment
- Remove redundant TestAdapter_CompileTimeCheck (compile-time var _ exists)
- Fix GetPullRequestFiles comment (Patch field is omitted, not 'set to empty')
- Acknowledge translateEvent fallback as intentional design

gitea/adapter.go

@@ -56,7 +56,7 @@ func (a *Adapter) GetPullRequestDiff(ctx context.Context, owner, repo string, nu
 }
 
 // GetPullRequestFiles maps []gitea.ChangedFile to []vcs.ChangedFile.
-// Patch is set to empty string since Gitea's /pulls/{n}/files does not return patch text.
+// Patch field is omitted (zero-value) since Gitea's /pulls/{n}/files does not return patch text.
 func (a *Adapter) GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]vcs.ChangedFile, error) {
 	files, err := a.client.GetPullRequestFiles(ctx, owner, repo, number)
 	if err != nil {
@@ -135,6 +135,9 @@ func translateEvent(event vcs.ReviewEvent) string {
 	case vcs.ReviewEventComment:
 		return "COMMENT"
 	default:
+		// Unknown events pass through as-is. This is intentional: new event types
+		// added to vcs.ReviewEvent will still be forwarded without a code change here,
+		// and Gitea will reject truly invalid values with a clear API error.
 		return string(event)
 	}
 }
@@ -153,16 +156,16 @@ func (a *Adapter) PostReview(ctx context.Context, owner, repo string, number int
 			return nil, fmt.Errorf("fetch diff for position translation: %w", err)
 		}
 
-		posMap, err := BuildPositionToLineMap(diff)
+		posMap := BuildPositionToLineMap(diff)
-		if err != nil {
-			return nil, fmt.Errorf("build position map: %w", err)
-		}
 
 		for _, c := range req.Comments {
 			lineNum, err := posMap.Translate(c.Path, c.Position)
 			if err != nil {
 				return nil, fmt.Errorf("translate position %d in %s: %w", c.Position, c.Path, err)
 			}
+			// CommitID from vcs.ReviewComment is intentionally not forwarded:
+			// Gitea review comments are pinned to the PR head SHA automatically,
+			// and the CreatePullReview API has no per-comment commit_id field.
 			giteaComments = append(giteaComments, ReviewComment{
 				Path:        c.Path,
 				NewPosition: int64(lineNum),

gitea/adapter_test.go

@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"net/http"
 	"net/http/httptest"
+	"strings"
 	"testing"
 
 	"gitea.weiker.me/rodin/review-bot/gitea"
@@ -229,30 +230,33 @@ func TestAdapter_PostReview_WithComments_PositionTranslation(t *testing.T) {
 		Body        string `json:"body"`
 	}
 
-	call := 0
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		call++
 		w.Header().Set("Content-Type", "application/json")
-		if call == 1 {
+		if strings.HasSuffix(r.URL.Path, ".diff") {
 			// Diff request
 			w.Write([]byte(diff))
 			return
 		}
-		// Review post
+		if strings.HasSuffix(r.URL.Path, "/reviews") {
-		var payload struct {
+			// Review post
-			Comments []struct {
+			var payload struct {
-				Path        string `json:"path"`
+				Comments []struct {
-				NewPosition int64  `json:"new_position"`
+					Path        string `json:"path"`
-				Body        string `json:"body"`
+					NewPosition int64  `json:"new_position"`
-			} `json:"comments"`
+					Body        string `json:"body"`
+				} `json:"comments"`
+			}
+			json.NewDecoder(r.Body).Decode(&payload)
+			gotComments = payload.Comments
+			json.NewEncoder(w).Encode(map[string]any{
+				"id":   1,
+				"body": "review",
+				"user": map[string]any{"login": "bot"},
+			})
+			return
 		}
-		json.NewDecoder(r.Body).Decode(&payload)
+		t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path)
-		gotComments = payload.Comments
+		w.WriteHeader(http.StatusNotFound)
-		json.NewEncoder(w).Encode(map[string]any{
-			"id":   1,
-			"body": "review",
-			"user": map[string]any{"login": "bot"},
-		})
 	}))
 	defer server.Close()
 
@@ -350,7 +354,36 @@ func TestAdapter_ListContents(t *testing.T) {
 	}
 }
 
-func TestAdapter_CompileTimeCheck(t *testing.T) {
+
-	// This is a compile-time assertion — if it compiles, the adapter satisfies vcs.Client
+func TestAdapter_GetFileContent_RefRouting(t *testing.T) {
-	var _ vcs.Client = (*gitea.Adapter)(nil)
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		// When ref is provided, the URL should contain ?ref=
+		if r.URL.RawQuery != "" && strings.Contains(r.URL.RawQuery, "ref=") {
+			w.Write([]byte("content-at-ref"))
+		} else {
+			w.Write([]byte("content-default"))
+		}
+	}))
+	defer server.Close()
+
+	client := gitea.NewClient(server.URL, "token")
+	adapter := gitea.NewAdapter(client)
+
+	// Empty ref → routes to GetFileContent (no ?ref= query param)
+	got, err := adapter.GetFileContent(context.Background(), "owner", "repo", "main.go", "")
+	if err != nil {
+		t.Fatalf("GetFileContent(ref=\"\"): %v", err)
+	}
+	if got != "content-default" {
+		t.Errorf("GetFileContent(ref=\"\") = %q, want %q", got, "content-default")
+	}
+
+	// Non-empty ref → routes to GetFileContentRef (with ?ref= query param)
+	got, err = adapter.GetFileContent(context.Background(), "owner", "repo", "main.go", "abc123")
+	if err != nil {
+		t.Fatalf("GetFileContent(ref=\"abc123\"): %v", err)
+	}
+	if got != "content-at-ref" {
+		t.Errorf("GetFileContent(ref=\"abc123\") = %q, want %q", got, "content-at-ref")
+	}
 }

gitea/position.go

@@ -53,6 +53,7 @@ func (pm *PositionMap) Translate(file string, position int) (int, error) {
 }
 
 // maxPosition returns the highest position number for a file.
+// O(n) per call — acceptable since deletion-line fallback is rare and n is small (typical hunk size).
 func (pm *PositionMap) maxPosition(file string) int {
 	max := 0
 	for pos := range pm.files[file] {
@@ -72,7 +73,7 @@ func (pm *PositionMap) maxPosition(file string) int {
 //   - A new @@ hunk within the same file continues incrementing (does not reset)
 //   - Position maps to the new file line number for additions and context lines
 //   - Deletion lines have a position but no new-file line number (stored as -1)
-func BuildPositionToLineMap(diff string) (*PositionMap, error) {
+func BuildPositionToLineMap(diff string) *PositionMap {
 	pm := &PositionMap{files: make(map[string]map[int]int)}
 
 	lines := strings.Split(diff, "\n")
@@ -153,7 +154,7 @@ func BuildPositionToLineMap(diff string) (*PositionMap, error) {
 		}
 	}
 
-	return pm, nil
+	return pm
 }
 
 // parseHunkStart extracts the new-file starting line number from a hunk header.

gitea/position_test.go

@@ -20,10 +20,7 @@ index abc..def 100644
 +added line
  context after
 `
-	pm, err := BuildPositionToLineMap(diff)
+	pm := BuildPositionToLineMap(diff)
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
 
 	tests := []struct {
 		pos      int
@@ -59,10 +56,7 @@ func TestBuildPositionToLineMap_MultipleHunks(t *testing.T) {
  	return
  }
 `
-	pm, err := BuildPositionToLineMap(diff)
+	pm := BuildPositionToLineMap(diff)
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
 
 	tests := []struct {
 		pos      int
@@ -100,10 +94,7 @@ func TestBuildPositionToLineMap_DeletionTargeted(t *testing.T) {
 -deleted
  line3
 `
-	pm, err := BuildPositionToLineMap(diff)
+	pm := BuildPositionToLineMap(diff)
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
 
 	// Position 3 is the deletion line "-deleted" — should map to nearest below
 	// Position 4 is " line3" which is new line 2
@@ -126,12 +117,9 @@ func TestBuildPositionToLineMap_DeletionAtEnd(t *testing.T) {
  line2
 -deleted at end
 `
-	pm, err := BuildPositionToLineMap(diff)
+	pm := BuildPositionToLineMap(diff)
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
 
-	_, err = pm.Translate("file.go", 4)
+	_, err := pm.Translate("file.go", 4)
 	if err == nil {
 		t.Error("expected error for deletion at end with no subsequent line")
 	}
@@ -147,10 +135,7 @@ new file mode 100644
 +
 +func init() {}
 `
-	pm, err := BuildPositionToLineMap(diff)
+	pm := BuildPositionToLineMap(diff)
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
 
 	tests := []struct {
 		pos      int
@@ -182,13 +167,10 @@ deleted file mode 100644
 -
 -func old() {}
 `
-	pm, err := BuildPositionToLineMap(diff)
+	pm := BuildPositionToLineMap(diff)
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
 
 	// Deleted file has no new-file lines; positions should error
-	_, err = pm.Translate("old.go", 2)
+	_, err := pm.Translate("old.go", 2)
 	if err == nil {
 		t.Error("expected error for deleted file position")
 	}
@@ -205,13 +187,10 @@ diff --git a/code.go b/code.go
 +// added
  func main() {}
 `
-	pm, err := BuildPositionToLineMap(diff)
+	pm := BuildPositionToLineMap(diff)
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
 
 	// Binary file should not be in the map
-	_, err = pm.Translate("image.png", 1)
+	_, err := pm.Translate("image.png", 1)
 	if err == nil {
 		t.Error("expected error for binary file")
 	}
@@ -235,13 +214,10 @@ func TestBuildPositionToLineMap_OutOfRange(t *testing.T) {
 -old
 +new
 `
-	pm, err := BuildPositionToLineMap(diff)
+	pm := BuildPositionToLineMap(diff)
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
 
 	// Position 0 is invalid
-	_, err = pm.Translate("file.go", 0)
+	_, err := pm.Translate("file.go", 0)
 	if err == nil {
 		t.Error("expected error for position 0")
 	}
@@ -275,10 +251,7 @@ diff --git a/b.go b/b.go
 +// file b
  func bFunc() {}
 `
-	pm, err := BuildPositionToLineMap(diff)
+	pm := BuildPositionToLineMap(diff)
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
 
 	// a.go: pos 3 is "+// file a" -> new line 2
 	got, err := pm.Translate("a.go", 3)