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
7 changed files with 1094 additions and 38 deletions
+232
View File
@@ -0,0 +1,232 @@
package gitea
import (
"context"
"fmt"
"gitea.weiker.me/rodin/review-bot/vcs"
)
// Adapter wraps a gitea.Client and satisfies the vcs.Client interface.
// It handles translation between GitHub-canonical diff positions and Gitea
// line numbers, and between canonical review event strings and Gitea-native values.
type Adapter struct {
client *Client
}
// Compile-time interface conformance assertion.
var _ vcs.Client = (*Adapter)(nil)
// NewAdapter creates a new Adapter wrapping the given gitea Client.
func NewAdapter(client *Client) *Adapter {
return &Adapter{client: client}
}
// Underlying returns the wrapped gitea.Client for Gitea-specific operations
// that have no vcs.Client equivalent (resolve comment, timeline, supersede flow).
func (a *Adapter) Underlying() *Client {
return a.client
}
// --- PRReader ---
Review

[NIT] The section comment // --- PRReader --- is not standard Go doc style. The patterns doc recommends # Section Name within package doc comments for sections. For in-file section dividers, plain comments are fine, but the --- style is unusual in Go codebases. Minor style deviation, not a blocking issue.

**[NIT]** The section comment `// --- PRReader ---` is not standard Go doc style. The patterns doc recommends `# Section Name` within package doc comments for sections. For in-file section dividers, plain comments are fine, but the `---` style is unusual in Go codebases. Minor style deviation, not a blocking issue.
// GetPullRequest maps gitea.PullRequest to vcs.PullRequest.
func (a *Adapter) GetPullRequest(ctx context.Context, owner, repo string, number int) (*vcs.PullRequest, error) {
pr, err := a.client.GetPullRequest(ctx, owner, repo, number)
if err != nil {
return nil, fmt.Errorf("get pull request: %w", err)
}
return &vcs.PullRequest{
Number: number,
Review

[MINOR] GetPullRequest does not wrap the upstream error with context. All other error returns in the adapter do wrap (e.g., PostReview wraps diff-fetch errors), but this one just returns nil, err directly. For consistency and debuggability, consider return nil, fmt.Errorf("get pull request: %w", err).

**[MINOR]** GetPullRequest does not wrap the upstream error with context. All other error returns in the adapter do wrap (e.g., PostReview wraps diff-fetch errors), but this one just returns `nil, err` directly. For consistency and debuggability, consider `return nil, fmt.Errorf("get pull request: %w", err)`.
Title: pr.Title,
Body: pr.Body,
Head: vcs.HeadRef{
SHA: pr.Head.Sha,
Ref: pr.Head.Ref,
},
Base: vcs.BaseRef{
Ref: pr.Base.Ref,
},
}, nil
}
// GetPullRequestDiff is a pass-through to the underlying client.
func (a *Adapter) GetPullRequestDiff(ctx context.Context, owner, repo string, number int) (string, error) {
return a.client.GetPullRequestDiff(ctx, owner, repo, number)
}
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.
// 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 {
return nil, err
}
result := make([]vcs.ChangedFile, len(files))
for i, f := range files {
result[i] = vcs.ChangedFile{
Filename: f.Filename,
Status: f.Status,
}
}
return result, nil
}
// GetFileContentAtRef is a pass-through to the underlying client.
func (a *Adapter) GetFileContentAtRef(ctx context.Context, owner, repo, path, ref string) (string, error) {
return a.client.GetFileContentAtRef(ctx, owner, repo, path, ref)
}
// GetCommitStatuses maps []gitea.CommitStatus to []vcs.CommitStatus.
func (a *Adapter) GetCommitStatuses(ctx context.Context, owner, repo, sha string) ([]vcs.CommitStatus, error) {
statuses, err := a.client.GetCommitStatuses(ctx, owner, repo, sha)
if err != nil {
return nil, err
}
result := make([]vcs.CommitStatus, len(statuses))
for i, s := range statuses {
result[i] = vcs.CommitStatus{
Status: s.Status,
Context: s.Context,
Description: s.Description,
TargetURL: s.TargetURL,
}
}
return result, nil
}
// --- FileReader ---
// GetFileContent delegates to the underlying client, routing to the ref-aware
// variant when ref is non-empty.
func (a *Adapter) GetFileContent(ctx context.Context, owner, repo, path, ref string) (string, error) {
if ref != "" {
return a.client.GetFileContentRef(ctx, owner, repo, path, ref)
}
return a.client.GetFileContent(ctx, owner, repo, path)
}
// ListContents maps []gitea.ContentEntry to []vcs.ContentEntry.
func (a *Adapter) ListContents(ctx context.Context, owner, repo, path string) ([]vcs.ContentEntry, error) {
entries, err := a.client.ListContents(ctx, owner, repo, path)
if err != nil {
return nil, err
}
result := make([]vcs.ContentEntry, len(entries))
for i, e := range entries {
result[i] = vcs.ContentEntry{
Name: e.Name,
Path: e.Path,
Type: e.Type,
}
}
return result, nil
}
// --- Reviewer ---
Review

