From 0ec5093aeb5a5821004e1d4421d6b1a430633e39 Mon Sep 17 00:00:00 2001 From: claw Date: Tue, 12 May 2026 13:49:36 -0700 Subject: [PATCH] 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 | 13 +++++--- gitea/adapter_test.go | 73 ++++++++++++++++++++++++++++++------------ gitea/position.go | 5 +-- gitea/position_test.go | 53 ++++++++---------------------- 4 files changed, 77 insertions(+), 67 deletions(-) diff --git a/gitea/adapter.go b/gitea/adapter.go index 20806b7..b3d8122 100644 --- a/gitea/adapter.go +++ b/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) - if err != nil { - return nil, fmt.Errorf("build position map: %w", err) - } + posMap := BuildPositionToLineMap(diff) 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), diff --git a/gitea/adapter_test.go b/gitea/adapter_test.go index 380e643..f272116 100644 --- a/gitea/adapter_test.go +++ b/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 - var payload struct { - Comments []struct { - Path string `json:"path"` - NewPosition int64 `json:"new_position"` - Body string `json:"body"` - } `json:"comments"` + if strings.HasSuffix(r.URL.Path, "/reviews") { + // Review post + var payload struct { + Comments []struct { + Path string `json:"path"` + NewPosition int64 `json:"new_position"` + 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) - gotComments = payload.Comments - json.NewEncoder(w).Encode(map[string]any{ - "id": 1, - "body": "review", - "user": map[string]any{"login": "bot"}, - }) + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotFound) })) 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 - var _ vcs.Client = (*gitea.Adapter)(nil) + +func TestAdapter_GetFileContent_RefRouting(t *testing.T) { + 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") + } } diff --git a/gitea/position.go b/gitea/position.go index ff84212..4be26b4 100644 --- a/gitea/position.go +++ b/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. diff --git a/gitea/position_test.go b/gitea/position_test.go index 843a0a8..743a2b0 100644 --- a/gitea/position_test.go +++ b/gitea/position_test.go @@ -20,10 +20,7 @@ index abc..def 100644 +added line context after ` - pm, err := BuildPositionToLineMap(diff) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } + pm := BuildPositionToLineMap(diff) tests := []struct { pos int @@ -59,10 +56,7 @@ func TestBuildPositionToLineMap_MultipleHunks(t *testing.T) { return } ` - pm, err := BuildPositionToLineMap(diff) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } + pm := BuildPositionToLineMap(diff) tests := []struct { pos int @@ -100,10 +94,7 @@ func TestBuildPositionToLineMap_DeletionTargeted(t *testing.T) { -deleted line3 ` - pm, err := BuildPositionToLineMap(diff) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } + pm := BuildPositionToLineMap(diff) // 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) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } + pm := BuildPositionToLineMap(diff) - _, 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) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } + pm := BuildPositionToLineMap(diff) tests := []struct { pos int @@ -182,13 +167,10 @@ deleted file mode 100644 - -func old() {} ` - pm, err := BuildPositionToLineMap(diff) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } + pm := BuildPositionToLineMap(diff) // 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) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } + pm := BuildPositionToLineMap(diff) // 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) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } + pm := BuildPositionToLineMap(diff) // 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) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } + pm := BuildPositionToLineMap(diff) // a.go: pos 3 is "+// file a" -> new line 2 got, err := pm.Translate("a.go", 3)