feat(vcs): Gitea adapter with diff-position translation (Phase 2) #90

Merged
aweiker merged 4 commits from review-bot-issue-79 into feature/github-support 2026-05-13 00:18:06 +00:00
4 changed files with 77 additions and 67 deletions
Showing only changes of commit 0ec5093aeb - Show all commits
+8 -5
View File
2
@@ -56,7 +56,7 @@ func (a *Adapter) GetPullRequestDiff(ctx context.Context, owner, repo string, nu
} }
Review

[NIT] The comment for GetPullRequestFiles mentions 'Patch is set to empty string', but the code does not set a Patch field (only Filename and Status). If vcs.ChangedFile has no Patch field, adjust the comment for accuracy; if it does, consider explicitly documenting zero-value behavior rather than mentioning it here.

**[NIT]** The comment for GetPullRequestFiles mentions 'Patch is set to empty string', but the code does not set a Patch field (only Filename and Status). If vcs.ChangedFile has no Patch field, adjust the comment for accuracy; if it does, consider explicitly documenting zero-value behavior rather than mentioning it here.
// GetPullRequestFiles maps []gitea.ChangedFile to []vcs.ChangedFile. // 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) { func (a *Adapter) GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]vcs.ChangedFile, error) {
files, err := a.client.GetPullRequestFiles(ctx, owner, repo, number) files, err := a.client.GetPullRequestFiles(ctx, owner, repo, number)
if err != nil { if err != nil {
2
@@ -135,6 +135,9 @@ func translateEvent(event vcs.ReviewEvent) string {
case vcs.ReviewEventComment: case vcs.ReviewEventComment:
return "COMMENT" return "COMMENT"
default: 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.
Review

[NIT] translateEvent defaults to passing through unknown events. Consider failing fast (returning an error) on unknown events to surface misconfigurations earlier, unless pass-through is an intentional contract with clear downstream handling.

**[NIT]** translateEvent defaults to passing through unknown events. Consider failing fast (returning an error) on unknown events to surface misconfigurations earlier, unless pass-through is an intentional contract with clear downstream handling.
return string(event) return string(event)
} }
} }
2
@@ -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) 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 { for _, c := range req.Comments {
lineNum, err := posMap.Translate(c.Path, c.Position) lineNum, err := posMap.Translate(c.Path, c.Position)
if err != nil { if err != nil {
return nil, fmt.Errorf("translate position %d in %s: %w", c.Position, c.Path, err) 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{ giteaComments = append(giteaComments, ReviewComment{
Path: c.Path, Path: c.Path,
NewPosition: int64(lineNum), NewPosition: int64(lineNum),
1
+39 -6
View File
@@ -5,6 +5,7 @@ import (
"encoding/json" "encoding/json"
"net/http" "net/http"
"net/http/httptest" "net/http/httptest"
"strings"
"testing" "testing"
"gitea.weiker.me/rodin/review-bot/gitea" "gitea.weiker.me/rodin/review-bot/gitea"
4
@@ -229,15 +230,14 @@ func TestAdapter_PostReview_WithComments_PositionTranslation(t *testing.T) {
Body string `json:"body"` Body string `json:"body"`
} }
call := 0
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
call++
w.Header().Set("Content-Type", "application/json") w.Header().Set("Content-Type", "application/json")
if call == 1 { if strings.HasSuffix(r.URL.Path, ".diff") {
// Diff request // Diff request
w.Write([]byte(diff)) w.Write([]byte(diff))
return return
} }
if strings.HasSuffix(r.URL.Path, "/reviews") {
// Review post // Review post
var payload struct { var payload struct {
Comments []struct { Comments []struct {
@@ -253,6 +253,10 @@ func TestAdapter_PostReview_WithComments_PositionTranslation(t *testing.T) {
"body": "review", "body": "review",
"user": map[string]any{"login": "bot"}, "user": map[string]any{"login": "bot"},
}) })
return
}
t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path)
w.WriteHeader(http.StatusNotFound)
})) }))
defer server.Close() defer server.Close()
2
@@ -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")
}
} }
+3 -2
View File
1
@@ -53,6 +53,7 @@ func (pm *PositionMap) Translate(file string, position int) (int, error) {
} }
Review

[MINOR] The maxPosition helper shadows the builtin max identifier (available since Go 1.21). The local variable max inside the function will compile fine, but since the repo targets the latest stable Go, renaming to maxPos would avoid the shadowing and is more idiomatic.