[NIT] The section separator comments (// --- PRReader ---, // --- FileReader ---, // --- Reviewer ---, // --- Identity ---) are not idiomatic Go. The standard approach is to let the godoc comments on each method group naturally, or split into multiple files by concern. That said, this is a minor style issue.

**[NIT]** The section separator comments (`// --- PRReader ---`, `// --- FileReader ---`, `// --- Reviewer ---`, `// --- Identity ---`) are not idiomatic Go. The standard approach is to let the godoc comments on each method group naturally, or split into multiple files by concern. That said, this is a minor style issue.
// translateEvent translates a vcs.ReviewEvent (GitHub-canonical) to a Gitea-native event string.
func translateEvent(event vcs.ReviewEvent) string {
Review

[NIT] translateEvent falls back to string(event) for unknown values. If vcs.ReviewEvent is a closed set, consider validating and returning a clear error for unexpected events to avoid posting invalid event strings to Gitea.

**[NIT]** translateEvent falls back to string(event) for unknown values. If vcs.ReviewEvent is a closed set, consider validating and returning a clear error for unexpected events to avoid posting invalid event strings to Gitea.
switch event {
case vcs.ReviewEventApprove:
return "APPROVED"
case vcs.ReviewEventRequestChanges:
return "REQUEST_CHANGES"
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.
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)
}
}
security-review-bot marked this conversation as resolved
Review

[MINOR] PostReview fetches the full PR diff and processes it in memory without size limits. If an attacker can trigger reviews on very large diffs, this could lead to elevated memory/CPU usage (potential DoS). Consider enforcing a maximum acceptable diff size or streaming/limiting reads in the underlying HTTP client for successful responses.

**[MINOR]** PostReview fetches the full PR diff and processes it in memory without size limits. If an attacker can trigger reviews on very large diffs, this could lead to elevated memory/CPU usage (potential DoS). Consider enforcing a maximum acceptable diff size or streaming/limiting reads in the underlying HTTP client for successful responses.
// PostReview translates vcs.ReviewRequest to the Gitea-native format.
// It fetches the PR diff, builds a position-to-line map, and translates each
// ReviewComment.Position (GitHub diff-position) to a Gitea new_position (line number).
func (a *Adapter) PostReview(ctx context.Context, owner, repo string, number int, req vcs.ReviewRequest) (*vcs.Review, error) {
event := translateEvent(req.Event)
var giteaComments []ReviewComment
if len(req.Comments) > 0 {
Review

[MINOR] The PostReview method fetches the full PR diff unconditionally whenever there are comments. If the diff is large, this adds latency and memory pressure for every review with inline comments. This is a design decision that may be acceptable, but it's worth noting that the diff could be cached or passed in if this becomes a performance concern.

**[MINOR]** The `PostReview` method fetches the full PR diff unconditionally whenever there are comments. If the diff is large, this adds latency and memory pressure for every review with inline comments. This is a design decision that may be acceptable, but it's worth noting that the diff could be cached or passed in if this becomes a performance concern.
// Fetch diff to build position → line number map.
Outdated
Review

[MINOR] In PostReview, the CommitID field from vcs.ReviewComment is silently dropped when building giteaComments. The comment notes Gitea uses new_position (line number), but if the Gitea API also accepts a commit SHA on the review comment for anchoring, discarding it may cause comments to be placed on the wrong commit if the PR has been updated. At minimum, a comment explaining why CommitID is intentionally not forwarded would help future maintainers.

**[MINOR]** In `PostReview`, the `CommitID` field from `vcs.ReviewComment` is silently dropped when building `giteaComments`. The comment notes Gitea uses `new_position` (line number), but if the Gitea API also accepts a commit SHA on the review comment for anchoring, discarding it may cause comments to be placed on the wrong commit if the PR has been updated. At minimum, a comment explaining why `CommitID` is intentionally not forwarded would help future maintainers.
// The diff is fetched unconditionally when comments exist. This adds latency
// for reviews with inline comments but keeps the implementation simple — caching
// the diff across calls would add complexity for minimal gain since PostReview
// is called at most once per review cycle.
diff, err := a.client.GetPullRequestDiff(ctx, owner, repo, number)
if err != nil {
return nil, fmt.Errorf("fetch diff for position translation: %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),
Body: c.Body,
})
}
}
review, err := a.client.PostReview(ctx, owner, repo, number, event, req.Body, giteaComments)
if err != nil {
return nil, fmt.Errorf("post review: %w", err)
}
return &vcs.Review{
ID: review.ID,
Body: review.Body,
User: vcs.UserInfo{Login: review.User.Login},
State: review.State,
Stale: review.Stale,
CommitID: review.CommitID,
}, nil
Review

[MINOR] PostReview does not wrap the final a.client.PostReview error with context. If the upstream call fails, the error message gives no indication that this came from the adapter's PostReview path. Consider return nil, fmt.Errorf("post review: %w", err) for consistency with other error wrapping in the file.

