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

feat(vcs): complete Phase 1 — util.go, type cleanup, interface additions (fixes #84, #85, #86) #88

Merged
aweiker merged 3 commits from review-bot-issue-84 into feature/github-support 2026-05-12 20:18:18 +00:00
Conversation 17 Commits 3 Files Changed 9
+627 -88
9 changed files with 627 additions and 88 deletions
Download Patch File Download Diff File Expand all files Collapse all files
cmd/review-bot/main.go
+10 -6
View File
@@ -15,6 +15,7 @@ import (
"gitea.weiker.me/rodin/review-bot/gitea" "gitea.weiker.me/rodin/review-bot/gitea"
"gitea.weiker.me/rodin/review-bot/llm" "gitea.weiker.me/rodin/review-bot/llm"
"gitea.weiker.me/rodin/review-bot/review" "gitea.weiker.me/rodin/review-bot/review"
"gitea.weiker.me/rodin/review-bot/vcs"
) )
var version = "dev" var version = "dev"
@@ -812,7 +813,7 @@ func shouldSkipStaleReview(evaluatedSHA, currentSHA string) bool {
return evaluatedSHA != currentSHA return evaluatedSHA != currentSHA
} }
// giteaClientAdapter adapts gitea.Client to review.GiteaClient interface. // giteaClientAdapter adapts gitea.Client to vcs.FileReader interface.
type giteaClientAdapter struct { type giteaClientAdapter struct {
client *gitea.Client client *gitea.Client
} }
@@ -821,14 +822,14 @@ func newGiteaClientAdapter(c *gitea.Client) *giteaClientAdapter {
return &giteaClientAdapter{client: c} return &giteaClientAdapter{client: c}
} }
func (a *giteaClientAdapter) ListContents(ctx context.Context, owner, repo, path string) ([]review.ContentEntry, error) { func (a *giteaClientAdapter) ListContents(ctx context.Context, owner, repo, path string) ([]vcs.ContentEntry, error) {
entries, err := a.client.ListContents(ctx, owner, repo, path) entries, err := a.client.ListContents(ctx, owner, repo, path)
if err != nil { if err != nil {
return nil, err return nil, err
} }
result := make([]review.ContentEntry, len(entries)) result := make([]vcs.ContentEntry, len(entries))
for i, e := range entries { for i, e := range entries {
result[i] = review.ContentEntry{ result[i] = vcs.ContentEntry{
Name: e.Name, Name: e.Name,
Path: e.Path, Path: e.Path,
Type: e.Type, Type: e.Type,
@@ -837,6 +838,9 @@ func (a *giteaClientAdapter) ListContents(ctx context.Context, owner, repo, path
return result, nil return result, nil
sonnet-review-bot commented 2026-05-12 19:57:18 +00:00
Review
Copy Link
Copy Source

[NIT] The giteaClientAdapter.GetFileContent method has a parameter named filepath which shadows the standard library package path/filepath. Although path/filepath is not imported in this file, the name filepath for a string parameter is mildly confusing. Renaming to filePath or path would be cleaner and is consistent with how the vcs.FileReader interface parameter is described elsewhere.

**[NIT]** The giteaClientAdapter.GetFileContent method has a parameter named `filepath` which shadows the standard library package `path/filepath`. Although `path/filepath` is not imported in this file, the name `filepath` for a string parameter is mildly confusing. Renaming to `filePath` or `path` would be cleaner and is consistent with how the vcs.FileReader interface parameter is described elsewhere.
} }
func (a *giteaClientAdapter) GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) { func (a *giteaClientAdapter) GetFileContent(ctx context.Context, owner, repo, filePath, ref string) (string, error) {
return a.client.GetFileContent(ctx, owner, repo, filepath) if ref != "" {
return a.client.GetFileContentRef(ctx, owner, repo, filePath, ref)
}
return a.client.GetFileContent(ctx, owner, repo, filePath)
} }
gitea/client.go
+12
View File
@@ -831,3 +831,15 @@ func (c *Client) ResolveComment(ctx context.Context, owner, repo string, comment
} }
return nil return nil
sonnet-review-bot commented 2026-05-12 19:39:28 +00:00
Review
Copy Link
Copy Source

[NIT] DismissReview returns a hardcoded fmt.Errorf("DismissReview: not implemented") without wrapping any sentinel or using errors.New. This is fine as a stub but should satisfy the vcs.Reviewer interface contract note and ideally reference errors.ErrUnsupported per the pattern (fmt.Errorf("DismissReview: %w", errors.ErrUnsupported)) so callers can detect the unsupported condition programmatically.

**[NIT]** DismissReview returns a hardcoded `fmt.Errorf("DismissReview: not implemented")` without wrapping any sentinel or using `errors.New`. This is fine as a stub but should satisfy the `vcs.Reviewer` interface contract note and ideally reference `errors.ErrUnsupported` per the pattern (`fmt.Errorf("DismissReview: %w", errors.ErrUnsupported)`) so callers can detect the unsupported condition programmatically.
} }
sonnet-review-bot commented 2026-05-12 19:57:18 +00:00
Review
Copy Link
Copy Source

[MINOR] DismissReview returns fmt.Errorf("DismissReview: %w", errors.ErrUnsupported). Per the error-handling pattern, errors.ErrUnsupported should be wrapped with context that describes what operation was attempted and why it's unsupported, rather than restating the function name. A more idiomatic message would be fmt.Errorf("dismiss review %d on %s/%s#%d: %w", reviewID, owner, repo, number, errors.ErrUnsupported). The current form matches the anti-pattern documented in error-handling.md §9 ('Don't: Return the sentinel directly without context').

**[MINOR]** DismissReview returns `fmt.Errorf("DismissReview: %w", errors.ErrUnsupported)`. Per the error-handling pattern, `errors.ErrUnsupported` should be wrapped with context that describes *what* operation was attempted and why it's unsupported, rather than restating the function name. A more idiomatic message would be `fmt.Errorf("dismiss review %d on %s/%s#%d: %w", reviewID, owner, repo, number, errors.ErrUnsupported)`. The current form matches the anti-pattern documented in error-handling.md §9 ('Don't: Return the sentinel directly without context').
// DismissReview dismisses a review on a pull request.
// This is a stub for the vcs.Reviewer interface; full implementation is Phase 2.
func (c *Client) DismissReview(ctx context.Context, owner, repo string, number int, reviewID int64, message string) error {
return fmt.Errorf("dismiss review %d on %s/%s#%d: %w", reviewID, owner, repo, number, errors.ErrUnsupported)
}
sonnet-review-bot commented 2026-05-12 20:08:36 +00:00
Review
Copy Link
Copy Source

[NIT] GetFileContentAtRef is a thin wrapper that does nothing but delegate to GetFileContentRef with the same arguments in the same order. The docstring says "This delegates to GetFileContentRef for the Gitea implementation" — since the method exists only to satisfy the vcs.PRReader interface, a one-liner return c.GetFileContentRef(ctx, owner, repo, path, ref) with no further ceremony would be ideal, which is already the case. No change needed, but consider whether GetFileContentRef should simply be renamed to GetFileContentAtRef to avoid the two names existing in parallel on the same type.