**[MINOR]** The `maxPosition` helper shadows the builtin `max` identifier (available since Go 1.21). The local variable `max` inside the function will compile fine, but since the repo targets the latest stable Go, renaming to `maxPos` would avoid the shadowing and is more idiomatic.
// maxPosition returns the highest position number for a file. // 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 { func (pm *PositionMap) maxPosition(file string) int {
max := 0 max := 0
for pos := range pm.files[file] { for pos := range pm.files[file] {
2
@@ -72,7 +73,7 @@ func (pm *PositionMap) maxPosition(file string) int {
// - A new @@ hunk within the same file continues incrementing (does not reset) // - 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 // - 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) // - Deletion lines have a position but no new-file line number (stored as -1)
Review

[NIT] The maxPosition method is a one-liner that just indexes the map and is only called from Translate. It adds a layer of indirection without adding clarity or encapsulation. Inlining pm.maxPositions[file] directly in Translate would be marginally clearer, though this is entirely a style preference.

**[NIT]** The `maxPosition` method is a one-liner that just indexes the map and is only called from `Translate`. It adds a layer of indirection without adding clarity or encapsulation. Inlining `pm.maxPositions[file]` directly in `Translate` would be marginally clearer, though this is entirely a style preference.
func BuildPositionToLineMap(diff string) (*PositionMap, error) { func BuildPositionToLineMap(diff string) *PositionMap {
pm := &PositionMap{files: make(map[string]map[int]int)} pm := &PositionMap{files: make(map[string]map[int]int)}
lines := strings.Split(diff, "\n") lines := strings.Split(diff, "\n")
Review

[MINOR] BuildPositionToLineMap splits the entire diff into a slice of lines and constructs per-position maps for all files. On very large PR diffs, this may cause elevated memory/CPU usage and could be leveraged as a mild DoS vector if the bot is induced to comment on such PRs. Consider streaming parsing and/or enforcing size limits or early exits based on comment targets.

**[MINOR]** BuildPositionToLineMap splits the entire diff into a slice of lines and constructs per-position maps for all files. On very large PR diffs, this may cause elevated memory/CPU usage and could be leveraged as a mild DoS vector if the bot is induced to comment on such PRs. Consider streaming parsing and/or enforcing size limits or early exits based on comment targets.
4
@@ -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. // parseHunkStart extracts the new-file starting line number from a hunk header.
+13 -40
View File
@@ -20,10 +20,7 @@ index abc..def 100644
+added line +added line
context after context after
` `
pm, err := BuildPositionToLineMap(diff) pm := BuildPositionToLineMap(diff)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
tests := []struct { tests := []struct {
pos int pos int
@@ -59,10 +56,7 @@ func TestBuildPositionToLineMap_MultipleHunks(t *testing.T) {
return return
} }
` `
pm, err := BuildPositionToLineMap(diff) pm := BuildPositionToLineMap(diff)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
tests := []struct { tests := []struct {
pos int pos int
@@ -100,10 +94,7 @@ func TestBuildPositionToLineMap_DeletionTargeted(t *testing.T) {
-deleted -deleted
line3 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 3 is the deletion line "-deleted" — should map to nearest below
// Position 4 is " line3" which is new line 2 // Position 4 is " line3" which is new line 2
@@ -126,12 +117,9 @@ func TestBuildPositionToLineMap_DeletionAtEnd(t *testing.T) {
line2 line2
-deleted at end -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 { if err == nil {
t.Error("expected error for deletion at end with no subsequent line") t.Error("expected error for deletion at end with no subsequent line")
} }
@@ -147,10 +135,7 @@ new file mode 100644
+ +
+func init() {} +func init() {}
` `
pm, err := BuildPositionToLineMap(diff) pm := BuildPositionToLineMap(diff)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
tests := []struct { tests := []struct {
pos int pos int
@@ -182,13 +167,10 @@ deleted file mode 100644
- -
-func old() {} -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 // 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 { if err == nil {
t.Error("expected error for deleted file position") t.Error("expected error for deleted file position")
} }
@@ -205,13 +187,10 @@ diff --git a/code.go b/code.go
+// added +// added
func main() {} 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 // Binary file should not be in the map
_, err = pm.Translate("image.png", 1) _, err := pm.Translate("image.png", 1)
if err == nil { if err == nil {
t.Error("expected error for binary file") t.Error("expected error for binary file")
} }
@@ -235,13 +214,10 @@ func TestBuildPositionToLineMap_OutOfRange(t *testing.T) {
-old -old
+new +new
` `
pm, err := BuildPositionToLineMap(diff) pm := BuildPositionToLineMap(diff)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
// Position 0 is invalid // Position 0 is invalid
_, err = pm.Translate("file.go", 0) _, err := pm.Translate("file.go", 0)
if err == nil { if err == nil {
t.Error("expected error for position 0") t.Error("expected error for position 0")
} }
@@ -275,10 +251,7 @@ diff --git a/b.go b/b.go
+// file b +// file b
func bFunc() {} 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 // a.go: pos 3 is "+// file a" -> new line 2
got, err := pm.Translate("a.go", 3) got, err := pm.Translate("a.go", 3)