**[MINOR]** PostReview does not wrap the final `a.client.PostReview` error with context. If the upstream call fails, the error message gives no indication that this came from the adapter's PostReview path. Consider `return nil, fmt.Errorf("post review: %w", err)` for consistency with other error wrapping in the file.
}
// ListReviews maps []gitea.Review to []vcs.Review.
func (a *Adapter) ListReviews(ctx context.Context, owner, repo string, number int) ([]vcs.Review, error) {
reviews, err := a.client.ListReviews(ctx, owner, repo, number)
if err != nil {
return nil, err
}
result := make([]vcs.Review, len(reviews))
for i, r := range reviews {
result[i] = vcs.Review{
ID: r.ID,
Body: r.Body,
User: vcs.UserInfo{Login: r.User.Login},
State: r.State,
Stale: r.Stale,
CommitID: r.CommitID,
}
}
return result, nil
}
// DeleteReview is a pass-through to the underlying client.
func (a *Adapter) DeleteReview(ctx context.Context, owner, repo string, number int, reviewID int64) error {
return a.client.DeleteReview(ctx, owner, repo, number, reviewID)
}
// DismissReview deletes the review. Gitea supports full deletion of any review state.
// The message parameter is intentionally unused — Gitea deletion has no dismissal message.
func (a *Adapter) DismissReview(ctx context.Context, owner, repo string, number int, reviewID int64, message string) error {
return a.client.DeleteReview(ctx, owner, repo, number, reviewID)
}
// --- Identity ---
// GetAuthenticatedUser is a pass-through to the underlying client.
func (a *Adapter) GetAuthenticatedUser(ctx context.Context) (string, error) {
return a.client.GetAuthenticatedUser(ctx)
}
+388
View File
@@ -0,0 +1,388 @@
package gitea_test
import (
"context"
"encoding/json"
"net/http"
"net/http/httptest"
"strings"
"testing"
"gitea.weiker.me/rodin/review-bot/gitea"
"gitea.weiker.me/rodin/review-bot/vcs"
)
func TestAdapter_GetPullRequest(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "application/json")
json.NewEncoder(w).Encode(map[string]any{
"title": "Test PR",
"body": "PR body",
"head": map[string]any{
"sha": "abc123",
"ref": "feature-branch",
},
"base": map[string]any{
"ref": "main",
},
})
}))
defer server.Close()
client := gitea.NewClient(server.URL, "token")
adapter := gitea.NewAdapter(client)
pr, err := adapter.GetPullRequest(context.Background(), "owner", "repo", 42)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if pr.Number != 42 {
t.Errorf("Number = %d, want 42", pr.Number)
}
if pr.Title != "Test PR" {
t.Errorf("Title = %q, want %q", pr.Title, "Test PR")
}
if pr.Body != "PR body" {
t.Errorf("Body = %q, want %q", pr.Body, "PR body")
}
if pr.Head.SHA != "abc123" {
t.Errorf("Head.SHA = %q, want %q", pr.Head.SHA, "abc123")
}
if pr.Head.Ref != "feature-branch" {
t.Errorf("Head.Ref = %q, want %q", pr.Head.Ref, "feature-branch")
}
if pr.Base.Ref != "main" {
t.Errorf("Base.Ref = %q, want %q", pr.Base.Ref, "main")
}
}
func TestAdapter_GetPullRequestFiles(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "application/json")
json.NewEncoder(w).Encode([]map[string]any{
{"filename": "main.go", "status": "modified"},
{"filename": "new.go", "status": "added"},
})
}))
defer server.Close()
client := gitea.NewClient(server.URL, "token")
adapter := gitea.NewAdapter(client)
files, err := adapter.GetPullRequestFiles(context.Background(), "owner", "repo", 1)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if len(files) != 2 {
t.Fatalf("got %d files, want 2", len(files))
}
if files[0].Filename != "main.go" || files[0].Status != "modified" {
t.Errorf("files[0] = %+v", files[0])
}
if files[1].Filename != "new.go" || files[1].Status != "added" {
t.Errorf("files[1] = %+v", files[1])
}
}
func TestAdapter_ListReviews(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "application/json")
json.NewEncoder(w).Encode([]map[string]any{
{
"id": 1,
"body": "LGTM",
"user": map[string]any{"login": "reviewer1"},
"state": "APPROVED",
"stale": false,
"commit_id": "abc123",
},
{
"id": 2,
"body": "Needs work",
"user": map[string]any{"login": "reviewer2"},
"state": "REQUEST_CHANGES",
"stale": true,
"commit_id": "def456",
},
})
}))
defer server.Close()
client := gitea.NewClient(server.URL, "token")
adapter := gitea.NewAdapter(client)
reviews, err := adapter.ListReviews(context.Background(), "owner", "repo", 1)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if len(reviews) != 2 {
t.Fatalf("got %d reviews, want 2", len(reviews))
}
if reviews[0].ID != 1 || reviews[0].Body != "LGTM" || reviews[0].User.Login != "reviewer1" {
t.Errorf("reviews[0] = %+v", reviews[0])
}
if reviews[0].State != "APPROVED" || reviews[0].Stale || reviews[0].CommitID != "abc123" {
t.Errorf("reviews[0] state/stale/commit = %v/%v/%v", reviews[0].State, reviews[0].Stale, reviews[0].CommitID)
}
if reviews[1].ID != 2 || !reviews[1].Stale || reviews[1].State != "REQUEST_CHANGES" {
t.Errorf("reviews[1] = %+v", reviews[1])
}
}
func TestAdapter_GetCommitStatuses(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "application/json")
json.NewEncoder(w).Encode([]map[string]any{
{
"status": "success",
"context": "ci/test",
"description": "All tests pass",
"target_url": "https://ci.example.com/1",
},
})
}))
defer server.Close()
client := gitea.NewClient(server.URL, "token")
adapter := gitea.NewAdapter(client)
statuses, err := adapter.GetCommitStatuses(context.Background(), "owner", "repo", "abc123")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if len(statuses) != 1 {
t.Fatalf("got %d statuses, want 1", len(statuses))
}
if statuses[0].Status != "success" {
t.Errorf("Status = %q, want %q", statuses[0].Status, "success")
}
if statuses[0].Context != "ci/test" {
t.Errorf("Context = %q, want %q", statuses[0].Context, "ci/test")
}
if statuses[0].Description != "All tests pass" {
t.Errorf("Description = %q, want %q", statuses[0].Description, "All tests pass")
}
if statuses[0].TargetURL != "https://ci.example.com/1" {
t.Errorf("TargetURL = %q, want %q", statuses[0].TargetURL, "https://ci.example.com/1")
}
}
func TestAdapter_PostReview_EventTranslation(t *testing.T) {
Review

[NIT] The unified diff fixture in TestAdapter_PostReview_WithComments_PositionTranslation includes a blank context line without a leading space. Real unified diffs prefix context lines (including blank ones) with a single space. Updating the fixture to reflect this would better mirror real-world input.

**[NIT]** The unified diff fixture in TestAdapter_PostReview_WithComments_PositionTranslation includes a blank context line without a leading space. Real unified diffs prefix context lines (including blank ones) with a single space. Updating the fixture to reflect this would better mirror real-world input.
tests := []struct {
name string
event vcs.ReviewEvent
wantEvent string
}{
{"APPROVE becomes APPROVED", vcs.ReviewEventApprove, "APPROVED"},
{"REQUEST_CHANGES stays", vcs.ReviewEventRequestChanges, "REQUEST_CHANGES"},
{"COMMENT stays", vcs.ReviewEventComment, "COMMENT"},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Review

[NIT] The call counter in TestAdapter_PostReview_WithComments_PositionTranslation assumes the diff fetch is always request #1 and the review POST is always request #2. This is a fragile ordering assumption — if the underlying client is ever refactored to make requests in a different order or adds a request, the test will silently misbehave. Using URL path inspection (r.URL.Path) to route requests would be more robust.

**[NIT]** The `call` counter in `TestAdapter_PostReview_WithComments_PositionTranslation` assumes the diff fetch is always request #1 and the review POST is always request #2. This is a fragile ordering assumption — if the underlying client is ever refactored to make requests in a different order or adds a request, the test will silently misbehave. Using URL path inspection (`r.URL.Path`) to route requests would be more robust.
var gotEvent string
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
Review

[NIT] The comment // Wait: first hunk has pos 1(@@ hdr), 2(" line1"), 3("-old"), 4("+new") is a left-over from the author working through the arithmetic. The 'Wait:' prefix makes it look like a mistake in the comment itself rather than an explanatory note. It should be rephrased or removed.

**[NIT]** The comment `// Wait: first hunk has pos 1(@@ hdr), 2(" line1"), 3("-old"), 4("+new")` is a left-over from the author working through the arithmetic. The 'Wait:' prefix makes it look like a mistake in the comment itself rather than an explanatory note. It should be rephrased or removed.
w.Header().Set("Content-Type", "application/json")
var payload struct {
Event string `json:"event"`
}
json.NewDecoder(r.Body).Decode(&payload)
gotEvent = payload.Event
json.NewEncoder(w).Encode(map[string]any{
"id": 1,
"body": "test",
"user": map[string]any{"login": "bot"},
})
}))
defer server.Close()
client := gitea.NewClient(server.URL, "token")
adapter := gitea.NewAdapter(client)
_, err := adapter.PostReview(context.Background(), "owner", "repo", 1, vcs.ReviewRequest{
Body: "test",
Event: tt.event,
// No comments → no diff fetch needed
})
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if gotEvent != tt.wantEvent {
t.Errorf("event = %q, want %q", gotEvent, tt.wantEvent)
}
})
}
}
func TestAdapter_PostReview_WithComments_PositionTranslation(t *testing.T) {
diff := `diff --git a/main.go b/main.go
--- a/main.go
Review

[NIT] The TestAdapter_PostReview_WithComments_PositionTranslation test comment says 'Position 4 in this diff is "+// new comment at line 3" → new line 3'. The diff has @@ -1,3 +1,4 @@ starting at new line 1, so: pos 1 = @@ header, pos 2 = package main (line 1), pos 3 = (blank, line 2), pos 4 = +// new comment at line 3 (line 3), pos 5 = func main() {} (line 4). The comment and assertion (new_position = 3) are correct, but the blank line between package main and the addition could cause confusion. The test is correct as written.

**[NIT]** The `TestAdapter_PostReview_WithComments_PositionTranslation` test comment says 'Position 4 in this diff is "+// new comment at line 3" → new line 3'. The diff has `@@ -1,3 +1,4 @@` starting at new line 1, so: pos 1 = @@ header, pos 2 = ` package main` (line 1), pos 3 = ` ` (blank, line 2), pos 4 = `+// new comment at line 3` (line 3), pos 5 = ` func main() {}` (line 4). The comment and assertion (new_position = 3) are correct, but the blank line between `package main` and the addition could cause confusion. The test is correct as written.
+++ b/main.go
@@ -1,3 +1,4 @@
package main
+// new comment at line 3
func main() {}
`
var gotComments []struct {
Path string `json:"path"`
NewPosition int64 `json:"new_position"`
Body string `json:"body"`
}
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "application/json")
if strings.HasSuffix(r.URL.Path, ".diff") {
// Diff request
w.Write([]byte(diff))
return
}
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
}
t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path)
w.WriteHeader(http.StatusNotFound)
}))
defer server.Close()
client := gitea.NewClient(server.URL, "token")
adapter := gitea.NewAdapter(client)
// Position 4 in this diff is "+// new comment at line 3" → new line 3
_, err := adapter.PostReview(context.Background(), "owner", "repo", 1, vcs.ReviewRequest{
Body: "review",
Event: vcs.ReviewEventRequestChanges,
Comments: []vcs.ReviewComment{
{
Path: "main.go",
Position: 4,
CommitID: "abc123",
Body: "needs fix",
},
},
})
if err != nil {
t.Fatalf("unexpected error: %v", err)
Review

[NIT] There is a blank line between TestAdapter_ListContents and TestAdapter_GetFileContent_RefRouting that is inconsistent with the rest of the file. Minor formatting issue that gofmt would not catch (extra blank line between top-level functions is still valid Go).

**[NIT]** There is a blank line between `TestAdapter_ListContents` and `TestAdapter_GetFileContent_RefRouting` that is inconsistent with the rest of the file. Minor formatting issue that `gofmt` would not catch (extra blank line between top-level functions is still valid Go).
}
if len(gotComments) != 1 {
t.Fatalf("got %d comments, want 1", len(gotComments))
}
if gotComments[0].Path != "main.go" {
t.Errorf("path = %q, want %q", gotComments[0].Path, "main.go")
}
if gotComments[0].NewPosition != 3 {
t.Errorf("new_position = %d, want 3", gotComments[0].NewPosition)
}
if gotComments[0].Body != "needs fix" {
t.Errorf("body = %q, want %q", gotComments[0].Body, "needs fix")
}
}
func TestAdapter_DismissReview(t *testing.T) {
var deleteCalled bool
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.Method == http.MethodDelete {
deleteCalled = true
w.WriteHeader(204)
return
}
w.WriteHeader(404)
}))
defer server.Close()
client := gitea.NewClient(server.URL, "token")
adapter := gitea.NewAdapter(client)
err := adapter.DismissReview(context.Background(), "owner", "repo", 1, 99, "stale review")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if !deleteCalled {
t.Error("expected delete to be called")
}
}
func TestAdapter_Underlying(t *testing.T) {
client := gitea.NewClient("http://example.com", "token")
adapter := gitea.NewAdapter(client)
if adapter.Underlying() != client {
t.Error("Underlying() should return the wrapped client")
}
}
func TestAdapter_ListContents(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "application/json")
json.NewEncoder(w).Encode([]map[string]any{
{"name": "main.go", "path": "src/main.go", "type": "file"},
{"name": "util", "path": "src/util", "type": "dir"},
})
}))
defer server.Close()
client := gitea.NewClient(server.URL, "token")
adapter := gitea.NewAdapter(client)
entries, err := adapter.ListContents(context.Background(), "owner", "repo", "src")
if err != nil {
t.Fatalf("unexpected error: %v", err)
Review

[NIT] TestAdapter_CompileTimeCheck is redundant — the same compile-time assertion already exists in gitea/conformance_test.go and vcs/check_test.go. A test function that only contains a compile-time var declaration adds no value as a Test* function (it can never fail at runtime). The assertion is already covered; this can be removed.

**[NIT]** `TestAdapter_CompileTimeCheck` is redundant — the same compile-time assertion already exists in `gitea/conformance_test.go` and `vcs/check_test.go`. A test function that only contains a compile-time var declaration adds no value as a `Test*` function (it can never fail at runtime). The assertion is already covered; this can be removed.
}
if len(entries) != 2 {
t.Fatalf("got %d entries, want 2", len(entries))
}
if entries[0].Name != "main.go" || entries[0].Type != "file" {
t.Errorf("entries[0] = %+v", entries[0])
}
if entries[1].Name != "util" || entries[1].Type != "dir" {
t.Errorf("entries[1] = %+v", entries[1])
}
}
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")
}
}
+3
View File
@@ -86,6 +86,9 @@ type PullRequest struct {
Sha string `json:"sha"`
Ref string `json:"ref"`
} `json:"head"`
Base struct {
Ref string `json:"ref"`
} `json:"base"`
}
// CommitStatus represents a single CI status entry.
+3 -18
View File
@@ -1,5 +1,3 @@
//go:build phase2
package gitea_test
import (
@@ -7,19 +5,6 @@ import (
"gitea.weiker.me/rodin/review-bot/vcs"
)
// Compile-time interface conformance assertions.
// These will verify gitea.Client satisfies vcs interfaces once the Phase 2
// adapter bridges the method signature gaps:
//
// - PRReader: GetPullRequest returns *gitea.PullRequest (needs *vcs.PullRequest)
// - PRReader: GetPullRequestFiles returns []gitea.ChangedFile (needs []vcs.ChangedFile)
// - FileReader: GetFileContent lacks ref parameter
// - Reviewer: PostReview uses (event, body, comments) instead of vcs.ReviewRequest
//
// Remove the phase2 build tag once the adapter is complete.
var (
_ vcs.PRReader = (*gitea.Client)(nil)
_ vcs.FileReader = (*gitea.Client)(nil)
_ vcs.Reviewer = (*gitea.Client)(nil)
_ vcs.Identity = (*gitea.Client)(nil)
)
// Compile-time interface conformance assertion.
// The Adapter (not the raw Client) satisfies the full vcs.Client interface.
var _ vcs.Client = (*gitea.Adapter)(nil)
+190
View File
@@ -0,0 +1,190 @@
package gitea
import (
"fmt"
"strconv"
"strings"
)
// PositionMap holds a per-file mapping of GitHub diff-position to new-file line number.
// Position is a 1-indexed offset from the @@ hunk header line in the unified diff.
type PositionMap struct {
// files maps filename → (position → new-file line number).
// Deletion lines are mapped to -1 (no new-file line).
files map[string]map[int]int
// maxPositions caches the highest position number per file,
// tracked during construction to avoid O(n) scans at translate time.
maxPositions map[string]int
}
// Translate converts a GitHub diff-position to a new-file line number for a given file.
// Returns an error if the file is not in the diff or the position is out of range.
// If the position targets a deletion line, it maps to the nearest non-deletion line below;
// if no such line exists, returns an error.
func (pm *PositionMap) Translate(file string, position int) (int, error) {
if pm == nil || pm.files == nil {
return 0, fmt.Errorf("empty position map")
}
fileMap, ok := pm.files[file]
if !ok {
return 0, fmt.Errorf("file %q not found in diff", file)
}
if position < 1 {
return 0, fmt.Errorf("position %d out of range (must be >= 1)", position)
}
lineNum, ok := fileMap[position]
if !ok {
return 0, fmt.Errorf("position %d out of range for file %q", position, file)
}
// lineNum == -1 means this position is a deletion line.
// Map to the nearest non-deletion line below.
if lineNum == -1 {
maxPos := pm.maxPosition(file)
for p := position + 1; p <= maxPos; p++ {
if ln, exists := fileMap[p]; exists && ln > 0 {
return ln, nil
Review

[MINOR] maxPosition scans the entire file position map on each deletion-line translation (O(n)). This is acceptable for small hunks but could be optimized by tracking the max position during map construction.

**[MINOR]** maxPosition scans the entire file position map on each deletion-line translation (O(n)). This is acceptable for small hunks but could be optimized by tracking the max position during map construction.
}
}
return 0, fmt.Errorf("position %d targets a deletion line with no subsequent new-file line in %q", position, file)
}
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.
return lineNum, nil
}
// maxPosition returns the highest position number for a file.
// O(1) — the maximum is tracked during map construction.
func (pm *PositionMap) maxPosition(file string) int {
Review

[NIT] The maxPosition helper iterates over the map on every call to Translate for deletion lines. For large diffs this is O(n) per deletion-line translation. The position counter is already available during BuildPositionToLineMap — storing it per-file alongside the map would make Translate O(1). Not a correctness issue, but worth noting for large PRs.

**[NIT]** The `maxPosition` helper iterates over the map on every call to `Translate` for deletion lines. For large diffs this is O(n) per deletion-line translation. The position counter is already available during `BuildPositionToLineMap` — storing it per-file alongside the map would make `Translate` O(1). Not a correctness issue, but worth noting for large PRs.
Review

[MINOR] The doc comment states the '@@' hunk header is position 1, and the implementation increments position for '@@', but Translate never maps position 1 to any line and will return an out-of-range error for position=1. Consider clarifying in comments that only content lines (context/additions/deletions) are translatable, or explicitly handling header positions if they can occur in inputs.

**[MINOR]** The doc comment states the '@@' hunk header is position 1, and the implementation increments position for '@@', but Translate never maps position 1 to any line and will return an out-of-range error for position=1. Consider clarifying in comments that only content lines (context/additions/deletions) are translatable, or explicitly handling header positions if they can occur in inputs.
return pm.maxPositions[file]
}
// BuildPositionToLineMap parses a unified diff and builds a PositionMap
// mapping diff-position → new-file line number per file.
//
// Diff-position counting rules (GitHub spec):
// - The @@ hunk header line is position 1 for the file's first hunk
// - Every subsequent line increments position by 1 — context, additions, AND deletions
// - 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 {
pm := &PositionMap{
files: make(map[string]map[int]int),
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.
maxPositions: make(map[string]int),
}
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.
var currentFile string
Review

[MINOR] The BuildPositionToLineMap function never returns a non-nil error — all error paths are deferred to Translate. The function signature promises (*PositionMap, error) but the error is always nil. This misleads callers into error-checking something that never fails. Either the error return should be removed (making it func BuildPositionToLineMap(diff string) *PositionMap) or genuine parsing errors (e.g. malformed hunk headers) should be surfaced. Since parseHunkStart silently falls back to 1 on any parse failure, the latter may not be practical — but the always-nil error is a minor API lie.

**[MINOR]** The `BuildPositionToLineMap` function never returns a non-nil error — all error paths are deferred to `Translate`. The function signature promises `(*PositionMap, error)` but the error is always nil. This misleads callers into error-checking something that never fails. Either the error return should be removed (making it `func BuildPositionToLineMap(diff string) *PositionMap`) or genuine parsing errors (e.g. malformed hunk headers) should be surfaced. Since `parseHunkStart` silently falls back to 1 on any parse failure, the latter may not be practical — but the always-nil error is a minor API lie.
var position int
var newLine int
for _, line := range lines {
// Detect new file in diff.
// "+++ b/" is checked before "+++ /dev/null" — the two prefixes are
// non-overlapping ("+++ /dev/null" does not start with "+++ b/"), so
// ordering is independent. Checking the common case first for clarity.
if strings.HasPrefix(line, "+++ b/") {
currentFile = strings.TrimPrefix(line, "+++ b/")
Review

[NIT] The comment on the +++ b/ check says 'non-overlapping' but the reasoning is slightly off: +++ /dev/null doesn't start with +++ b/ so the order doesn't matter for correctness. The comment is technically fine but slightly misleading — it implies order matters when it doesn't. A simpler comment would suffice.

**[NIT]** The comment on the `+++ b/` check says 'non-overlapping' but the reasoning is slightly off: `+++ /dev/null` doesn't start with `+++ b/` so the order doesn't matter for correctness. The comment is technically fine but slightly misleading — it implies order matters when it doesn't. A simpler comment would suffice.
position = 0
newLine = 0
if pm.files[currentFile] == nil {
pm.files[currentFile] = make(map[int]int)
}
continue
}
// Deleted file: +++ /dev/null means the file is being deleted
if strings.HasPrefix(line, "+++ /dev/null") {
Review

[NIT] The comment says '+++ b/ is checked before +++ /dev/null — the two prefixes are non-overlapping... so ordering is independent' but then immediately says 'Checking the common case first for clarity.' The comment is slightly contradictory: if ordering is independent, the justification doesn't need the non-overlapping note. The comment could be simplified to just explain intent (common case first for readability).

**[NIT]** The comment says '+++ b/ is checked before +++ /dev/null — the two prefixes are non-overlapping... so ordering is independent' but then immediately says 'Checking the common case first for clarity.' The comment is slightly contradictory: if ordering is independent, the justification doesn't need the non-overlapping note. The comment could be simplified to just explain intent (common case first for readability).
currentFile = ""
continue
}
// Skip --- lines (old file header)
if strings.HasPrefix(line, "--- ") {
continue
Review

[NIT] The +++ /dev/null prefix check must come before the +++ b/ check to be reachable. Currently the ordering is correct (+++ b/ is checked first, then +++ /dev/null), but since +++ /dev/null does not start with +++ b/, both checks are actually independent and ordering doesn't matter. A comment clarifying the ordering intent would improve readability.

**[NIT]** The `+++ /dev/null` prefix check must come before the `+++ b/` check to be reachable. Currently the ordering is correct (+++ b/ is checked first, then +++ /dev/null), but since `+++ /dev/null` does not start with `+++ b/`, both checks are actually independent and ordering doesn't matter. A comment clarifying the ordering intent would improve readability.
}
// Skip diff --git lines
if strings.HasPrefix(line, "diff --git") {
continue
}
// Skip index lines
if strings.HasPrefix(line, "index ") {
continue
}
// Binary file detection
if strings.HasPrefix(line, "Binary files") {
currentFile = ""
continue
}
// Parse hunk headers
if strings.HasPrefix(line, "@@") && currentFile != "" {
position++
pm.maxPositions[currentFile] = position
newLine = parseHunkStart(line)
continue
}
if currentFile == "" {
continue
}
// Skip "\ No newline at end of file" markers
if strings.HasPrefix(line, `\`) {
continue
}
// Process diff content lines
if strings.HasPrefix(line, "+") {
// Addition: has a new-file line number
position++
pm.files[currentFile][position] = newLine
pm.maxPositions[currentFile] = position
newLine++
} else if strings.HasPrefix(line, "-") {
// Deletion: has a position but no new-file line number
position++
pm.files[currentFile][position] = -1
pm.maxPositions[currentFile] = position
} else if strings.HasPrefix(line, " ") {
// Context line
position++
pm.files[currentFile][position] = newLine
pm.maxPositions[currentFile] = position
newLine++
}
}
return pm
}
// parseHunkStart extracts the new-file starting line number from a hunk header.
// Format: @@ -old_start[,old_count] +new_start[,new_count] @@
func parseHunkStart(hunkLine string) int {
plusIdx := strings.Index(hunkLine, "+")
if plusIdx < 0 {
return 1
}
rest := hunkLine[plusIdx+1:]
endIdx := 0
for endIdx < len(rest) && rest[endIdx] >= '0' && rest[endIdx] <= '9' {
endIdx++
}
if endIdx == 0 {
return 1
}
n, err := strconv.Atoi(rest[:endIdx])
if err != nil {
return 1
}
return n
}
+274
View File
@@ -0,0 +1,274 @@
package gitea
import (
"testing"
)
func TestBuildPositionToLineMap_SingleHunk(t *testing.T) {
// @@ -16,4 +16,5 @@ ← position 1
// context ← position 2, new line 16
//-deleted ← position 3, no new line
//+added ← position 4, new line 17
// context ← position 5, new line 18
diff := `diff --git a/file.go b/file.go
index abc..def 100644
--- a/file.go
+++ b/file.go
@@ -16,4 +16,5 @@ func example() {
context line
-deleted line
+added line
context after
`
pm := BuildPositionToLineMap(diff)
tests := []struct {
pos int
wantLine int
}{
{2, 16}, // context line -> new line 16
{4, 17}, // added line -> new line 17
{5, 18}, // context after -> new line 18
}
for _, tt := range tests {
got, err := pm.Translate("file.go", tt.pos)
if err != nil {
t.Errorf("Translate(file.go, %d): unexpected error: %v", tt.pos, err)
continue
}
if got != tt.wantLine {
t.Errorf("Translate(file.go, %d) = %d, want %d", tt.pos, got, tt.wantLine)
}
}
}
func TestBuildPositionToLineMap_MultipleHunks(t *testing.T) {
diff := `diff --git a/file.go b/file.go
--- a/file.go
+++ b/file.go
@@ -1,3 +1,3 @@ package main
line1
-old
+new
@@ -10,3 +10,4 @@ func foo() {
func foo() {
+ // added
return
}
`
pm := BuildPositionToLineMap(diff)
tests := []struct {
pos int
wantLine int
}{
// First hunk: @@ is pos 1
{2, 1}, // " line1" -> new line 1
{4, 2}, // "+new" -> new line 2
// Second hunk: @@ is pos 5 (continues from 4)
// Wait: first hunk has pos 1(@@ hdr), 2(" line1"), 3("-old"), 4("+new")
// Second hunk @@ is pos 5
{6, 10}, // " func foo() {" -> new line 10
{7, 11}, // "+\t// added" -> new line 11
{8, 12}, // " \treturn" -> new line 12
{9, 13}, // " }" -> new line 13
}
for _, tt := range tests {
got, err := pm.Translate("file.go", tt.pos)
if err != nil {
t.Errorf("Translate(file.go, %d): unexpected error: %v", tt.pos, err)
continue
}
if got != tt.wantLine {
t.Errorf("Translate(file.go, %d) = %d, want %d", tt.pos, got, tt.wantLine)
}
}
}
func TestBuildPositionToLineMap_DeletionTargeted(t *testing.T) {
diff := `diff --git a/file.go b/file.go
--- a/file.go
+++ b/file.go
@@ -1,4 +1,3 @@ package main
line1
-deleted
line3
`
pm := BuildPositionToLineMap(diff)
// Position 3 is the deletion line "-deleted" — should map to nearest below
// Position 4 is " line3" which is new line 2
got, err := pm.Translate("file.go", 3)
if err != nil {
t.Fatalf("Translate(file.go, 3): unexpected error: %v", err)
}
if got != 2 {
t.Errorf("Translate(file.go, 3) = %d, want 2 (nearest non-deletion below)", got)
}
}
func TestBuildPositionToLineMap_DeletionAtEnd(t *testing.T) {
// If a deletion line is at the end with no subsequent non-deletion line, error
diff := `diff --git a/file.go b/file.go
--- a/file.go
+++ b/file.go
@@ -1,3 +1,2 @@ package main
line1
line2
-deleted at end
`
pm := BuildPositionToLineMap(diff)
_, err := pm.Translate("file.go", 4)
if err == nil {
t.Error("expected error for deletion at end with no subsequent line")
}
}
func TestBuildPositionToLineMap_NewFile(t *testing.T) {
diff := `diff --git a/new.go b/new.go
new file mode 100644
--- /dev/null
+++ b/new.go
@@ -0,0 +1,3 @@
+package main
+
+func init() {}
`
pm := BuildPositionToLineMap(diff)
tests := []struct {
pos int
wantLine int
}{
{2, 1}, // "+package main" -> line 1
{3, 2}, // "+" (empty line) -> line 2
{4, 3}, // "+func init() {}" -> line 3
}
for _, tt := range tests {
got, err := pm.Translate("new.go", tt.pos)
if err != nil {
t.Errorf("Translate(new.go, %d): unexpected error: %v", tt.pos, err)
continue
}
if got != tt.wantLine {
t.Errorf("Translate(new.go, %d) = %d, want %d", tt.pos, got, tt.wantLine)
}
}
}
func TestBuildPositionToLineMap_DeletedFile(t *testing.T) {
diff := `diff --git a/old.go b/old.go
deleted file mode 100644
--- a/old.go
+++ /dev/null
@@ -1,3 +0,0 @@
-package main
-
-func old() {}
`
pm := BuildPositionToLineMap(diff)
// Deleted file has no new-file lines; positions should error
_, err := pm.Translate("old.go", 2)
if err == nil {
t.Error("expected error for deleted file position")
}
}
func TestBuildPositionToLineMap_BinaryFile(t *testing.T) {
diff := `diff --git a/image.png b/image.png
Binary files /dev/null and b/image.png differ
diff --git a/code.go b/code.go
--- a/code.go
+++ b/code.go
@@ -1,2 +1,3 @@
package main
+// added
func main() {}
`
pm := BuildPositionToLineMap(diff)
// Binary file should not be in the map
_, err := pm.Translate("image.png", 1)
if err == nil {
t.Error("expected error for binary file")
}
// code.go should still work
got, err := pm.Translate("code.go", 3)
if err != nil {
t.Fatalf("Translate(code.go, 3): unexpected error: %v", err)
}
if got != 2 {
t.Errorf("Translate(code.go, 3) = %d, want 2", got)
}
}
func TestBuildPositionToLineMap_OutOfRange(t *testing.T) {
diff := `diff --git a/file.go b/file.go
--- a/file.go
+++ b/file.go
@@ -1,2 +1,2 @@
line1
-old
+new
`
pm := BuildPositionToLineMap(diff)
// Position 0 is invalid
_, err := pm.Translate("file.go", 0)
if err == nil {
t.Error("expected error for position 0")
}
// Position 5 is out of range (only positions 1-4 exist)
_, err = pm.Translate("file.go", 5)
if err == nil {
t.Error("expected error for position 5 (out of range)")
}
// Unknown file
_, err = pm.Translate("unknown.go", 1)
if err == nil {
t.Error("expected error for unknown file")
}
}
func TestBuildPositionToLineMap_MultipleFiles(t *testing.T) {
diff := `diff --git a/a.go b/a.go
--- a/a.go
+++ b/a.go
@@ -1,2 +1,3 @@
package a
+// file a
func aFunc() {}
diff --git a/b.go b/b.go
--- a/b.go
+++ b/b.go
@@ -1,2 +1,3 @@
package b
+// file b
func bFunc() {}
`
pm := BuildPositionToLineMap(diff)
// a.go: pos 3 is "+// file a" -> new line 2
got, err := pm.Translate("a.go", 3)
if err != nil {
t.Fatalf("Translate(a.go, 3): %v", err)
}
if got != 2 {
t.Errorf("Translate(a.go, 3) = %d, want 2", got)
}
// b.go: pos 3 is "+// file b" -> new line 2
// Note: position resets per file
got, err = pm.Translate("b.go", 3)
if err != nil {
t.Fatalf("Translate(b.go, 3): %v", err)
}
if got != 2 {
t.Errorf("Translate(b.go, 3) = %d, want 2", got)
}
}
+4 -20
View File
@@ -1,5 +1,3 @@
//go:build phase2
package vcs_test
import (
@@ -7,21 +5,7 @@ import (
"gitea.weiker.me/rodin/review-bot/vcs"
)
// Compile-time assertion: documents the gap between gitea.Client and vcs.Client.
// Guarded by the "phase2" build tag — enable once the Gitea adapter bridges these gaps:
//
// 1. PostReview signature mismatch:
// gitea.Client: PostReview(ctx, owner, repo, number, event, body string, comments []gitea.ReviewComment)
// vcs.Reviewer: PostReview(ctx, owner, repo, number, req vcs.ReviewRequest)
//
// 2. GetFileContent signature mismatch:
// gitea.Client: GetFileContent(ctx, owner, repo, filepath string) [no ref; uses default branch]
// vcs.FileReader: GetFileContent(ctx, owner, repo, path, ref string)
// (gitea.Client has GetFileContentRef for the ref variant)
//
// 3. ReviewComment type mismatch:
// gitea.ReviewComment uses NewPosition int64 (Gitea line-number convention)
// vcs.ReviewComment uses Position int (GitHub diff-position convention)
//
// The Gitea adapter (Phase 2) will wrap gitea.Client to bridge these gaps.
var _ vcs.Client = (*gitea.Client)(nil)
// Compile-time assertion: the gitea.Adapter satisfies vcs.Client.
// (The raw gitea.Client does NOT satisfy vcs.Client due to signature differences;
// the Adapter bridges them.)
var _ vcs.Client = (*gitea.Adapter)(nil)