From 921599542db0d05d9bb4180ddbca80b4a581dcf5 Mon Sep 17 00:00:00 2001 From: Aaron Weiker Date: Wed, 13 May 2026 04:13:00 +0000 Subject: [PATCH 1/2] feat(github): implement FileReader interface (#80) Implement FileReader conformance on the GitHub client: GetFileContent, ListContents, path helpers, base64 decode. Includes compile-time conformance checks for both PRReader and FileReader. Requires PR B (#102). Part 3 of 3 for #80. --- github/conformance_test.go | 9 +- github/files.go | 60 +++++++ github/files_test.go | 309 ++++++++++++++++++++++++++++++++++++- 3 files changed, 373 insertions(+), 5 deletions(-) diff --git a/github/conformance_test.go b/github/conformance_test.go index 666bcab..ca13188 100644 --- a/github/conformance_test.go +++ b/github/conformance_test.go @@ -5,6 +5,9 @@ import ( "gitea.weiker.me/rodin/review-bot/vcs" ) -// Compile-time interface conformance assertion. -// Verifies github.Client satisfies vcs.PRReader. -var _ vcs.PRReader = (*github.Client)(nil) +// Compile-time interface conformance assertions. +// These verify github.Client satisfies vcs.PRReader and vcs.FileReader. +var ( + _ vcs.PRReader = (*github.Client)(nil) + _ vcs.FileReader = (*github.Client)(nil) +) diff --git a/github/files.go b/github/files.go index 6d968a3..11e2aa2 100644 --- a/github/files.go +++ b/github/files.go @@ -8,8 +8,16 @@ import ( "net/url" "path" "strings" + + "gitea.weiker.me/rodin/review-bot/vcs" ) +// GetFileContent fetches a file from a repo at the given ref. +// Delegates to GetFileContentAtRef with the provided ref. +func (c *Client) GetFileContent(ctx context.Context, owner, repo, filePath, ref string) (string, error) { + return c.GetFileContentAtRef(ctx, owner, repo, filePath, ref) +} + // GetFileContentAtRef fetches a file at a specific ref from a repo. // If ref is empty, the query parameter is omitted (uses default branch). // @@ -46,6 +54,58 @@ func (c *Client) GetFileContentAtRef(ctx context.Context, owner, repo, filePath, return decoded, nil } +// ListContents lists files and directories at a given path in a repo. +// Returns the directory listing from the GitHub contents API. +// If the path points to a single file (not a directory), the API returns +// a JSON object instead of an array; this is handled by returning a +// single-element slice. +func (c *Client) ListContents(ctx context.Context, owner, repo, filePath string) ([]vcs.ContentEntry, error) { + escaped, err := escapePath(filePath) + if err != nil { + return nil, fmt.Errorf("invalid file path: %w", err) + } + reqURL := fmt.Sprintf("%s/repos/%s/%s/contents/%s", + c.baseURL, url.PathEscape(owner), url.PathEscape(repo), escaped) + body, err := c.doGet(ctx, reqURL) + if err != nil { + return nil, fmt.Errorf("list contents %s: %w", filePath, err) + } + + type entry struct { + Name string `json:"name"` + Path string `json:"path"` + Type string `json:"type"` + } + + // The GitHub contents API returns an array for directories and an object + // for single files. Try array first (common case), then fall back to object. + // An empty array ([]) is valid — it represents an empty directory — and + // results in a zero-length slice returned without error. + var entries []entry + if err := json.Unmarshal(body, &entries); err != nil { + var single entry + if err2 := json.Unmarshal(body, &single); err2 != nil { + return nil, fmt.Errorf("parse contents JSON: as array: %v; as object: %w", err, err2) + } + // Guard against empty objects ({}) or unexpected shapes that + // unmarshal successfully but carry no useful data. + if single.Name == "" && single.Path == "" && single.Type == "" { + return nil, fmt.Errorf("parse contents JSON: unexpected response format") + } + entries = []entry{single} + } + + result := make([]vcs.ContentEntry, len(entries)) + for i, e := range entries { + result[i] = vcs.ContentEntry{ + Name: e.Name, + Path: e.Path, + Type: e.Type, + } + } + return result, nil +} + // escapePath validates and encodes a slash-separated file path for use in // GitHub API URLs. Returns an error if the path contains dot-segments ("." // or "..") or resolves to a path outside the repository root. diff --git a/github/files_test.go b/github/files_test.go index 8385a07..51a970c 100644 --- a/github/files_test.go +++ b/github/files_test.go @@ -2,12 +2,287 @@ package github import ( "context" + "encoding/json" "net/http" "net/http/httptest" "strings" "testing" + "time" ) +func TestGetFileContent_DelegatesToGetFileContentAtRef(t *testing.T) { + var gotRef string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotRef = r.URL.Query().Get("ref") + json.NewEncoder(w).Encode(map[string]string{ + "content": "dGVzdA==", // "test" in base64 + "encoding": "base64", + }) + })) + defer srv.Close() + + c := NewClient("token", srv.URL, AllowInsecureHTTP()) + c.SetHTTPClient(srv.Client()) + + // Call with empty ref — should not include ref param + content, err := c.GetFileContent(context.Background(), "owner", "repo", "file.go", "") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if content != "test" { + t.Errorf("expected 'test', got %q", content) + } + if gotRef != "" { + t.Errorf("expected empty ref, got %q", gotRef) + } +} + +func TestGetFileContent_WithRef(t *testing.T) { + var gotRef string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotRef = r.URL.Query().Get("ref") + json.NewEncoder(w).Encode(map[string]string{ + "content": "dGVzdA==", + "encoding": "base64", + }) + })) + defer srv.Close() + + c := NewClient("token", srv.URL, AllowInsecureHTTP()) + c.SetHTTPClient(srv.Client()) + + _, err := c.GetFileContent(context.Background(), "owner", "repo", "file.go", "abc123") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if gotRef != "abc123" { + t.Errorf("expected ref 'abc123', got %q", gotRef) + } +} + +func TestGetFileContent_404(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(404) + w.Write([]byte(`{"message":"Not Found"}`)) + })) + defer srv.Close() + + c := NewClient("token", srv.URL, AllowInsecureHTTP()) + c.SetHTTPClient(srv.Client()) + + _, err := c.GetFileContent(context.Background(), "owner", "repo", "missing.go", "") + if err == nil { + t.Fatal("expected error for 404") + } +} + +func TestGetFileContent_401(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(401) + w.Write([]byte(`{"message":"Bad credentials"}`)) + })) + defer srv.Close() + + c := NewClient("token", srv.URL, AllowInsecureHTTP()) + c.SetHTTPClient(srv.Client()) + + _, err := c.GetFileContent(context.Background(), "owner", "repo", "file.go", "") + if err == nil { + t.Fatal("expected error for 401") + } +} + +func TestGetFileContent_429Retry(t *testing.T) { + attempts := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + attempts++ + if attempts == 1 { + w.WriteHeader(429) + w.Write([]byte(`{"message":"rate limit"}`)) + return + } + json.NewEncoder(w).Encode(map[string]string{ + "content": "b2s=", + "encoding": "base64", + }) + })) + defer srv.Close() + + c := NewClient("token", srv.URL, AllowInsecureHTTP()) + c.SetHTTPClient(srv.Client()) + c.SetRetryBackoff([]time.Duration{1 * time.Millisecond}) + + content, err := c.GetFileContent(context.Background(), "owner", "repo", "file.go", "") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if content != "ok" { + t.Errorf("expected 'ok', got %q", content) + } + if attempts != 2 { + t.Errorf("expected 2 attempts, got %d", attempts) + } +} + +func TestGetFileContent_MalformedJSON(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(200) + w.Write([]byte(`not json`)) + })) + defer srv.Close() + + c := NewClient("token", srv.URL, AllowInsecureHTTP()) + c.SetHTTPClient(srv.Client()) + + _, err := c.GetFileContent(context.Background(), "owner", "repo", "file.go", "") + if err == nil { + t.Fatal("expected error for malformed JSON") + } +} + +func TestListContents_HappyPath(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/repos/owner/repo/contents/src" { + t.Errorf("unexpected path: %s", r.URL.Path) + } + json.NewEncoder(w).Encode([]map[string]string{ + {"name": "main.go", "path": "src/main.go", "type": "file"}, + {"name": "lib", "path": "src/lib", "type": "dir"}, + }) + })) + defer srv.Close() + + c := NewClient("token", srv.URL, AllowInsecureHTTP()) + c.SetHTTPClient(srv.Client()) + + entries, err := c.ListContents(context.Background(), "owner", "repo", "src") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(entries) != 2 { + t.Fatalf("expected 2 entries, got %d", len(entries)) + } + if entries[0].Name != "main.go" { + t.Errorf("expected name 'main.go', got %q", entries[0].Name) + } + if entries[0].Path != "src/main.go" { + t.Errorf("expected path 'src/main.go', got %q", entries[0].Path) + } + if entries[0].Type != "file" { + t.Errorf("expected type 'file', got %q", entries[0].Type) + } + if entries[1].Name != "lib" { + t.Errorf("expected name 'lib', got %q", entries[1].Name) + } + if entries[1].Type != "dir" { + t.Errorf("expected type 'dir', got %q", entries[1].Type) + } +} + +func TestListContents_404(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(404) + w.Write([]byte(`{"message":"Not Found"}`)) + })) + defer srv.Close() + + c := NewClient("token", srv.URL, AllowInsecureHTTP()) + c.SetHTTPClient(srv.Client()) + + _, err := c.ListContents(context.Background(), "owner", "repo", "missing") + if err == nil { + t.Fatal("expected error for 404") + } +} + +func TestListContents_401(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(401) + w.Write([]byte(`{"message":"Bad credentials"}`)) + })) + defer srv.Close() + + c := NewClient("token", srv.URL, AllowInsecureHTTP()) + c.SetHTTPClient(srv.Client()) + + _, err := c.ListContents(context.Background(), "owner", "repo", "src") + if err == nil { + t.Fatal("expected error for 401") + } +} + +func TestListContents_429Retry(t *testing.T) { + attempts := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + attempts++ + if attempts == 1 { + w.WriteHeader(429) + w.Write([]byte(`{"message":"rate limit"}`)) + return + } + json.NewEncoder(w).Encode([]map[string]string{ + {"name": "file.go", "path": "file.go", "type": "file"}, + }) + })) + defer srv.Close() + + c := NewClient("token", srv.URL, AllowInsecureHTTP()) + c.SetHTTPClient(srv.Client()) + c.SetRetryBackoff([]time.Duration{1 * time.Millisecond}) + + entries, err := c.ListContents(context.Background(), "owner", "repo", "src") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(entries) != 1 { + t.Fatalf("expected 1 entry, got %d", len(entries)) + } + if attempts != 2 { + t.Errorf("expected 2 attempts, got %d", attempts) + } +} + +func TestListContents_MalformedJSON(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(200) + w.Write([]byte(`not json`)) + })) + defer srv.Close() + + c := NewClient("token", srv.URL, AllowInsecureHTTP()) + c.SetHTTPClient(srv.Client()) + + _, err := c.ListContents(context.Background(), "owner", "repo", "src") + if err == nil { + t.Fatal("expected error for malformed JSON") + } +} + +func TestListContents_SingleFile(t *testing.T) { + // GitHub Contents API returns a JSON object (not array) for single-file paths + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(200) + w.Write([]byte(`{"name":"README.md","path":"README.md","type":"file"}`)) + })) + defer srv.Close() + + c := NewClient("token", srv.URL, AllowInsecureHTTP()) + c.SetHTTPClient(srv.Client()) + entries, err := c.ListContents(context.Background(), "owner", "repo", "README.md") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(entries) != 1 { + t.Fatalf("expected 1 entry, got %d", len(entries)) + } + if entries[0].Name != "README.md" { + t.Errorf("expected name 'README.md', got %q", entries[0].Name) + } + if entries[0].Type != "file" { + t.Errorf("expected type 'file', got %q", entries[0].Type) + } +} + func TestEscapePath_ValidPaths(t *testing.T) { t.Parallel() tests := []struct { @@ -68,7 +343,7 @@ func TestGetFileContentAtRef_DotSegmentError(t *testing.T) { })) defer srv.Close() - c := NewClient("token", srv.URL) + c := NewClient("token", srv.URL, AllowInsecureHTTP()) _, err := c.GetFileContentAtRef(context.Background(), "owner", "repo", "foo/../bar.go", "main") if err == nil { t.Fatal("expected error for path with dot-segments") @@ -78,6 +353,37 @@ func TestGetFileContentAtRef_DotSegmentError(t *testing.T) { } } +func TestDecodeBase64Content(t *testing.T) { + // Test with newlines (GitHub's format) + encoded := "cGFja2FnZSBt\nYWlu" + decoded, err := decodeBase64Content(encoded) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if decoded != "package main" { + t.Errorf("expected 'package main', got %q", decoded) + } +} + +func TestDecodeBase64Content_Invalid(t *testing.T) { + _, err := decodeBase64Content("not!!!valid!!!base64") + if err == nil { + t.Fatal("expected error for invalid base64") + } +} + +func TestDecodeBase64Content_CRLF(t *testing.T) { + // Base64 of "hello world" with CRLF line breaks inserted + encoded := "aGVs\r\nbG8g\r\nd29y\r\nbGQ=" + decoded, err := decodeBase64Content(encoded) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if decoded != "hello world" { + t.Errorf("expected 'hello world', got %q", decoded) + } +} + func TestDecodeBase64Content_SizeLimit(t *testing.T) { t.Parallel() // Create base64 content that would decode to > maxFileContentSize. @@ -93,4 +399,3 @@ func TestDecodeBase64Content_SizeLimit(t *testing.T) { t.Errorf("expected 'too large' error, got: %v", err) } } - From dca260f5820350cbb3477b5789dbeacc2fd3c840 Mon Sep 17 00:00:00 2001 From: claw Date: Tue, 12 May 2026 22:47:31 -0700 Subject: [PATCH 2/2] fix(test): SetRetryBackoff with correct slice length Pass 2 elements to SetRetryBackoff (matching maxRetryAttempts-1 = 2) and check the error return. Previously passing 1 element silently failed, causing tests to fall back to default {1s, 2s} backoffs. Fixes self-review finding: 429Retry tests now run in <10ms instead of ~1s. --- github/files_test.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/github/files_test.go b/github/files_test.go index 51a970c..d45670d 100644 --- a/github/files_test.go +++ b/github/files_test.go @@ -110,7 +110,9 @@ func TestGetFileContent_429Retry(t *testing.T) { c := NewClient("token", srv.URL, AllowInsecureHTTP()) c.SetHTTPClient(srv.Client()) - c.SetRetryBackoff([]time.Duration{1 * time.Millisecond}) + if err := c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}); err != nil { + t.Fatalf("SetRetryBackoff: %v", err) + } content, err := c.GetFileContent(context.Background(), "owner", "repo", "file.go", "") if err != nil { @@ -228,7 +230,9 @@ func TestListContents_429Retry(t *testing.T) { c := NewClient("token", srv.URL, AllowInsecureHTTP()) c.SetHTTPClient(srv.Client()) - c.SetRetryBackoff([]time.Duration{1 * time.Millisecond}) + if err := c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}); err != nil { + t.Fatalf("SetRetryBackoff: %v", err) + } entries, err := c.ListContents(context.Background(), "owner", "repo", "src") if err != nil {