From 7c83365fc46f872058c97b98354232c51473656c Mon Sep 17 00:00:00 2001 From: claw Date: Tue, 12 May 2026 12:38:21 -0700 Subject: [PATCH 1/3] =?UTF-8?q?feat(vcs):=20complete=20Phase=201=20?= =?UTF-8?q?=E2=80=94=20util.go,=20type=20cleanup,=20interface=20additions?= =?UTF-8?q?=20(fixes=20#84,=20#85,=20#86)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Create vcs/util.go with GetAllFilesInPath and BuildLineToPositionMap - Create vcs/util_test.go with comprehensive tests for both functions - Remove review.ContentEntry type, replace with vcs.ContentEntry - Remove review.GiteaClient interface, replace with vcs.FileReader - Update review/repo_persona.go to use vcs.FileReader - Update review/repo_persona_test.go to use vcs.ContentEntry - Update cmd/review-bot/main.go adapter to implement vcs.FileReader - Add Number and Base fields to vcs.PullRequest - Add CommitStatus type to vcs/types.go - Add GetFileContentAtRef to vcs.PRReader interface - Add GetCommitStatuses to vcs.PRReader interface - Add DismissReview to vcs.Reviewer interface - Add stub implementations on gitea.Client for new interface methods Closes #84, Closes #85, Closes #86 --- cmd/review-bot/main.go | 14 +- gitea/client.go | 12 ++ review/repo_persona.go | 21 +-- review/repo_persona_test.go | 93 +++++--------- vcs/interfaces.go | 3 + vcs/types.go | 21 ++- vcs/util.go | 145 +++++++++++++++++++++ vcs/util_test.go | 250 ++++++++++++++++++++++++++++++++++++ 8 files changed, 472 insertions(+), 87 deletions(-) create mode 100644 vcs/util.go create mode 100644 vcs/util_test.go diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index cc6381a..44f1306 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -15,6 +15,7 @@ import ( "gitea.weiker.me/rodin/review-bot/gitea" "gitea.weiker.me/rodin/review-bot/llm" "gitea.weiker.me/rodin/review-bot/review" + "gitea.weiker.me/rodin/review-bot/vcs" ) var version = "dev" @@ -812,7 +813,7 @@ func shouldSkipStaleReview(evaluatedSHA, currentSHA string) bool { return evaluatedSHA != currentSHA } -// giteaClientAdapter adapts gitea.Client to review.GiteaClient interface. +// giteaClientAdapter adapts gitea.Client to vcs.FileReader interface. type giteaClientAdapter struct { client *gitea.Client } @@ -821,14 +822,14 @@ func newGiteaClientAdapter(c *gitea.Client) *giteaClientAdapter { 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) if err != nil { return nil, err } - result := make([]review.ContentEntry, len(entries)) + result := make([]vcs.ContentEntry, len(entries)) for i, e := range entries { - result[i] = review.ContentEntry{ + result[i] = vcs.ContentEntry{ Name: e.Name, Path: e.Path, Type: e.Type, @@ -837,6 +838,9 @@ func (a *giteaClientAdapter) ListContents(ctx context.Context, owner, repo, path return result, nil } -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) { + if ref != "" { + return a.client.GetFileContentRef(ctx, owner, repo, filepath, ref) + } return a.client.GetFileContent(ctx, owner, repo, filepath) } diff --git a/gitea/client.go b/gitea/client.go index 55835ac..74fce56 100644 --- a/gitea/client.go +++ b/gitea/client.go @@ -831,3 +831,15 @@ func (c *Client) ResolveComment(ctx context.Context, owner, repo string, comment } return nil } + +// 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("DismissReview: not implemented") +} + +// 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) +} diff --git a/review/repo_persona.go b/review/repo_persona.go index 58c0b9d..5b0fe6e 100644 --- a/review/repo_persona.go +++ b/review/repo_persona.go @@ -4,32 +4,19 @@ import ( "context" "log/slog" "strings" + + "gitea.weiker.me/rodin/review-bot/vcs" ) // RepoPersonaPath is the directory path where repo-specific personas are stored. 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. // 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. // Auth errors and other non-404 errors are propagated. // 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) entries, err := client.ListContents(ctx, owner, repo, RepoPersonaPath) @@ -57,7 +44,7 @@ func LoadRepoPersonas(ctx context.Context, client GiteaClient, owner, repo strin continue } - content, err := client.GetFileContent(ctx, owner, repo, entry.Path) + content, err := client.GetFileContent(ctx, owner, repo, entry.Path, "") if err != nil { slog.Warn("could not fetch repo persona file", "file", entry.Path, diff --git a/review/repo_persona_test.go b/review/repo_persona_test.go index 56b0a59..43652b5 100644 --- a/review/repo_persona_test.go +++ b/review/repo_persona_test.go @@ -5,23 +5,21 @@ import ( "errors" "strings" "testing" + + "gitea.weiker.me/rodin/review-bot/vcs" ) func TestParsePersonaBytes(t *testing.T) { tests := []struct { - name string - data string - source string - wantName string - wantErr string + name string + data string + source string + wantName string + wantErr string }{ { name: "valid yaml", - data: `name: test -identity: test identity -focus: - - testing -`, + data: "name: test\nidentity: test identity\nfocus:\n - testing\n", source: "test.yaml", wantName: "test", }, @@ -38,8 +36,8 @@ focus: wantErr: "parse", }, { - name: "json format by extension", - data: `{"name": "jsontest", "identity": "json identity"}`, + name: "json format by extension", + data: `{"name": "jsontest", "identity": "json identity"}`, source: "test.json", wantName: "jsontest", }, @@ -67,15 +65,15 @@ focus: } } -// mockGiteaClient implements GiteaClient for testing. +// mockGiteaClient implements vcs.FileReader for testing. type mockGiteaClient struct { - contents map[string][]ContentEntry // path -> entries - files map[string]string // path -> content + contents map[string][]vcs.ContentEntry // path -> entries + files map[string]string // path -> content listErr 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 { return nil, m.listErr } @@ -86,7 +84,7 @@ func (m *mockGiteaClient) ListContents(ctx context.Context, owner, repo, path st 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 err, ok := m.fileErr[filepath]; ok { return "", err @@ -118,7 +116,7 @@ func TestLoadRepoPersonas(t *testing.T) { t.Run("empty directory returns empty map", func(t *testing.T) { client := &mockGiteaClient{ - contents: map[string][]ContentEntry{ + contents: map[string][]vcs.ContentEntry{ RepoPersonaPath: {}, }, } @@ -133,27 +131,15 @@ func TestLoadRepoPersonas(t *testing.T) { t.Run("loads valid personas", func(t *testing.T) { client := &mockGiteaClient{ - contents: map[string][]ContentEntry{ + contents: map[string][]vcs.ContentEntry{ RepoPersonaPath: { {Name: "trading.yaml", Path: ".review-bot/personas/trading.yaml", Type: "file"}, {Name: "crypto.yaml", Path: ".review-bot/personas/crypto.yaml", Type: "file"}, }, }, files: map[string]string{ - ".review-bot/personas/trading.yaml": `name: trading -display_name: Trading Expert -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 -`, + ".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", + ".review-bot/personas/crypto.yaml": "name: crypto\ndisplay_name: Crypto Expert\nidentity: You are a cryptography expert.\nfocus:\n - key management\n - encryption\n", }, } personas, err := LoadRepoPersonas(ctx, client, "owner", "repo") @@ -176,16 +162,14 @@ focus: t.Run("skips invalid persona files", func(t *testing.T) { client := &mockGiteaClient{ - contents: map[string][]ContentEntry{ + contents: map[string][]vcs.ContentEntry{ RepoPersonaPath: { {Name: "valid.yaml", Path: ".review-bot/personas/valid.yaml", Type: "file"}, {Name: "invalid.yaml", Path: ".review-bot/personas/invalid.yaml", Type: "file"}, }, }, files: map[string]string{ - ".review-bot/personas/valid.yaml": `name: valid -identity: Valid persona -`, + ".review-bot/personas/valid.yaml": "name: valid\nidentity: Valid persona\n", ".review-bot/personas/invalid.yaml": "not valid yaml: [broken", }, } @@ -193,7 +177,6 @@ identity: Valid persona if err != nil { t.Fatalf("unexpected error: %v", err) } - // Should have the valid one, skip the invalid if len(personas) != 1 { 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) { client := &mockGiteaClient{ - contents: map[string][]ContentEntry{ + contents: map[string][]vcs.ContentEntry{ RepoPersonaPath: { {Name: "persona.yaml", Path: ".review-bot/personas/persona.yaml", Type: "file"}, {Name: "README.md", Path: ".review-bot/personas/README.md", Type: "file"}, @@ -212,10 +195,8 @@ identity: Valid persona }, }, files: map[string]string{ - ".review-bot/personas/persona.yaml": `name: test -identity: Test persona -`, - ".review-bot/personas/README.md": "# Personas\n\nPut your personas here.", + ".review-bot/personas/persona.yaml": "name: test\nidentity: Test persona\n", + ".review-bot/personas/README.md": "# Personas\n\nPut your personas here.", }, } personas, err := LoadRepoPersonas(ctx, client, "owner", "repo") @@ -229,16 +210,14 @@ identity: Test persona t.Run("skips subdirectories", func(t *testing.T) { client := &mockGiteaClient{ - contents: map[string][]ContentEntry{ + contents: map[string][]vcs.ContentEntry{ RepoPersonaPath: { {Name: "persona.yaml", Path: ".review-bot/personas/persona.yaml", Type: "file"}, {Name: "subdir", Path: ".review-bot/personas/subdir", Type: "dir"}, }, }, files: map[string]string{ - ".review-bot/personas/persona.yaml": `name: test -identity: Test persona -`, + ".review-bot/personas/persona.yaml": "name: test\nidentity: Test persona\n", }, } 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) { client := &mockGiteaClient{ - contents: map[string][]ContentEntry{ + contents: map[string][]vcs.ContentEntry{ RepoPersonaPath: { {Name: "good.yaml", Path: ".review-bot/personas/good.yaml", Type: "file"}, {Name: "bad.yaml", Path: ".review-bot/personas/bad.yaml", Type: "file"}, }, }, files: map[string]string{ - ".review-bot/personas/good.yaml": `name: good -identity: Good persona -`, + ".review-bot/personas/good.yaml": "name: good\nidentity: Good persona\n", }, fileErr: map[string]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) { - // Create a content string that exceeds MaxPersonaFileSize (64KB) oversizedContent := strings.Repeat("a", MaxPersonaFileSize+1) client := &mockGiteaClient{ - contents: map[string][]ContentEntry{ + contents: map[string][]vcs.ContentEntry{ RepoPersonaPath: { {Name: "normal.yaml", Path: ".review-bot/personas/normal.yaml", Type: "file"}, {Name: "huge.yaml", Path: ".review-bot/personas/huge.yaml", Type: "file"}, }, }, files: map[string]string{ - ".review-bot/personas/normal.yaml": `name: normal -identity: Normal sized persona -`, - ".review-bot/personas/huge.yaml": oversizedContent, + ".review-bot/personas/normal.yaml": "name: normal\nidentity: Normal sized persona\n", + ".review-bot/personas/huge.yaml": oversizedContent, }, } personas, err := LoadRepoPersonas(ctx, client, "owner", "repo") if err != nil { t.Fatalf("unexpected error: %v", err) } - // Should have the normal one, skip the oversized if len(personas) != 1 { 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") } - // Verify expected personas exist expected := []string{"security", "architect", "docs"} for _, name := range expected { if personas[name] == nil { @@ -378,7 +350,6 @@ func TestGetBuiltinPersonasMap(t *testing.T) { } } - // Verify personas are valid for name, p := range personas { if p.Name != name { t.Errorf("persona %q has mismatched name %q", name, p.Name) @@ -422,8 +393,6 @@ func TestIsNotFoundError(t *testing.T) { {nil, false}, {errors.New("HTTP 404: not found"), 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("HTTP 401: unauthorized"), false}, {errors.New("connection refused"), false}, diff --git a/vcs/interfaces.go b/vcs/interfaces.go index ecada13..3bc299e 100644 --- a/vcs/interfaces.go +++ b/vcs/interfaces.go @@ -10,6 +10,8 @@ type PRReader interface { GetPullRequest(ctx context.Context, owner, repo string, number int) (*PullRequest, error) GetPullRequestDiff(ctx context.Context, owner, repo string, number int) (string, 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) } // 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) ListReviews(ctx context.Context, owner, repo string, number int) ([]Review, 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. diff --git a/vcs/types.go b/vcs/types.go index 6dfd932..de904f3 100644 --- a/vcs/types.go +++ b/vcs/types.go @@ -15,6 +15,11 @@ const ( 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. type HeadRef struct { SHA string `json:"sha"` @@ -28,9 +33,11 @@ type UserInfo struct { // PullRequest holds relevant PR metadata. type PullRequest struct { - Title string `json:"title"` - Body string `json:"body"` - Head HeadRef `json:"head"` + Number int `json:"number"` + Title string `json:"title"` + Body string `json:"body"` + Head HeadRef `json:"head"` + Base BaseRef `json:"base"` } // ChangedFile represents a file modified in a PR. @@ -46,6 +53,14 @@ type ContentEntry struct { 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. type Review struct { ID int64 `json:"id"` diff --git a/vcs/util.go b/vcs/util.go new file mode 100644 index 0000000..9f133fd --- /dev/null +++ b/vcs/util.go @@ -0,0 +1,145 @@ +package vcs + +import ( + "context" + "fmt" + "strings" +) + +// 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. +func GetAllFilesInPath(ctx context.Context, client FileReader, owner, repo, path string) (map[string]string, error) { + results := make(map[string]string) + + entries, err := client.ListContents(ctx, owner, repo, path) + if err != nil { + return nil, fmt.Errorf("list contents %q: %w", path, err) + } + + for _, entry := range entries { + switch entry.Type { + case "file": + content, err := client.GetFileContent(ctx, owner, repo, entry.Path, "") + if err != nil { + return nil, fmt.Errorf("get file %q: %w", entry.Path, err) + } + results[entry.Path] = content + case "dir": + subResults, err := GetAllFilesInPath(ctx, client, owner, repo, entry.Path) + if err != nil { + return nil, fmt.Errorf("recurse into %q: %w", entry.Path, err) + } + for k, v := range subResults { + results[k] = v + } + } + } + + 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. +// 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 + + 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 + } + + // Skip --- lines (old file header) + if strings.HasPrefix(line, "--- ") { + continue + } + + // Skip diff --git lines + if strings.HasPrefix(line, "diff --git") { + continue + } + + // 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 + } + + // 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) + 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:] + + // Read digits until comma or space + var numStr string + for _, ch := range rest { + if ch >= '0' && ch <= '9' { + numStr += string(ch) + } else { + break + } + } + + if numStr == "" { + return 1 + } + + n := 0 + for _, ch := range numStr { + n = n*10 + int(ch-'0') + } + return n +} diff --git a/vcs/util_test.go b/vcs/util_test.go new file mode 100644 index 0000000..acdac7f --- /dev/null +++ b/vcs/util_test.go @@ -0,0 +1,250 @@ +package vcs_test + +import ( + "context" + "fmt" + "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]) + } + // 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]) + } + }) +} -- 2.47.3 From 1749d95727f84d0228ca72ab9b8decf349f4b795 Mon Sep 17 00:00:00 2001 From: claw Date: Tue, 12 May 2026 12:56:13 -0700 Subject: [PATCH 2/3] fix(vcs): address review findings on PR #88 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Findings addressed: - F1/G1: Add doc comment to GetAllFilesInPath documenting fail-fast contract - F2/G2: Add explicit backslash-prefix guard to skip '\ No newline' markers - F3: Add comment explaining position > 0 guard (skip lines before first hunk) - F4: Refactor parseHunkNewStart to use strconv.Atoi instead of per-char concat - F5: Add error propagation tests (ListContents, GetFileContent, nested, ctx cancel) - F6: Wrap errors.ErrUnsupported in DismissReview for programmatic checking - S1: Add ctx.Err() checks + max file count/byte constants with clear errors - S2: Addressed by S1 — input bounds are now enforced via the same constants --- gitea/client.go | 2 +- vcs/util.go | 110 ++++++++++++++++++++++++++++++++++------------- vcs/util_test.go | 81 ++++++++++++++++++++++++++++++++++ 3 files changed, 161 insertions(+), 32 deletions(-) diff --git a/gitea/client.go b/gitea/client.go index 74fce56..ab963a6 100644 --- a/gitea/client.go +++ b/gitea/client.go @@ -835,7 +835,7 @@ func (c *Client) ResolveComment(ctx context.Context, owner, repo string, comment // 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("DismissReview: not implemented") + return fmt.Errorf("DismissReview: %w", errors.ErrUnsupported) } // GetFileContentAtRef fetches a file at a specific ref from a repo. diff --git a/vcs/util.go b/vcs/util.go index 9f133fd..f1266c8 100644 --- a/vcs/util.go +++ b/vcs/util.go @@ -3,39 +3,82 @@ package vcs import ( "context" "fmt" + "strconv" "strings" ) +const ( + // maxFilesInPath is the maximum number of files GetAllFilesInPath will fetch. + // Prevents unbounded resource consumption on very large directory trees. + 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. +// +// This function uses fail-fast error handling: any error from ListContents or +// 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 +// 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) + totalBytes := 0 - entries, err := client.ListContents(ctx, owner, repo, path) - if err != nil { - return nil, fmt.Errorf("list contents %q: %w", path, err) - } + var walk func(string) error + walk = func(dir string) error { + if err := ctx.Err(); err != nil { + return fmt.Errorf("context cancelled during traversal: %w", err) + } - for _, entry := range entries { - switch entry.Type { - case "file": - content, err := client.GetFileContent(ctx, owner, repo, entry.Path, "") - if err != nil { - return nil, fmt.Errorf("get file %q: %w", entry.Path, err) + entries, err := client.ListContents(ctx, owner, repo, dir) + if err != nil { + return fmt.Errorf("list contents %q: %w", dir, err) + } + + for _, entry := range entries { + if err := ctx.Err(); err != nil { + return fmt.Errorf("context cancelled during traversal: %w", err) } - results[entry.Path] = content - case "dir": - subResults, err := GetAllFilesInPath(ctx, client, owner, repo, entry.Path) - if err != nil { - return nil, fmt.Errorf("recurse into %q: %w", entry.Path, err) - } - for k, v := range subResults { - results[k] = v + + switch entry.Type { + case "file": + if len(results) >= maxFilesInPath { + return fmt.Errorf("exceeded max file count (%d) in path %q", maxFilesInPath, path) + } + + 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) + } + + results[entry.Path] = content + case "dir": + if err := walk(entry.Path); err != nil { + return err + } } } + return nil } + if err := walk(path); err != nil { + return nil, err + } return results, nil } @@ -92,6 +135,12 @@ func BuildLineToPositionMap(diff string) map[string]map[int]int { 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++ @@ -101,7 +150,10 @@ func BuildLineToPositionMap(diff string) map[string]map[int]int { position++ // Deletion lines don't map to new line numbers } else if strings.HasPrefix(line, " ") { - // Context line (space-prefixed) + // 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 @@ -123,23 +175,19 @@ func parseHunkNewStart(hunkLine string) int { } rest := hunkLine[plusIdx+1:] - // Read digits until comma or space - var numStr string - for _, ch := range rest { - if ch >= '0' && ch <= '9' { - numStr += string(ch) - } else { - break - } + // Find the end of the number (first non-digit after +) + endIdx := 0 + for endIdx < len(rest) && rest[endIdx] >= '0' && rest[endIdx] <= '9' { + endIdx++ } - if numStr == "" { + if endIdx == 0 { return 1 } - n := 0 - for _, ch := range numStr { - n = n*10 + int(ch-'0') + n, err := strconv.Atoi(rest[:endIdx]) + if err != nil { + return 1 } return n } diff --git a/vcs/util_test.go b/vcs/util_test.go index acdac7f..8436884 100644 --- a/vcs/util_test.go +++ b/vcs/util_test.go @@ -2,6 +2,7 @@ package vcs_test import ( "context" + "strings" "fmt" "testing" @@ -248,3 +249,83 @@ func TestBuildLineToPositionMap(t *testing.T) { } }) } + +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("cancelled 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 cancelled context, got nil") + } + if !strings.Contains(err.Error(), "context cancelled") { + t.Errorf("expected context cancellation error, got: %v", err) + } + }) +} -- 2.47.3 From ec03dc2373b049889e3d09f29ac104fbaa6bc2b4 Mon Sep 17 00:00:00 2001 From: claw Date: Tue, 12 May 2026 13:07:41 -0700 Subject: [PATCH 3/3] fix: address remaining review findings (interface assertions, DismissReview ctx, import order, filepath param, spelling) --- cmd/review-bot/main.go | 6 +++--- gitea/client.go | 2 +- gitea/conformance_test.go | 25 +++++++++++++++++++++++++ vcs/util.go | 4 ++-- vcs/util_test.go | 8 ++++---- 5 files changed, 35 insertions(+), 10 deletions(-) create mode 100644 gitea/conformance_test.go diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index 44f1306..40efc46 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -838,9 +838,9 @@ func (a *giteaClientAdapter) ListContents(ctx context.Context, owner, repo, path return result, nil } -func (a *giteaClientAdapter) GetFileContent(ctx context.Context, owner, repo, filepath, ref string) (string, error) { +func (a *giteaClientAdapter) GetFileContent(ctx context.Context, owner, repo, filePath, ref string) (string, error) { if ref != "" { - return a.client.GetFileContentRef(ctx, owner, repo, filepath, ref) + return a.client.GetFileContentRef(ctx, owner, repo, filePath, ref) } - return a.client.GetFileContent(ctx, owner, repo, filepath) + return a.client.GetFileContent(ctx, owner, repo, filePath) } diff --git a/gitea/client.go b/gitea/client.go index ab963a6..3284314 100644 --- a/gitea/client.go +++ b/gitea/client.go @@ -835,7 +835,7 @@ func (c *Client) ResolveComment(ctx context.Context, owner, repo string, comment // 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("DismissReview: %w", errors.ErrUnsupported) + return fmt.Errorf("dismiss review %d on %s/%s#%d: %w", reviewID, owner, repo, number, errors.ErrUnsupported) } // GetFileContentAtRef fetches a file at a specific ref from a repo. diff --git a/gitea/conformance_test.go b/gitea/conformance_test.go new file mode 100644 index 0000000..08b7f97 --- /dev/null +++ b/gitea/conformance_test.go @@ -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) + _ vcs.FileReader = (*gitea.Client)(nil) + _ vcs.Reviewer = (*gitea.Client)(nil) + _ vcs.Identity = (*gitea.Client)(nil) +) diff --git a/vcs/util.go b/vcs/util.go index f1266c8..62f2015 100644 --- a/vcs/util.go +++ b/vcs/util.go @@ -37,7 +37,7 @@ func GetAllFilesInPath(ctx context.Context, client FileReader, owner, repo, path var walk func(string) error walk = func(dir string) error { if err := ctx.Err(); err != nil { - return fmt.Errorf("context cancelled during traversal: %w", err) + return fmt.Errorf("context canceled during traversal: %w", err) } entries, err := client.ListContents(ctx, owner, repo, dir) @@ -47,7 +47,7 @@ func GetAllFilesInPath(ctx context.Context, client FileReader, owner, repo, path for _, entry := range entries { if err := ctx.Err(); err != nil { - return fmt.Errorf("context cancelled during traversal: %w", err) + return fmt.Errorf("context canceled during traversal: %w", err) } switch entry.Type { diff --git a/vcs/util_test.go b/vcs/util_test.go index 8436884..6d1e42a 100644 --- a/vcs/util_test.go +++ b/vcs/util_test.go @@ -2,8 +2,8 @@ package vcs_test import ( "context" - "strings" "fmt" + "strings" "testing" "gitea.weiker.me/rodin/review-bot/vcs" @@ -306,7 +306,7 @@ func TestGetAllFilesInPath_ErrorPropagation(t *testing.T) { } }) - t.Run("cancelled context propagates", func(t *testing.T) { + t.Run("canceled context propagates", func(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) cancel() // Cancel immediately @@ -322,9 +322,9 @@ func TestGetAllFilesInPath_ErrorPropagation(t *testing.T) { } _, err := vcs.GetAllFilesInPath(ctx, client, "owner", "repo", "src") if err == nil { - t.Fatal("expected error from cancelled context, got nil") + t.Fatal("expected error from canceled context, got nil") } - if !strings.Contains(err.Error(), "context cancelled") { + if !strings.Contains(err.Error(), "context canceled") { t.Errorf("expected context cancellation error, got: %v", err) } }) -- 2.47.3