From 98a4772f3026d3b0a074d182d918810083e2d7f8 Mon Sep 17 00:00:00 2001 From: Rodin Date: Fri, 1 May 2026 14:33:18 -0700 Subject: [PATCH 1/4] fix: path-escape file paths and eliminate url package shadowing - Add escapePath() helper: escapes each path segment individually (preserves slashes as separators, escapes spaces/#/? etc) - Apply to GetFileContent, GetFileContentRef, ListContents - Rename doGet parameter from url to reqURL (avoids shadowing net/url) - Rename local variables in GetFileContent/ListContents for consistency Addresses remaining findings from PR #16 review. --- gitea/client.go | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/gitea/client.go b/gitea/client.go index cbd266d..f73a55c 100644 --- a/gitea/client.go +++ b/gitea/client.go @@ -108,8 +108,8 @@ func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string) // GetFileContent fetches a file from the default branch of a repo. func (c *Client) GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) { - url := fmt.Sprintf("%s/api/v1/repos/%s/%s/raw/%s", c.baseURL, owner, repo, filepath) - body, err := c.doGet(ctx, url) + reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/raw/%s", c.baseURL, owner, repo, escapePath(filepath)) + body, err := c.doGet(ctx, reqURL) if err != nil { return "", fmt.Errorf("fetch file %s: %w", filepath, err) } @@ -118,7 +118,7 @@ func (c *Client) GetFileContent(ctx context.Context, owner, repo, filepath strin // GetFileContentRef fetches a file from a specific ref (branch/tag/sha) in a repo. func (c *Client) GetFileContentRef(ctx context.Context, owner, repo, filepath, ref string) (string, error) { - reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/raw/%s?ref=%s", c.baseURL, owner, repo, filepath, url.QueryEscape(ref)) + reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/raw/%s?ref=%s", c.baseURL, owner, repo, escapePath(filepath), url.QueryEscape(ref)) body, err := c.doGet(ctx, reqURL) if err != nil { return "", fmt.Errorf("fetch file %s@%s: %w", filepath, ref, err) @@ -164,8 +164,8 @@ func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, return nil } -func (c *Client) doGet(ctx context.Context, url string) ([]byte, error) { - req, err := http.NewRequestWithContext(ctx, "GET", url, nil) +func (c *Client) doGet(ctx context.Context, reqURL string) ([]byte, error) { + req, err := http.NewRequestWithContext(ctx, "GET", reqURL, nil) if err != nil { return nil, err } @@ -184,6 +184,16 @@ func (c *Client) doGet(ctx context.Context, url string) ([]byte, error) { return io.ReadAll(resp.Body) } +// escapePath escapes each segment of a file path for use in URLs. +// Slashes are preserved as path separators; other special characters are escaped. +func escapePath(p string) string { + parts := strings.Split(p, "/") + for i, part := range parts { + parts[i] = url.PathEscape(part) + } + return strings.Join(parts, "/") +} + // ContentEntry represents a file or directory entry from the contents API. type ContentEntry struct { Name string `json:"name"` @@ -193,8 +203,8 @@ type ContentEntry struct { // ListContents lists files and directories at a given path in a repo. func (c *Client) ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error) { - url := fmt.Sprintf("%s/api/v1/repos/%s/%s/contents/%s", c.baseURL, owner, repo, path) - body, err := c.doGet(ctx, url) + reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/contents/%s", c.baseURL, owner, repo, escapePath(path)) + body, err := c.doGet(ctx, reqURL) if err != nil { return nil, fmt.Errorf("list contents %s: %w", path, err) } From dd2661fe1406f8458ad9b0be1f2193d0fc96e5fb Mon Sep 17 00:00:00 2001 From: Rodin Date: Fri, 1 May 2026 14:40:19 -0700 Subject: [PATCH 2/4] fix: address all review findings from PR #17 - Rename all remaining url locals to reqURL (consistency) - Use http.MethodGet/http.MethodPost constants - Document escapePath: relative paths only, double-encoding expected - Add TestEscapePath with 7 edge cases (empty, spaces, #, deep, encoded) --- gitea/client.go | 26 ++++++++++++++------------ gitea/client_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 12 deletions(-) diff --git a/gitea/client.go b/gitea/client.go index f73a55c..c2060fc 100644 --- a/gitea/client.go +++ b/gitea/client.go @@ -56,8 +56,8 @@ type ChangedFile struct { // GetPullRequest fetches PR metadata. func (c *Client) GetPullRequest(ctx context.Context, owner, repo string, number int) (*PullRequest, error) { - url := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d", c.baseURL, owner, repo, number) - body, err := c.doGet(ctx, url) + reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d", c.baseURL, owner, repo, number) + body, err := c.doGet(ctx, reqURL) if err != nil { return nil, fmt.Errorf("fetch PR: %w", err) } @@ -70,8 +70,8 @@ func (c *Client) GetPullRequest(ctx context.Context, owner, repo string, number // GetPullRequestDiff fetches the unified diff for a PR. func (c *Client) GetPullRequestDiff(ctx context.Context, owner, repo string, number int) (string, error) { - url := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d.diff", c.baseURL, owner, repo, number) - body, err := c.doGet(ctx, url) + reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d.diff", c.baseURL, owner, repo, number) + body, err := c.doGet(ctx, reqURL) if err != nil { return "", fmt.Errorf("fetch diff: %w", err) } @@ -80,8 +80,8 @@ func (c *Client) GetPullRequestDiff(ctx context.Context, owner, repo string, num // GetPullRequestFiles fetches the list of files changed in a PR. func (c *Client) GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]ChangedFile, error) { - url := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/files", c.baseURL, owner, repo, number) - body, err := c.doGet(ctx, url) + reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/files", c.baseURL, owner, repo, number) + body, err := c.doGet(ctx, reqURL) if err != nil { return nil, fmt.Errorf("fetch PR files: %w", err) } @@ -94,8 +94,8 @@ func (c *Client) GetPullRequestFiles(ctx context.Context, owner, repo string, nu // GetCommitStatuses fetches CI statuses for a commit SHA. func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string) ([]CommitStatus, error) { - url := fmt.Sprintf("%s/api/v1/repos/%s/%s/commits/%s/statuses", c.baseURL, owner, repo, sha) - body, err := c.doGet(ctx, url) + reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/commits/%s/statuses", c.baseURL, owner, repo, sha) + body, err := c.doGet(ctx, reqURL) if err != nil { return nil, fmt.Errorf("fetch commit statuses: %w", err) } @@ -129,7 +129,7 @@ func (c *Client) GetFileContentRef(ctx context.Context, owner, repo, filepath, r // PostReview submits a review to a PR. // event should be "APPROVED" or "REQUEST_CHANGES". func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, event, body string) error { - url := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews", c.baseURL, owner, repo, number) + reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews", c.baseURL, owner, repo, number) payload := struct { Body string `json:"body"` @@ -144,7 +144,7 @@ func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, return fmt.Errorf("marshal review payload: %w", err) } - req, err := http.NewRequestWithContext(ctx, "POST", url, bytes.NewReader(data)) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, reqURL, bytes.NewReader(data)) if err != nil { return fmt.Errorf("create review request: %w", err) } @@ -165,7 +165,7 @@ func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, } func (c *Client) doGet(ctx context.Context, reqURL string) ([]byte, error) { - req, err := http.NewRequestWithContext(ctx, "GET", reqURL, nil) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, reqURL, nil) if err != nil { return nil, err } @@ -184,8 +184,10 @@ func (c *Client) doGet(ctx context.Context, reqURL string) ([]byte, error) { return io.ReadAll(resp.Body) } -// escapePath escapes each segment of a file path for use in URLs. +// escapePath escapes each segment of a relative file path for use in URLs. // Slashes are preserved as path separators; other special characters are escaped. +// Input should be a relative path (no leading slash). Already-encoded segments +// will be double-encoded, which is the desired behavior for user-provided paths. func escapePath(p string) string { parts := strings.Split(p, "/") for i, part := range parts { diff --git a/gitea/client_test.go b/gitea/client_test.go index 0ec067c..24f60f8 100644 --- a/gitea/client_test.go +++ b/gitea/client_test.go @@ -294,3 +294,27 @@ func TestGetAllFilesInPath_File(t *testing.T) { t.Errorf("unexpected content: %q", files["README.md"]) } } + +func TestEscapePath(t *testing.T) { + tests := []struct { + name string + input string + want string + }{ + {"simple", "src/main.go", "src/main.go"}, + {"spaces", "my dir/my file.go", "my%20dir/my%20file.go"}, + {"special chars", "path/file#1.txt", "path/file%231.txt"}, + {"empty", "", ""}, + {"single segment", "README.md", "README.md"}, + {"nested deep", "a/b/c/d.md", "a/b/c/d.md"}, + {"already encoded", "path/file%20name.go", "path/file%2520name.go"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := escapePath(tt.input) + if got != tt.want { + t.Errorf("escapePath(%q) = %q, want %q", tt.input, got, tt.want) + } + }) + } +} From 7b42de67ca062bb0392e3dc7c1d8fe0a0620af94 Mon Sep 17 00:00:00 2001 From: Rodin Date: Fri, 1 May 2026 14:46:40 -0700 Subject: [PATCH 3/4] fix: handle empty path in ListContents (root listing) Empty path now yields /contents instead of /contents/ (trailing slash). Added doc comment noting empty path = repo root. --- gitea/client.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/gitea/client.go b/gitea/client.go index c2060fc..9653a18 100644 --- a/gitea/client.go +++ b/gitea/client.go @@ -204,8 +204,14 @@ type ContentEntry struct { } // ListContents lists files and directories at a given path in a repo. +// Pass an empty path to list the repository root. func (c *Client) ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error) { - reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/contents/%s", c.baseURL, owner, repo, escapePath(path)) + var reqURL string + if path == "" { + reqURL = fmt.Sprintf("%s/api/v1/repos/%s/%s/contents", c.baseURL, owner, repo) + } else { + reqURL = fmt.Sprintf("%s/api/v1/repos/%s/%s/contents/%s", c.baseURL, owner, repo, escapePath(path)) + } body, err := c.doGet(ctx, reqURL) if err != nil { return nil, fmt.Errorf("list contents %s: %w", path, err) From aade89112906a9d74186cb767decfb8bf5351424 Mon Sep 17 00:00:00 2001 From: Rodin Date: Fri, 1 May 2026 14:54:58 -0700 Subject: [PATCH 4/4] docs: add package-level documentation Per go-patterns/package-design.md, every package needs a doc comment. Added to gitea, llm, and review packages. --- gitea/client.go | 3 +++ llm/client.go | 1 + review/prompt.go | 2 ++ 3 files changed, 6 insertions(+) diff --git a/gitea/client.go b/gitea/client.go index 9653a18..73a4abf 100644 --- a/gitea/client.go +++ b/gitea/client.go @@ -1,3 +1,6 @@ +// Package gitea provides a client for the Gitea API. +// It supports pull request operations, file content retrieval, +// and review submission. package gitea import ( diff --git a/llm/client.go b/llm/client.go index c66ae89..41c1154 100644 --- a/llm/client.go +++ b/llm/client.go @@ -1,3 +1,4 @@ +// Package llm provides a client for OpenAI-compatible chat completion APIs. package llm import ( diff --git a/review/prompt.go b/review/prompt.go index 1011906..a4072a9 100644 --- a/review/prompt.go +++ b/review/prompt.go @@ -1,3 +1,5 @@ +// Package review builds prompts for AI code review and parses LLM responses +// into structured review results. package review import (