rodin/review-bot
2
1
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 6 Wiki Activity

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
Conversation 19 Commits 4 Files Changed 7
+22 -12
3 changed files with 22 additions and 12 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit b2eea502d0 - Show all commits
gitea/adapter.go
+5 -1
View File
7
 @@ -150,7 +150,11 @@ func (a *Adapter) PostReview(ctx context.Context, owner, repo string, number int
var giteaComments []ReviewComment
if len(req.Comments) > 0 {
sonnet-review-bot commented 2026-05-12 20:50:31 +00:00
Review
Copy Link
Copy Source

[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
// Fetch diff to build position → line number map.
sonnet-review-bot commented 2026-05-12 20:31:49 +00:00
Outdated
Review
Copy Link
Copy Source

[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)
1
gitea/adapter_test.go
-1
View File
6
 @@ -354,7 +354,6 @@ func TestAdapter_ListContents(t *testing.T) {
}
}
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=
gitea/position.go
+17 -10
View File
@@ -12,6 +12,9 @@ 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.
2
 @@ -53,15 +56,9 @@ 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).
// O(1) — the maximum is tracked during map construction.
func (pm *PositionMap) maxPosition(file string) int {
sonnet-review-bot commented 2026-05-12 20:31:49 +00:00
Review
Copy Link
Copy Source

[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.
gpt-review-bot commented 2026-05-12 20:33:19 +00:00
Review
Copy Link
Copy Source

[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.
max := 0
for pos := range pm.files[file] {
if pos > max {
max = pos
}
}
return max
return pm.maxPositions[file]
}
// BuildPositionToLineMap parses a unified diff and builds a PositionMap
@@ -74,7 +71,10 @@ func (pm *PositionMap) maxPosition(file string) int {
// - 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)}
pm := &PositionMap{
files: make(map[string]map[int]int),
sonnet-review-bot commented 2026-05-12 21:57:51 +00:00
Review
Copy Link
Copy Source

[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")
security-review-bot commented 2026-05-12 21:58:55 +00:00
Review
Copy Link
Copy Source

[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
sonnet-review-bot commented 2026-05-12 20:31:49 +00:00
Review
Copy Link
Copy Source

[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.
@@ -82,7 +82,10 @@ func BuildPositionToLineMap(diff string) *PositionMap {
var newLine int
for _, line := range lines {
// Detect new file in diff
// 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/")
sonnet-review-bot commented 2026-05-12 20:58:43 +00:00
Review
Copy Link
Copy Source

[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
2
 @@ -123,6 +126,7 @@ func BuildPositionToLineMap(diff string) *PositionMap {
// Parse hunk headers
if strings.HasPrefix(line, "@@") && currentFile != "" {
position++
pm.maxPositions[currentFile] = position
newLine = parseHunkStart(line)
continue
}
@@ -141,15 +145,18 @@ func BuildPositionToLineMap(diff string) *PositionMap {
// 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++
}
}