**[NIT]** `GetFileContentAtRef` is a thin wrapper that does nothing but delegate to `GetFileContentRef` with the same arguments in the same order. The docstring says "This delegates to GetFileContentRef for the Gitea implementation" — since the method exists only to satisfy the `vcs.PRReader` interface, a one-liner `return c.GetFileContentRef(ctx, owner, repo, path, ref)` with no further ceremony would be ideal, which is already the case. No change needed, but consider whether `GetFileContentRef` should simply be renamed to `GetFileContentAtRef` to avoid the two names existing in parallel on the same type.
// GetFileContentAtRef fetches a file at a specific ref from a repo.
// This delegates to GetFileContentRef for the Gitea implementation.
func (c *Client) GetFileContentAtRef(ctx context.Context, owner, repo, path, ref string) (string, error) {
return c.GetFileContentRef(ctx, owner, repo, path, ref)
}
gitea/conformance_test.go
+25
View File
@@ -0,0 +1,25 @@
//go:build phase2
package gitea_test
import (
"gitea.weiker.me/rodin/review-bot/gitea"
"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)
sonnet-review-bot commented 2026-05-12 20:08:36 +00:00
Review
Copy Link
Copy Source

[NIT] The conformance assertions under //go:build phase2 include vcs.PRReader and vcs.Reviewer which gitea.Client explicitly does NOT yet satisfy (wrong return types for GetPullRequest, GetPullRequestFiles, PostReview). The build tag correctly guards against this, but the comment block lists the gaps well. This is fine as-is — just noting that once Phase 2 lands this file should have the build tag removed rather than being left indefinitely.

**[NIT]** The conformance assertions under `//go:build phase2` include `vcs.PRReader` and `vcs.Reviewer` which `gitea.Client` explicitly does NOT yet satisfy (wrong return types for `GetPullRequest`, `GetPullRequestFiles`, `PostReview`). The build tag correctly guards against this, but the comment block lists the gaps well. This is fine as-is — just noting that once Phase 2 lands this file should have the build tag removed rather than being left indefinitely.
_ vcs.FileReader = (*gitea.Client)(nil)
_ vcs.Reviewer = (*gitea.Client)(nil)
_ vcs.Identity = (*gitea.Client)(nil)
)
review/repo_persona.go
+4 -17
View File
@@ -4,32 +4,19 @@ import (
"context" "context"
"log/slog" "log/slog"
"strings" "strings"
"gitea.weiker.me/rodin/review-bot/vcs"
) )
// RepoPersonaPath is the directory path where repo-specific personas are stored. // RepoPersonaPath is the directory path where repo-specific personas are stored.
const RepoPersonaPath = ".review-bot/personas" const RepoPersonaPath = ".review-bot/personas"
// GiteaClient defines the subset of gitea.Client methods needed for loading repo personas.
// This interface allows for easier testing and decouples the review package from gitea.
type GiteaClient interface {
ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error)
GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error)
}
// ContentEntry represents a file or directory entry from the contents API.
// This mirrors gitea.ContentEntry to avoid import cycles.
type ContentEntry struct {
Name string `json:"name"`
Path string `json:"path"`
Type string `json:"type"` // "file" or "dir"
}
// LoadRepoPersonas fetches personas from a repository's .review-bot/personas/ directory. // LoadRepoPersonas fetches personas from a repository's .review-bot/personas/ directory.
// Returns an empty map (not nil) if the directory doesn't exist or is empty. // Returns an empty map (not nil) if the directory doesn't exist or is empty.
// Individual parse failures are logged and skipped; the remaining personas are still returned. // Individual parse failures are logged and skipped; the remaining personas are still returned.
// Auth errors and other non-404 errors are propagated. // Auth errors and other non-404 errors are propagated.
// Files exceeding MaxPersonaFileSize are rejected to prevent resource exhaustion. // Files exceeding MaxPersonaFileSize are rejected to prevent resource exhaustion.
func LoadRepoPersonas(ctx context.Context, client GiteaClient, owner, repo string) (map[string]*Persona, error) { func LoadRepoPersonas(ctx context.Context, client vcs.FileReader, owner, repo string) (map[string]*Persona, error) {
result := make(map[string]*Persona) result := make(map[string]*Persona)
entries, err := client.ListContents(ctx, owner, repo, RepoPersonaPath) entries, err := client.ListContents(ctx, owner, repo, RepoPersonaPath)
@@ -57,7 +44,7 @@ func LoadRepoPersonas(ctx context.Context, client GiteaClient, owner, repo strin
continue continue
} }
content, err := client.GetFileContent(ctx, owner, repo, entry.Path) content, err := client.GetFileContent(ctx, owner, repo, entry.Path, "")
if err != nil { if err != nil {
slog.Warn("could not fetch repo persona file", slog.Warn("could not fetch repo persona file",
"file", entry.Path, "file", entry.Path,
review/repo_persona_test.go
+31 -62
View File
@@ -5,23 +5,21 @@ import (
"errors" "errors"
"strings" "strings"
"testing" "testing"
"gitea.weiker.me/rodin/review-bot/vcs"
) )
func TestParsePersonaBytes(t *testing.T) { func TestParsePersonaBytes(t *testing.T) {
tests := []struct { tests := []struct {
name string name string
data string data string
source string source string
wantName string wantName string
wantErr string wantErr string
}{ }{
{ {
name: "valid yaml", name: "valid yaml",
data: `name: test data: "name: test\nidentity: test identity\nfocus:\n - testing\n",
identity: test identity
focus:
- testing
`,
source: "test.yaml", source: "test.yaml",
wantName: "test", wantName: "test",
}, },
@@ -38,8 +36,8 @@ focus:
wantErr: "parse", wantErr: "parse",
}, },
{ {
name: "json format by extension", name: "json format by extension",
data: `{"name": "jsontest", "identity": "json identity"}`, data: `{"name": "jsontest", "identity": "json identity"}`,
source: "test.json", source: "test.json",
wantName: "jsontest", wantName: "jsontest",
}, },
@@ -67,15 +65,15 @@ focus:
} }
} }
// mockGiteaClient implements GiteaClient for testing. // mockGiteaClient implements vcs.FileReader for testing.
type mockGiteaClient struct { type mockGiteaClient struct {
contents map[string][]ContentEntry // path -> entries contents map[string][]vcs.ContentEntry // path -> entries
files map[string]string // path -> content files map[string]string // path -> content
listErr error listErr error
fileErr map[string]error // path -> error fileErr map[string]error // path -> error
} }
func (m *mockGiteaClient) ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error) { func (m *mockGiteaClient) ListContents(ctx context.Context, owner, repo, path string) ([]vcs.ContentEntry, error) {
if m.listErr != nil { if m.listErr != nil {
return nil, m.listErr return nil, m.listErr
} }
@@ -86,7 +84,7 @@ func (m *mockGiteaClient) ListContents(ctx context.Context, owner, repo, path st
return entries, nil return entries, nil
} }
func (m *mockGiteaClient) GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) { func (m *mockGiteaClient) GetFileContent(ctx context.Context, owner, repo, filepath, ref string) (string, error) {
if m.fileErr != nil { if m.fileErr != nil {
if err, ok := m.fileErr[filepath]; ok { if err, ok := m.fileErr[filepath]; ok {
return "", err return "", err
@@ -118,7 +116,7 @@ func TestLoadRepoPersonas(t *testing.T) {
t.Run("empty directory returns empty map", func(t *testing.T) { t.Run("empty directory returns empty map", func(t *testing.T) {
client := &mockGiteaClient{ client := &mockGiteaClient{
contents: map[string][]ContentEntry{ contents: map[string][]vcs.ContentEntry{
RepoPersonaPath: {}, RepoPersonaPath: {},
}, },
} }
@@ -133,27 +131,15 @@ func TestLoadRepoPersonas(t *testing.T) {
t.Run("loads valid personas", func(t *testing.T) { t.Run("loads valid personas", func(t *testing.T) {
client := &mockGiteaClient{ client := &mockGiteaClient{
contents: map[string][]ContentEntry{ contents: map[string][]vcs.ContentEntry{
RepoPersonaPath: { RepoPersonaPath: {
{Name: "trading.yaml", Path: ".review-bot/personas/trading.yaml", Type: "file"}, {Name: "trading.yaml", Path: ".review-bot/personas/trading.yaml", Type: "file"},
{Name: "crypto.yaml", Path: ".review-bot/personas/crypto.yaml", Type: "file"}, {Name: "crypto.yaml", Path: ".review-bot/personas/crypto.yaml", Type: "file"},
}, },
}, },
files: map[string]string{ files: map[string]string{
".review-bot/personas/trading.yaml": `name: trading ".review-bot/personas/trading.yaml": "name: trading\ndisplay_name: Trading Expert\nidentity: You are a trading expert.\nfocus:\n - order handling\n - risk management\n",
display_name: Trading Expert ".review-bot/personas/crypto.yaml": "name: crypto\ndisplay_name: Crypto Expert\nidentity: You are a cryptography expert.\nfocus:\n - key management\n - encryption\n",
identity: You are a trading expert.
focus:
- order handling
- risk management
`,
".review-bot/personas/crypto.yaml": `name: crypto
display_name: Crypto Expert
identity: You are a cryptography expert.
focus:
- key management
- encryption
`,
}, },
} }
personas, err := LoadRepoPersonas(ctx, client, "owner", "repo") personas, err := LoadRepoPersonas(ctx, client, "owner", "repo")
@@ -176,16 +162,14 @@ focus:
t.Run("skips invalid persona files", func(t *testing.T) { t.Run("skips invalid persona files", func(t *testing.T) {
client := &mockGiteaClient{ client := &mockGiteaClient{
contents: map[string][]ContentEntry{ contents: map[string][]vcs.ContentEntry{
RepoPersonaPath: { RepoPersonaPath: {
{Name: "valid.yaml", Path: ".review-bot/personas/valid.yaml", Type: "file"}, {Name: "valid.yaml", Path: ".review-bot/personas/valid.yaml", Type: "file"},
{Name: "invalid.yaml", Path: ".review-bot/personas/invalid.yaml", Type: "file"}, {Name: "invalid.yaml", Path: ".review-bot/personas/invalid.yaml", Type: "file"},
}, },
}, },
files: map[string]string{ files: map[string]string{
".review-bot/personas/valid.yaml": `name: valid ".review-bot/personas/valid.yaml": "name: valid\nidentity: Valid persona\n",
identity: Valid persona
`,
".review-bot/personas/invalid.yaml": "not valid yaml: [broken", ".review-bot/personas/invalid.yaml": "not valid yaml: [broken",
}, },
} }
@@ -193,7 +177,6 @@ identity: Valid persona
if err != nil { if err != nil {
t.Fatalf("unexpected error: %v", err) t.Fatalf("unexpected error: %v", err)
} }
// Should have the valid one, skip the invalid
if len(personas) != 1 { if len(personas) != 1 {
t.Fatalf("expected 1 persona (skipped invalid), got %d", len(personas)) t.Fatalf("expected 1 persona (skipped invalid), got %d", len(personas))
} }
@@ -204,7 +187,7 @@ identity: Valid persona
t.Run("skips non-yaml files", func(t *testing.T) { t.Run("skips non-yaml files", func(t *testing.T) {
client := &mockGiteaClient{ client := &mockGiteaClient{
contents: map[string][]ContentEntry{ contents: map[string][]vcs.ContentEntry{
RepoPersonaPath: { RepoPersonaPath: {
{Name: "persona.yaml", Path: ".review-bot/personas/persona.yaml", Type: "file"}, {Name: "persona.yaml", Path: ".review-bot/personas/persona.yaml", Type: "file"},
{Name: "README.md", Path: ".review-bot/personas/README.md", Type: "file"}, {Name: "README.md", Path: ".review-bot/personas/README.md", Type: "file"},
@@ -212,10 +195,8 @@ identity: Valid persona
}, },
}, },
files: map[string]string{ files: map[string]string{
".review-bot/personas/persona.yaml": `name: test ".review-bot/personas/persona.yaml": "name: test\nidentity: Test persona\n",
identity: Test persona ".review-bot/personas/README.md": "# Personas\n\nPut your personas here.",
`,
".review-bot/personas/README.md": "# Personas\n\nPut your personas here.",
}, },
} }
personas, err := LoadRepoPersonas(ctx, client, "owner", "repo") personas, err := LoadRepoPersonas(ctx, client, "owner", "repo")
@@ -229,16 +210,14 @@ identity: Test persona
t.Run("skips subdirectories", func(t *testing.T) { t.Run("skips subdirectories", func(t *testing.T) {
client := &mockGiteaClient{ client := &mockGiteaClient{
contents: map[string][]ContentEntry{ contents: map[string][]vcs.ContentEntry{
RepoPersonaPath: { RepoPersonaPath: {
{Name: "persona.yaml", Path: ".review-bot/personas/persona.yaml", Type: "file"}, {Name: "persona.yaml", Path: ".review-bot/personas/persona.yaml", Type: "file"},
{Name: "subdir", Path: ".review-bot/personas/subdir", Type: "dir"}, {Name: "subdir", Path: ".review-bot/personas/subdir", Type: "dir"},
}, },
}, },
files: map[string]string{ files: map[string]string{
".review-bot/personas/persona.yaml": `name: test ".review-bot/personas/persona.yaml": "name: test\nidentity: Test persona\n",
identity: Test persona
`,
}, },
} }
personas, err := LoadRepoPersonas(ctx, client, "owner", "repo") personas, err := LoadRepoPersonas(ctx, client, "owner", "repo")
@@ -265,16 +244,14 @@ identity: Test persona
t.Run("skips files that fail to fetch", func(t *testing.T) { t.Run("skips files that fail to fetch", func(t *testing.T) {
client := &mockGiteaClient{ client := &mockGiteaClient{
contents: map[string][]ContentEntry{ contents: map[string][]vcs.ContentEntry{
RepoPersonaPath: { RepoPersonaPath: {
{Name: "good.yaml", Path: ".review-bot/personas/good.yaml", Type: "file"}, {Name: "good.yaml", Path: ".review-bot/personas/good.yaml", Type: "file"},
{Name: "bad.yaml", Path: ".review-bot/personas/bad.yaml", Type: "file"}, {Name: "bad.yaml", Path: ".review-bot/personas/bad.yaml", Type: "file"},
}, },
}, },
files: map[string]string{ files: map[string]string{
".review-bot/personas/good.yaml": `name: good ".review-bot/personas/good.yaml": "name: good\nidentity: Good persona\n",
identity: Good persona
`,
}, },
fileErr: map[string]error{ fileErr: map[string]error{
".review-bot/personas/bad.yaml": errors.New("HTTP 500: internal server error"), ".review-bot/personas/bad.yaml": errors.New("HTTP 500: internal server error"),
@@ -290,27 +267,23 @@ identity: Good persona
}) })
t.Run("skips oversized files", func(t *testing.T) { t.Run("skips oversized files", func(t *testing.T) {
// Create a content string that exceeds MaxPersonaFileSize (64KB)
oversizedContent := strings.Repeat("a", MaxPersonaFileSize+1) oversizedContent := strings.Repeat("a", MaxPersonaFileSize+1)
client := &mockGiteaClient{ client := &mockGiteaClient{
contents: map[string][]ContentEntry{ contents: map[string][]vcs.ContentEntry{
RepoPersonaPath: { RepoPersonaPath: {
{Name: "normal.yaml", Path: ".review-bot/personas/normal.yaml", Type: "file"}, {Name: "normal.yaml", Path: ".review-bot/personas/normal.yaml", Type: "file"},
{Name: "huge.yaml", Path: ".review-bot/personas/huge.yaml", Type: "file"}, {Name: "huge.yaml", Path: ".review-bot/personas/huge.yaml", Type: "file"},
}, },
}, },
files: map[string]string{ files: map[string]string{
".review-bot/personas/normal.yaml": `name: normal ".review-bot/personas/normal.yaml": "name: normal\nidentity: Normal sized persona\n",
identity: Normal sized persona ".review-bot/personas/huge.yaml": oversizedContent,
`,
".review-bot/personas/huge.yaml": oversizedContent,
}, },
} }
personas, err := LoadRepoPersonas(ctx, client, "owner", "repo") personas, err := LoadRepoPersonas(ctx, client, "owner", "repo")
if err != nil { if err != nil {
t.Fatalf("unexpected error: %v", err) t.Fatalf("unexpected error: %v", err)
} }
// Should have the normal one, skip the oversized
if len(personas) != 1 { if len(personas) != 1 {
t.Fatalf("expected 1 persona (skipped oversized), got %d", len(personas)) t.Fatalf("expected 1 persona (skipped oversized), got %d", len(personas))
} }
@@ -370,7 +343,6 @@ func TestGetBuiltinPersonasMap(t *testing.T) {
t.Fatal("expected at least one built-in persona") t.Fatal("expected at least one built-in persona")
} }
// Verify expected personas exist
expected := []string{"security", "architect", "docs"} expected := []string{"security", "architect", "docs"}
for _, name := range expected { for _, name := range expected {
if personas[name] == nil { if personas[name] == nil {
@@ -378,7 +350,6 @@ func TestGetBuiltinPersonasMap(t *testing.T) {
} }
} }
// Verify personas are valid
for name, p := range personas { for name, p := range personas {
if p.Name != name { if p.Name != name {
t.Errorf("persona %q has mismatched name %q", name, p.Name) t.Errorf("persona %q has mismatched name %q", name, p.Name)
@@ -422,8 +393,6 @@ func TestIsNotFoundError(t *testing.T) {
{nil, false}, {nil, false},
{errors.New("HTTP 404: not found"), true}, {errors.New("HTTP 404: not found"), true},
{errors.New("HTTP 404"), true}, {errors.New("HTTP 404"), true},
// Intentionally false: generic "not found" could mask auth/transport errors.
// Only explicit HTTP 404 responses should be treated as "directory doesn't exist".
{errors.New("something not found"), false}, {errors.New("something not found"), false},
{errors.New("HTTP 401: unauthorized"), false}, {errors.New("HTTP 401: unauthorized"), false},
{errors.New("connection refused"), false}, {errors.New("connection refused"), false},
vcs/interfaces.go
+3
View File
@@ -10,6 +10,8 @@ type PRReader interface {
GetPullRequest(ctx context.Context, owner, repo string, number int) (*PullRequest, error) GetPullRequest(ctx context.Context, owner, repo string, number int) (*PullRequest, error)
GetPullRequestDiff(ctx context.Context, owner, repo string, number int) (string, error) GetPullRequestDiff(ctx context.Context, owner, repo string, number int) (string, error)
GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]ChangedFile, error) GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]ChangedFile, error)
GetFileContentAtRef(ctx context.Context, owner, repo, path, ref string) (string, error)
GetCommitStatuses(ctx context.Context, owner, repo, sha string) ([]CommitStatus, error)
gpt-review-bot commented 2026-05-12 19:58:18 +00:00
Review
Copy Link
Copy Source

[MINOR] PRReader now includes GetFileContentAtRef and GetCommitStatuses, and Reviewer includes DismissReview. The gitea.Client gained stubs for these, but it still does not implement the full vcs.Client interface (e.g., its GetPullRequest returns *gitea.PullRequest, not *vcs.PullRequest). This may cause confusion for future implementers; consider adding a compile-time assertion in the adapter package when full conformance is intended or documenting that full implementation is planned for Phase 2.

**[MINOR]** PRReader now includes GetFileContentAtRef and GetCommitStatuses, and Reviewer includes DismissReview. The gitea.Client gained stubs for these, but it still does not implement the full vcs.Client interface (e.g., its GetPullRequest returns *gitea.PullRequest, not *vcs.PullRequest). This may cause confusion for future implementers; consider adding a compile-time assertion in the adapter package when full conformance is intended or documenting that full implementation is planned for Phase 2.
} }
// FileReader can fetch file contents and list directory entries. // FileReader can fetch file contents and list directory entries.
@@ -23,6 +25,7 @@ type Reviewer interface {
PostReview(ctx context.Context, owner, repo string, number int, req ReviewRequest) (*Review, error) PostReview(ctx context.Context, owner, repo string, number int, req ReviewRequest) (*Review, error)
ListReviews(ctx context.Context, owner, repo string, number int) ([]Review, error) ListReviews(ctx context.Context, owner, repo string, number int) ([]Review, error)
DeleteReview(ctx context.Context, owner, repo string, number int, reviewID int64) error DeleteReview(ctx context.Context, owner, repo string, number int, reviewID int64) error
DismissReview(ctx context.Context, owner, repo string, number int, reviewID int64, message string) error
} }
// Identity can report who the authenticated user is. // Identity can report who the authenticated user is.
vcs/types.go
+18 -3
View File
@@ -15,6 +15,11 @@ const (
ReviewEventComment ReviewEvent = "COMMENT" ReviewEventComment ReviewEvent = "COMMENT"
) )
// BaseRef identifies the target branch of a pull request.
type BaseRef struct {
Ref string `json:"ref"`
}
// HeadRef identifies the source branch and latest commit of a pull request. // HeadRef identifies the source branch and latest commit of a pull request.
type HeadRef struct { type HeadRef struct {
SHA string `json:"sha"` SHA string `json:"sha"`
@@ -28,9 +33,11 @@ type UserInfo struct {
// PullRequest holds relevant PR metadata. // PullRequest holds relevant PR metadata.
type PullRequest struct { type PullRequest struct {
Title string `json:"title"` Number int `json:"number"`
Body string `json:"body"` Title string `json:"title"`
Head HeadRef `json:"head"` Body string `json:"body"`
Head HeadRef `json:"head"`
Base BaseRef `json:"base"`
} }
// ChangedFile represents a file modified in a PR. // ChangedFile represents a file modified in a PR.
@@ -46,6 +53,14 @@ type ContentEntry struct {
Type string `json:"type"` // "file" or "dir" Type string `json:"type"` // "file" or "dir"
} }
// CommitStatus represents a single CI status entry for a commit.
type CommitStatus struct {
Status string `json:"status"`
Context string `json:"context"`
Description string `json:"description"`
TargetURL string `json:"target_url"`
}
// Review represents a pull request review. // Review represents a pull request review.
type Review struct { type Review struct {
ID int64 `json:"id"` ID int64 `json:"id"`
vcs/util.go
+193
View File
@@ -0,0 +1,193 @@
package vcs
import (
"context"
"fmt"
"strconv"
"strings"
)
const (
// maxFilesInPath is the maximum number of files GetAllFilesInPath will fetch.
security-review-bot marked this conversation as resolved Outdated
security-review-bot commented 2026-05-12 19:32:43 +00:00
Outdated
Review
Copy Link
Copy Source

[MAJOR] GetAllFilesInPath performs unbounded recursive traversal and loads all file contents into memory without limits on depth, number of files, or total bytes. An attacker controlling repository contents could craft deeply nested directories or many/large files to exhaust CPU/memory, causing a denial of service. Although context is passed to client calls, the function does not check ctx for cancellation nor enforce resource bounds.

**[MAJOR]** GetAllFilesInPath performs unbounded recursive traversal and loads all file contents into memory without limits on depth, number of files, or total bytes. An attacker controlling repository contents could craft deeply nested directories or many/large files to exhaust CPU/memory, causing a denial of service. Although context is passed to client calls, the function does not check ctx for cancellation nor enforce resource bounds.
// Prevents unbounded resource consumption on very large directory trees.
security-review-bot marked this conversation as resolved Outdated
security-review-bot commented 2026-05-12 19:40:39 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] GetAllFilesInPath recursively fetches and accumulates contents for all files without bounding the number of files or aggregate size, which could be leveraged for resource exhaustion if used on large paths. Consider enforcing limits (max files, max bytes) or filtering before fetching content.

**[MINOR]** GetAllFilesInPath recursively fetches and accumulates contents for all files without bounding the number of files or aggregate size, which could be leveraged for resource exhaustion if used on large paths. Consider enforcing limits (max files, max bytes) or filtering before fetching content.
maxFilesInPath = 10000
// maxTotalBytesInPath is the maximum total bytes GetAllFilesInPath will accumulate.
// Prevents memory exhaustion when fetching large repositories.
maxTotalBytesInPath = 100 * 1024 * 1024 // 100 MB
)
// GetAllFilesInPath recursively fetches all file contents under a path using the
// provided FileReader. Returns a map of filepath -> content for all files found.
// If the path points to an empty directory, returns an empty map.
gpt-review-bot commented 2026-05-12 19:34:15 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The switch over entry.Type handles only "file" and "dir"; other types (e.g., symlink, submodule) are silently ignored. Consider adding a default case or a comment to make this intent explicit.

**[NIT]** The switch over entry.Type handles only "file" and "dir"; other types (e.g., symlink, submodule) are silently ignored. Consider adding a default case or a comment to make this intent explicit.
//
// This function uses fail-fast error handling: any error from ListContents or
gpt-review-bot commented 2026-05-12 19:40:40 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] GetAllFilesInPath aborts on the first file read error; depending on usage, you may want to continue collecting other files (logging and skipping failures), similar to the gitea.Client implementation, to be more resilient to partial failures.

**[MINOR]** GetAllFilesInPath aborts on the first file read error; depending on usage, you may want to continue collecting other files (logging and skipping failures), similar to the gitea.Client implementation, to be more resilient to partial failures.
// GetFileContent aborts the entire traversal and returns the error immediately.
// This differs from gitea.Client.GetAllFilesInPath, which logs errors and continues.
// The fail-fast contract ensures callers can trust that a nil error means all files
sonnet-review-bot commented 2026-05-12 19:39:28 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] GetAllFilesInPath propagates errors from GetFileContent and recursive calls (returns nil, err) but the gitea.Client.GetAllFilesInPath in gitea/client.go logs warnings and continues instead of failing. The two implementations now have divergent error-handling semantics for the same conceptual operation. The vcs package version is stricter (fail-fast), which is the right design for a utility, but callers should be aware that a single unreachable file aborts the entire traversal. Consider documenting this difference or adding a comment explaining the fail-fast contract.

**[MINOR]** GetAllFilesInPath propagates errors from GetFileContent and recursive calls (returns nil, err) but the gitea.Client.GetAllFilesInPath in gitea/client.go logs warnings and continues instead of failing. The two implementations now have divergent error-handling semantics for the same conceptual operation. The vcs package version is stricter (fail-fast), which is the right design for a utility, but callers should be aware that a single unreachable file aborts the entire traversal. Consider documenting this difference or adding a comment explaining the fail-fast contract.
// were successfully fetched.
//
// Resource limits: the traversal is bounded by maxFilesInPath (file count) and
// maxTotalBytesInPath (total accumulated bytes). The context is checked before each
// recursive call and file fetch to respect cancellation.
func GetAllFilesInPath(ctx context.Context, client FileReader, owner, repo, path string) (map[string]string, error) {
results := make(map[string]string)
gpt-review-bot commented 2026-05-12 19:58:18 +00:00
Review
Copy Link
Copy Source

[MINOR] GetAllFilesInPath assumes ListContents works for both directories and files. If a FileReader implementation cannot list a file path (only directories), this will return an error rather than fetching the single file content. Consider documenting this requirement or adding a fallback to GetFileContent when ListContents indicates the path is a file.

**[MINOR]** GetAllFilesInPath assumes ListContents works for both directories and files. If a FileReader implementation cannot list a file path (only directories), this will return an error rather than fetching the single file content. Consider documenting this requirement or adding a fallback to GetFileContent when ListContents indicates the path is a file.
totalBytes := 0
var walk func(string) error
walk = func(dir string) error {
if err := ctx.Err(); err != nil {
return fmt.Errorf("context canceled during traversal: %w", err)
}
entries, err := client.ListContents(ctx, owner, repo, dir)
sonnet-review-bot commented 2026-05-12 20:08:36 +00:00
Review
Copy Link
Copy Source

[MINOR] There is a functional duplication between vcs.GetAllFilesInPath and gitea.Client.GetAllFilesInPath. The latter already has identical logic (though with warn-and-continue error handling instead of fail-fast). The doc comment on the vcs version explicitly calls out the difference, which is good, but over time both implementations will need to be kept in sync. Consider whether gitea.Client.GetAllFilesInPath should be refactored to delegate to vcs.GetAllFilesInPath via the vcs.FileReader interface (which gitea.Client will satisfy in Phase 2) to eliminate the duplication entirely.

**[MINOR]** There is a functional duplication between `vcs.GetAllFilesInPath` and `gitea.Client.GetAllFilesInPath`. The latter already has identical logic (though with warn-and-continue error handling instead of fail-fast). The doc comment on the vcs version explicitly calls out the difference, which is good, but over time both implementations will need to be kept in sync. Consider whether `gitea.Client.GetAllFilesInPath` should be refactored to delegate to `vcs.GetAllFilesInPath` via the `vcs.FileReader` interface (which `gitea.Client` will satisfy in Phase 2) to eliminate the duplication entirely.
if err != nil {
gpt-review-bot commented 2026-05-12 19:58:18 +00:00
Review
Copy Link
Copy Source

[NIT] Error messages use "context cancelled" (double-l) while the standard library uses "context canceled". This is harmless but can make grepping for cancellations less consistent. Consider aligning wording to the stdlib spelling.

**[NIT]** Error messages use "context cancelled" (double-l) while the standard library uses "context canceled". This is harmless but can make grepping for cancellations less consistent. Consider aligning wording to the stdlib spelling.
return fmt.Errorf("list contents %q: %w", dir, err)
}
sonnet-review-bot commented 2026-05-12 19:57:18 +00:00
Review
Copy Link
Copy Source

[MINOR] GetAllFilesInPath does not handle the case where path itself is a file rather than a directory. When ListContents returns a single-element slice containing the file itself (which is how Gitea behaves when the path is a file — see gitea.Client.ListContents which normalises the single-object response to a slice), the walk function will call GetFileContent for that entry and store it with entry.Path as the key, which works correctly. However if the API returns a 404 for the path on ListContents (e.g., a bare filename in a flat repo), the function will immediately return a 'list contents' error rather than trying to fetch the file directly, unlike gitea.Client.GetAllFilesInPath. This difference in behaviour is documented in the doc comment ('fail-fast'), but callers that previously relied on the gitea fallback (single-file fetch on 404) will silently get an error instead. Worth verifying that all call-sites only pass directory paths.

**[MINOR]** GetAllFilesInPath does not handle the case where `path` itself is a file rather than a directory. When `ListContents` returns a single-element slice containing the file itself (which is how Gitea behaves when the path is a file — see gitea.Client.ListContents which normalises the single-object response to a slice), the walk function will call GetFileContent for that entry and store it with entry.Path as the key, which works correctly. However if the API returns a 404 for the path on ListContents (e.g., a bare filename in a flat repo), the function will immediately return a 'list contents' error rather than trying to fetch the file directly, unlike gitea.Client.GetAllFilesInPath. This difference in behaviour is documented in the doc comment ('fail-fast'), but callers that previously relied on the gitea fallback (single-file fetch on 404) will silently get an error instead. Worth verifying that all call-sites only pass directory paths.
for _, entry := range entries {
if err := ctx.Err(); err != nil {
return fmt.Errorf("context canceled during traversal: %w", err)
security-review-bot marked this conversation as resolved Outdated
security-review-bot commented 2026-05-12 19:40:39 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] BuildLineToPositionMap splits the entire diff into lines and iterates without any size checks. Very large diffs could cause high memory/cpu usage. Consider validating input size or imposing time/size limits when processing untrusted diffs.

**[MINOR]** BuildLineToPositionMap splits the entire diff into lines and iterates without any size checks. Very large diffs could cause high memory/cpu usage. Consider validating input size or imposing time/size limits when processing untrusted diffs.
}
switch entry.Type {
case "file":
if len(results) >= maxFilesInPath {
return fmt.Errorf("exceeded max file count (%d) in path %q", maxFilesInPath, path)
}
sonnet-review-bot commented 2026-05-12 19:32:18 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The doc comment uses → (Unicode arrow) in the Returns line. This is fine for readability but slightly inconsistent with stdlib conventions which typically use plain ASCII (->) in godoc. Not a real issue.

**[NIT]** The doc comment uses `→` (Unicode arrow) in the Returns line. This is fine for readability but slightly inconsistent with stdlib conventions which typically use plain ASCII (`->`) in godoc. Not a real issue.
content, err := client.GetFileContent(ctx, owner, repo, entry.Path, "")
if err != nil {
return fmt.Errorf("get file %q: %w", entry.Path, err)
}
totalBytes += len(content)
if totalBytes > maxTotalBytesInPath {
return fmt.Errorf("exceeded max total bytes (%d) in path %q", maxTotalBytesInPath, path)
sonnet-review-bot commented 2026-05-12 19:57:18 +00:00
Review
Copy Link
Copy Source

[NIT] The walk closure captures path (the original root path) by reference for use in error messages about resource limits. This is correct and intentional, but it's subtle — path is used only in the limit-exceeded error messages, not in the traversal logic. A comment noting this would improve readability.

**[NIT]** The walk closure captures `path` (the original root path) by reference for use in error messages about resource limits. This is correct and intentional, but it's subtle — path is used only in the limit-exceeded error messages, not in the traversal logic. A comment noting this would improve readability.
}
results[entry.Path] = content
security-review-bot marked this conversation as resolved Outdated
security-review-bot commented 2026-05-12 19:32:43 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] BuildLineToPositionMap processes the entire diff string without any size checks. If diff input is attacker-controlled and very large, it could still lead to high memory/CPU usage. While operations are linear, adding input size limits or early termination would harden against DoS.

**[MINOR]** BuildLineToPositionMap processes the entire diff string without any size checks. If diff input is attacker-controlled and very large, it could still lead to high memory/CPU usage. While operations are linear, adding input size limits or early termination would harden against DoS.
case "dir":
if err := walk(entry.Path); err != nil {
return err
}
}
}
return nil
gpt-review-bot commented 2026-05-12 19:40:40 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] BuildLineToPositionMap implicitly ignores '\ No newline at end of file' markers. Consider explicitly checking and skipping such lines for clarity and future maintainability.

**[NIT]** BuildLineToPositionMap implicitly ignores '\ No newline at end of file' markers. Consider explicitly checking and skipping such lines for clarity and future maintainability.
}
if err := walk(path); err != nil {
return nil, err
}
gpt-review-bot commented 2026-05-12 19:34:15 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The metadata skip includes any line starting with a backslash ("\"). While intended for the "\ No newline at end of file" marker, being explicit (checking for that exact string) would avoid inadvertently skipping any other backslash-prefixed lines in future formats.

**[MINOR]** The metadata skip includes any line starting with a backslash ("\\"). While intended for the "\ No newline at end of file" marker, being explicit (checking for that exact string) would avoid inadvertently skipping any other backslash-prefixed lines in future formats.
return results, nil
}
// BuildLineToPositionMap parses a unified diff and returns a map of
// filename -> (new line number -> diff position). The diff position is a
// 1-indexed offset from the @@ hunk header line for each file.
// Only lines that appear in the new file (context lines and additions) are mapped.
sonnet-review-bot commented 2026-05-12 19:32:18 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The \ prefix check used to skip \ No newline at end of file markers (strings.HasPrefix(line, "\\")) would also inadvertently skip any other backslash-prefixed diff metadata lines. This is unlikely to be a problem in practice since unified diffs don't use backslash-prefixed lines for anything other than this marker, but a more precise check like strings.HasPrefix(line, "\\ ") (with trailing space) or matching the full string would be more defensive.

**[MINOR]** The `\` prefix check used to skip `\ No newline at end of file` markers (`strings.HasPrefix(line, "\\")`) would also inadvertently skip any other backslash-prefixed diff metadata lines. This is unlikely to be a problem in practice since unified diffs don't use backslash-prefixed lines for anything other than this marker, but a more precise check like `strings.HasPrefix(line, "\\ ")` (with trailing space) or matching the full string would be more defensive.
// Deletion-only lines are not included.
func BuildLineToPositionMap(diff string) map[string]map[int]int {
result := make(map[string]map[int]int)
lines := strings.Split(diff, "\n")
var currentFile string
var position int
var newLine int
sonnet-review-bot commented 2026-05-12 19:39:28 +00:00
Review
Copy Link
Copy Source

[MINOR] The \ No newline at end of file marker handling mentioned in the PR description is not actually implemented in BuildLineToPositionMap. Lines starting with \ are not explicitly skipped. In practice they won't be counted (they don't start with +, -, or space) so it works correctly by accident, but this should either be made explicit with a comment or an explicit strings.HasPrefix(line, "\\") continue guard for clarity and robustness.

**[MINOR]** The `\ No newline at end of file` marker handling mentioned in the PR description is not actually implemented in BuildLineToPositionMap. Lines starting with `\` are not explicitly skipped. In practice they won't be counted (they don't start with +, -, or space) so it works correctly by accident, but this should either be made explicit with a comment or an explicit `strings.HasPrefix(line, "\\")` continue guard for clarity and robustness.
sonnet-review-bot commented 2026-05-12 19:39:28 +00:00
Review
Copy Link
Copy Source

[MINOR] BuildLineToPositionMap skips context lines when position == 0 (the guard if position > 0). This is correct for lines before the first hunk header, but it silently drops context lines if a diff somehow starts with a space-prefixed line before any @@ hunk. The condition also means context lines at the very beginning of the first hunk aren't counted if the hunk header somehow wasn't seen. The logic is correct for well-formed unified diffs but the guard is subtle and deserves a comment explaining why.

**[MINOR]** BuildLineToPositionMap skips context lines when `position == 0` (the guard `if position > 0`). This is correct for lines before the first hunk header, but it silently drops context lines if a diff somehow starts with a space-prefixed line before any @@ hunk. The condition also means context lines at the very beginning of the first hunk aren't counted if the hunk header somehow wasn't seen. The logic is correct for well-formed unified diffs but the guard is subtle and deserves a comment explaining why.
for _, line := range lines {
// Detect new file in diff
if strings.HasPrefix(line, "+++ b/") {
currentFile = strings.TrimPrefix(line, "+++ b/")
position = 0
newLine = 0
if result[currentFile] == nil {
result[currentFile] = make(map[int]int)
}
continue
}
sonnet-review-bot commented 2026-05-12 19:32:18 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The parseHunkNewStart function finds the first + in the hunk line using strings.Index(hunkLine, "+"). A unified diff hunk header is formatted as @@ -old +new @@, so the + reliably appears after the @@ preamble. However, if the function signature heading (after the trailing @@) contains a + character, this would still be correct since strings.Index finds the first occurrence and the +new_start always precedes the section heading. The bigger subtlety: for a hunk like @@ -0,0 +1 @@ (new file, single line added), idx in strings.IndexAny(rest, ", @") would find @ at some position — but only if there's a space before @@. If rest is just "1", IndexAny returns -1 and Atoi works correctly. The guard idx > 0 (strictly greater than) would miss the case where idx == 0, i.e., the + is immediately followed by a delimiter. In practice this can't happen in valid unified diffs (you always have at least one digit), but the condition should arguably be idx >= 0 to be safe. This is very unlikely to matter in practice.

**[MINOR]** The `parseHunkNewStart` function finds the first `+` in the hunk line using `strings.Index(hunkLine, "+")`. A unified diff hunk header is formatted as `@@ -old +new @@`, so the `+` reliably appears after the `@@` preamble. However, if the function signature heading (after the trailing `@@`) contains a `+` character, this would still be correct since `strings.Index` finds the *first* occurrence and the `+new_start` always precedes the section heading. The bigger subtlety: for a hunk like `@@ -0,0 +1 @@` (new file, single line added), `idx` in `strings.IndexAny(rest, ", @")` would find `@` at some position — but only if there's a space before `@@`. If `rest` is just `"1"`, `IndexAny` returns -1 and `Atoi` works correctly. The guard `idx > 0` (strictly greater than) would miss the case where `idx == 0`, i.e., the `+` is immediately followed by a delimiter. In practice this can't happen in valid unified diffs (you always have at least one digit), but the condition should arguably be `idx >= 0` to be safe. This is very unlikely to matter in practice.
// Skip --- lines (old file header)
if strings.HasPrefix(line, "--- ") {
continue
}
// Skip diff --git lines
if strings.HasPrefix(line, "diff --git") {
continue
}
sonnet-review-bot commented 2026-05-12 19:39:28 +00:00
Review
Copy Link
Copy Source

[NIT] parseHunkNewStart builds a string by concatenating single characters (numStr += string(ch)), causing repeated small allocations. For a hot parsing path this is negligible, but using strconv.Atoi after extracting the substring (or using a byte-slice accumulator) would be both simpler and more idiomatic.

**[NIT]** parseHunkNewStart builds a string by concatenating single characters (`numStr += string(ch)`), causing repeated small allocations. For a hot parsing path this is negligible, but using `strconv.Atoi` after extracting the substring (or using a byte-slice accumulator) would be both simpler and more idiomatic.
// Skip index lines
if strings.HasPrefix(line, "index ") {
continue
}
// Parse hunk headers
if strings.HasPrefix(line, "@@") {
position++
// Extract new file start line from @@ -a,b +c,d @@
newLine = parseHunkNewStart(line)
continue
}
// We need a current file to map lines
if currentFile == "" {
continue
}
// Skip "\ No newline at end of file" markers — these are git diff
// metadata and not part of the file content.
if strings.HasPrefix(line, `\`) {
continue
}
// Process diff content lines
if strings.HasPrefix(line, "+") {
position++
result[currentFile][newLine] = position
newLine++
} else if strings.HasPrefix(line, "-") {
position++
// Deletion lines don't map to new line numbers
} else if strings.HasPrefix(line, " ") {
// Context line (space-prefixed).
// Only map if position > 0, which means we've seen a hunk header.
// Lines before the first hunk header (position == 0) are not part
// of any diff hunk and should be skipped.
if position > 0 {
position++
result[currentFile][newLine] = position
newLine++
}
}
}
return result
}
// parseHunkNewStart extracts the new-file starting line number from a hunk header.
// Format: @@ -old_start[,old_count] +new_start[,new_count] @@
func parseHunkNewStart(hunkLine string) int {
// Find the +N part
plusIdx := strings.Index(hunkLine, "+")
if plusIdx < 0 {
return 1
}
rest := hunkLine[plusIdx+1:]
// Find the end of the number (first non-digit after +)
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
}
vcs/util_test.go
+331
View File
@@ -0,0 +1,331 @@
package vcs_test
sonnet-review-bot commented 2026-05-12 19:39:28 +00:00
Review
Copy Link
Copy Source

[NIT] TestGetAllFilesInPath does not test error propagation (ListContents error and GetFileContent error cases), which are explicitly mentioned in the PR description. The description says these tests exist, but they are missing from the added test file. The PR description is inaccurate — the missing tests were promised but omitted.

**[NIT]** TestGetAllFilesInPath does not test error propagation (ListContents error and GetFileContent error cases), which are explicitly mentioned in the PR description. The description says these tests exist, but they are missing from the added test file. The PR description is inaccurate — the missing tests were promised but omitted.
import (
"context"
"fmt"
sonnet-review-bot commented 2026-05-12 19:57:18 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] Import ordering: fmt is placed between strings and testing rather than in the stdlib block before them. goimports would reorder these. The conventions doc requires go vet ./... to pass but goimports/gofmt ordering should also be maintained. Not a compilation error but violates the canonical import grouping pattern documented in style.md.

**[MINOR]** Import ordering: `fmt` is placed between `strings` and `testing` rather than in the stdlib block before them. goimports would reorder these. The conventions doc requires `go vet ./...` to pass but goimports/gofmt ordering should also be maintained. Not a compilation error but violates the canonical import grouping pattern documented in style.md.
"strings"
"testing"
"gitea.weiker.me/rodin/review-bot/vcs"
)
// mockFileReader implements vcs.FileReader for testing.
type mockFileReader struct {
contents map[string][]vcs.ContentEntry // path -> entries
files map[string]string // path -> content
}
func (m *mockFileReader) GetFileContent(ctx context.Context, owner, repo, path, ref string) (string, error) {
content, ok := m.files[path]
if !ok {
return "", fmt.Errorf("HTTP 404: file not found: %s", path)
}
return content, nil
}
func (m *mockFileReader) ListContents(ctx context.Context, owner, repo, path string) ([]vcs.ContentEntry, error) {
entries, ok := m.contents[path]
if !ok {
return nil, fmt.Errorf("HTTP 404: path not found: %s", path)
}
return entries, nil
}
func TestGetAllFilesInPath(t *testing.T) {
ctx := context.Background()
t.Run("empty directory", func(t *testing.T) {
client := &mockFileReader{
contents: map[string][]vcs.ContentEntry{
"src": {},
},
}
result, err := vcs.GetAllFilesInPath(ctx, client, "owner", "repo", "src")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if len(result) != 0 {
t.Errorf("expected empty map, got %d entries", len(result))
}
})
t.Run("flat directory", func(t *testing.T) {
client := &mockFileReader{
contents: map[string][]vcs.ContentEntry{
"src": {
{Name: "main.go", Path: "src/main.go", Type: "file"},
{Name: "util.go", Path: "src/util.go", Type: "file"},
},
},
files: map[string]string{
"src/main.go": "package main",
"src/util.go": "package main\n// util",
},
}
result, err := vcs.GetAllFilesInPath(ctx, client, "owner", "repo", "src")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if len(result) != 2 {
t.Fatalf("expected 2 files, got %d", len(result))
}
if result["src/main.go"] != "package main" {
t.Errorf("main.go content = %q", result["src/main.go"])
}
if result["src/util.go"] != "package main\n// util" {
t.Errorf("util.go content = %q", result["src/util.go"])
}
})
t.Run("nested directories", func(t *testing.T) {
client := &mockFileReader{
contents: map[string][]vcs.ContentEntry{
"src": {
{Name: "main.go", Path: "src/main.go", Type: "file"},
{Name: "pkg", Path: "src/pkg", Type: "dir"},
},
"src/pkg": {
{Name: "lib.go", Path: "src/pkg/lib.go", Type: "file"},
{Name: "sub", Path: "src/pkg/sub", Type: "dir"},
},
"src/pkg/sub": {
{Name: "deep.go", Path: "src/pkg/sub/deep.go", Type: "file"},
},
},
files: map[string]string{
"src/main.go": "package main",
"src/pkg/lib.go": "package pkg",
"src/pkg/sub/deep.go": "package sub",
},
}
result, err := vcs.GetAllFilesInPath(ctx, client, "owner", "repo", "src")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if len(result) != 3 {
t.Fatalf("expected 3 files, got %d", len(result))
}
if result["src/main.go"] != "package main" {
t.Errorf("main.go content = %q", result["src/main.go"])
}
if result["src/pkg/lib.go"] != "package pkg" {
t.Errorf("lib.go content = %q", result["src/pkg/lib.go"])
}
if result["src/pkg/sub/deep.go"] != "package sub" {
t.Errorf("deep.go content = %q", result["src/pkg/sub/deep.go"])
}
})
t.Run("mixed files and dirs", func(t *testing.T) {
client := &mockFileReader{
contents: map[string][]vcs.ContentEntry{
"root": {
{Name: "README.md", Path: "root/README.md", Type: "file"},
{Name: "docs", Path: "root/docs", Type: "dir"},
{Name: "config.yaml", Path: "root/config.yaml", Type: "file"},
},
"root/docs": {
{Name: "guide.md", Path: "root/docs/guide.md", Type: "file"},
},
},
files: map[string]string{
"root/README.md": "# Hello",
"root/config.yaml": "key: value",
"root/docs/guide.md": "## Guide",
},
}
result, err := vcs.GetAllFilesInPath(ctx, client, "owner", "repo", "root")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if len(result) != 3 {
t.Fatalf("expected 3 files, got %d", len(result))
}
if result["root/README.md"] != "# Hello" {
t.Errorf("README content = %q", result["root/README.md"])
}
if result["root/docs/guide.md"] != "## Guide" {
t.Errorf("guide content = %q", result["root/docs/guide.md"])
}
})
}
func TestBuildLineToPositionMap(t *testing.T) {
t.Run("single hunk", func(t *testing.T) {
diff := "diff --git a/file.go b/file.go\nindex abc..def 100644\n--- a/file.go\n+++ b/file.go\n@@ -1,3 +1,4 @@\n package main\n \n+// new comment\n func main() {}\n"
result := vcs.BuildLineToPositionMap(diff)
fileMap, ok := result["file.go"]
if !ok {
t.Fatal("expected file.go in result")
}
// Hunk header @@ is position 1
// Line 1: " package main" -> position 2
if fileMap[1] != 2 {
t.Errorf("line 1 position = %d, want 2", fileMap[1])
}
// Line 2: " " (context) -> position 3
if fileMap[2] != 3 {
t.Errorf("line 2 position = %d, want 3", fileMap[2])
}
// Line 3: "+// new comment" -> position 4
if fileMap[3] != 4 {
t.Errorf("line 3 position = %d, want 4", fileMap[3])
}
// Line 4: " func main() {}" -> position 5
if fileMap[4] != 5 {
t.Errorf("line 4 position = %d, want 5", fileMap[4])
}
})
t.Run("multi hunk", func(t *testing.T) {
diff := "diff --git a/file.go b/file.go\n--- a/file.go\n+++ b/file.go\n@@ -1,3 +1,3 @@\n package main\n \n-// old\n+// new\n@@ -10,3 +10,4 @@\n func foo() {\n+\t// added\n \treturn\n }\n"
result := vcs.BuildLineToPositionMap(diff)
fileMap, ok := result["file.go"]
if !ok {
t.Fatal("expected file.go in result")
}
// First hunk: @@ is position 1
// Line 1: " package main" -> position 2
if fileMap[1] != 2 {
t.Errorf("line 1 position = %d, want 2", fileMap[1])
}
// Line 3: "+// new" -> position 5 (after " ", "-// old" at pos 3,4)
if fileMap[3] != 5 {
t.Errorf("line 3 position = %d, want 5", fileMap[3])
}
sonnet-review-bot commented 2026-05-12 19:32:18 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The TestBuildLineToPositionMap_MultiHunk test verifies expected positions but doesn't check len(fileMap) the way TestBuildLineToPositionMap_SingleHunk does. Adding a length check would make the test stricter and catch any spurious extra entries.

**[NIT]** The `TestBuildLineToPositionMap_MultiHunk` test verifies expected positions but doesn't check `len(fileMap)` the way `TestBuildLineToPositionMap_SingleHunk` does. Adding a length check would make the test stricter and catch any spurious extra entries.
// Second hunk: @@ is position 6
// Line 10: " func foo() {" -> position 7
if fileMap[10] != 7 {
t.Errorf("line 10 position = %d, want 7", fileMap[10])
}
// Line 11: "+\t// added" -> position 8
if fileMap[11] != 8 {
t.Errorf("line 11 position = %d, want 8", fileMap[11])
}
})
t.Run("deletion lines not in map", func(t *testing.T) {
diff := "diff --git a/file.go b/file.go\n--- a/file.go\n+++ b/file.go\n@@ -1,4 +1,3 @@\n package main\n \n-// deleted line\n func main() {}\n"
result := vcs.BuildLineToPositionMap(diff)
fileMap, ok := result["file.go"]
if !ok {
t.Fatal("expected file.go in result")
}
// Line 1: " package main" -> position 2
if fileMap[1] != 2 {
t.Errorf("line 1 position = %d, want 2", fileMap[1])
}
// Line 3 in new file: " func main() {}" -> position 5 (after deletion at pos 4)
if fileMap[3] != 5 {
t.Errorf("line 3 position = %d, want 5", fileMap[3])
}
// Should only have 3 entries (lines 1, 2, 3 of new file)
if len(fileMap) != 3 {
t.Errorf("expected 3 mapped lines, got %d: %v", len(fileMap), fileMap)
}
})
t.Run("multiple files", func(t *testing.T) {
diff := "diff --git a/a.go b/a.go\n--- a/a.go\n+++ b/a.go\n@@ -1,2 +1,3 @@\n package a\n \n+// file a\ndiff --git a/b.go b/b.go\n--- a/b.go\n+++ b/b.go\n@@ -1,2 +1,3 @@\n package b\n \n+// file b\n"
result := vcs.BuildLineToPositionMap(diff)
if len(result) != 2 {
t.Fatalf("expected 2 files, got %d", len(result))
}
aMap, ok := result["a.go"]
if !ok {
t.Fatal("expected a.go in result")
}
bMap, ok := result["b.go"]
if !ok {
t.Fatal("expected b.go in result")
}
// a.go line 3: "+// file a" -> position 4
if aMap[3] != 4 {
t.Errorf("a.go line 3 position = %d, want 4", aMap[3])
}
// b.go line 3: "+// file b" -> position 4
if bMap[3] != 4 {
t.Errorf("b.go line 3 position = %d, want 4", bMap[3])
}
})
}
func TestGetAllFilesInPath_ErrorPropagation(t *testing.T) {
ctx := context.Background()
t.Run("ListContents error propagates", func(t *testing.T) {
client := &mockFileReader{
contents: map[string][]vcs.ContentEntry{
// "src" not in map, so ListContents will fail
},
}
_, err := vcs.GetAllFilesInPath(ctx, client, "owner", "repo", "src")
if err == nil {
t.Fatal("expected error, got nil")
}
if !strings.Contains(err.Error(), "list contents") {
t.Errorf("expected error about list contents, got: %v", err)
}
})
t.Run("GetFileContent error propagates", func(t *testing.T) {
client := &mockFileReader{
contents: map[string][]vcs.ContentEntry{
"src": {
{Name: "main.go", Path: "src/main.go", Type: "file"},
},
},
files: map[string]string{
// "src/main.go" not in files map, so GetFileContent will fail
},
}
_, err := vcs.GetAllFilesInPath(ctx, client, "owner", "repo", "src")
if err == nil {
t.Fatal("expected error, got nil")
}
if !strings.Contains(err.Error(), "get file") {
t.Errorf("expected error about get file, got: %v", err)
}
})
t.Run("nested ListContents error propagates", func(t *testing.T) {
client := &mockFileReader{
contents: map[string][]vcs.ContentEntry{
"src": {
{Name: "pkg", Path: "src/pkg", Type: "dir"},
},
// "src/pkg" not in map, so recursive ListContents will fail
},
}
_, err := vcs.GetAllFilesInPath(ctx, client, "owner", "repo", "src")
if err == nil {
t.Fatal("expected error, got nil")
}
if !strings.Contains(err.Error(), "list contents") {
t.Errorf("expected error about list contents, got: %v", err)
}
})
t.Run("canceled context propagates", func(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
cancel() // Cancel immediately
client := &mockFileReader{
contents: map[string][]vcs.ContentEntry{
"src": {
{Name: "main.go", Path: "src/main.go", Type: "file"},
},
},
files: map[string]string{
"src/main.go": "package main",
},
}
_, err := vcs.GetAllFilesInPath(ctx, client, "owner", "repo", "src")
if err == nil {
t.Fatal("expected error from canceled context, got nil")
}
if !strings.Contains(err.Error(), "context canceled") {
t.Errorf("expected context cancellation error, got: %v", err)
}
})
}