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

fix(gitea): normalize "." path to empty string in ListContents #72

Merged
aweiker merged 2 commits from issue-70 into main 2026-05-11 14:16:22 +00:00
Conversation 14 Commits 2 Files Changed 3
+32 -2
3 changed files with 32 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.gitea/workflows/ci.yml
+4 -2
View File
@@ -38,6 +38,8 @@ jobs:
- name: security - name: security
token_secret: SECURITY_REVIEW_TOKEN token_secret: SECURITY_REVIEW_TOKEN
model: gpt-5 model: gpt-5
patterns_repo: rodin/security-patterns
patterns_files: "."
system_prompt_file: SECURITY_REVIEW.md system_prompt_file: SECURITY_REVIEW.md
steps: steps:
- uses: actions/checkout@v4 - uses: actions/checkout@v4
@@ -60,8 +62,8 @@ jobs:
AICORE_API_URL: ${{ secrets.AICORE_API_URL }} AICORE_API_URL: ${{ secrets.AICORE_API_URL }}
AICORE_RESOURCE_GROUP: ${{ secrets.AICORE_RESOURCE_GROUP }} AICORE_RESOURCE_GROUP: ${{ secrets.AICORE_RESOURCE_GROUP }}
CONVENTIONS_FILE: "CONVENTIONS.md" CONVENTIONS_FILE: "CONVENTIONS.md"
PATTERNS_REPO: "rodin/go-patterns" PATTERNS_REPO: ${{ matrix.patterns_repo || 'rodin/go-patterns' }}
security-review-bot marked this conversation as resolved
security-review-bot commented 2026-05-11 14:12:23 +00:00
Review
Copy Link
Copy Source

[MINOR] A new pull_request job ('test-dot-path') runs code from the PR with repository secrets (e.g., GPT_REVIEW_TOKEN, AICORE credentials) in the environment. If PRs from forks can trigger this workflow with secrets, a malicious change could exfiltrate them. Ensure secrets are not exposed to untrusted forks or gate this job to trusted actors only.

**[MINOR]** A new pull_request job ('test-dot-path') runs code from the PR with repository secrets (e.g., GPT_REVIEW_TOKEN, AICORE credentials) in the environment. If PRs from forks can trigger this workflow with secrets, a malicious change could exfiltrate them. Ensure secrets are not exposed to untrusted forks or gate this job to trusted actors only.
PATTERNS_FILES: "README.md,patterns/" PATTERNS_FILES: ${{ matrix.patterns_files || 'README.md,patterns/' }}
LLM_TIMEOUT: "600" LLM_TIMEOUT: "600"
SYSTEM_PROMPT_FILE: ${{ matrix.system_prompt_file }} SYSTEM_PROMPT_FILE: ${{ matrix.system_prompt_file }}
run: ./review-bot run: ./review-bot
gpt-review-bot commented 2026-05-11 14:12:14 +00:00
Review
Copy Link
Copy Source

[NIT] The new test-dot-path job introduces an external integration check that depends on secrets and a live repo. Since a unit test already covers the behavior, consider whether this job is necessary to avoid CI flakiness or secret requirements.

**[NIT]** The new test-dot-path job introduces an external integration check that depends on secrets and a live repo. Since a unit test already covers the behavior, consider whether this job is necessary to avoid CI flakiness or secret requirements.
1
gitea/client.go
+4
View File
@@ -435,6 +435,10 @@ type ContentEntry struct {
// ListContents lists files and directories at a given path in a repo. // ListContents lists files and directories at a given path in a repo.
gpt-review-bot commented 2026-05-11 14:12:14 +00:00
Review
Copy Link
Copy Source

[MINOR] Consider also normalizing equivalent root path variants such as "./" or "/" (if ever passed) to "" for robustness. Currently only exact "." is handled.

**[MINOR]** Consider also normalizing equivalent root path variants such as "./" or "/" (if ever passed) to "" for robustness. Currently only exact "." is handled.
gpt-review-bot commented 2026-05-11 14:13:43 +00:00
Review
Copy Link
Copy Source

[NIT] The ListContents doc comment says "Pass an empty path to list the repository root." Since "." is now accepted, consider updating the comment to note that "." is treated as root for clarity.

**[NIT]** The ListContents doc comment says "Pass an empty path to list the repository root." Since "." is now accepted, consider updating the comment to note that "." is treated as root for clarity.
// Pass an empty path to list the repository root. // Pass an empty path to list the repository root.
gpt-review-bot commented 2026-05-11 14:13:43 +00:00
Review
Copy Link
Copy Source

[MINOR] Normalization only handles the exact string ".". Consider also normalizing common root equivalents like "./" and trailing slash variants (e.g., "/" if ever passed) to improve robustness.

**[MINOR]** Normalization only handles the exact string ".". Consider also normalizing common root equivalents like "./" and trailing slash variants (e.g., "/" if ever passed) to improve robustness.
func (c *Client) ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error) { func (c *Client) ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error) {
// Normalize "." to empty string — Gitea API rejects "." with 500
gpt-review-bot commented 2026-05-11 14:12:12 +00:00
Review
Copy Link
Copy Source

[MINOR] Consider normalizing other dot-equivalent root paths such as "./" (and possibly variants like ".//") to the empty string as well, to be robust against alternate inputs. A small follow-up (e.g., trimming a single leading "./") and an extra test would harden this further.

**[MINOR]** Consider normalizing other dot-equivalent root paths such as "./" (and possibly variants like ".//") to the empty string as well, to be robust against alternate inputs. A small follow-up (e.g., trimming a single leading "./") and an extra test would harden this further.
if path == "." {
path = ""
aweiker commented 2026-05-11 14:02:57 +00:00
Review
Copy Link
Copy Source

Should this be '""' or '/'? In Rodin we needed to use a '/' as a '""' caused it to default to README.md

Should this be '""' or '/'? In Rodin we needed to use a '/' as a '""' caused it to default to README.md
}
var reqURL string var reqURL string
if path == "" { if path == "" {
reqURL = fmt.Sprintf("%s/api/v1/repos/%s/%s/contents", c.baseURL, url.PathEscape(owner), url.PathEscape(repo)) reqURL = fmt.Sprintf("%s/api/v1/repos/%s/%s/contents", c.baseURL, url.PathEscape(owner), url.PathEscape(repo))
gitea/client_test.go
+24
View File
@@ -280,6 +280,30 @@ func TestListContents(t *testing.T) {
} }
} }
func TestListContents_DotPath(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
// "." should be normalized to empty path, which hits the root contents endpoint
if r.URL.Path != "/api/v1/repos/owner/repo/contents" {
t.Errorf("expected root contents path, got: %s", r.URL.Path)
}
w.Header().Set("Content-Type", "application/json")
fmt.Fprintf(w, `[{"name":"README.md","path":"README.md","type":"file"}]`)
}))
defer server.Close()
client := NewClient(server.URL, "test-token")
entries, err := client.ListContents(context.Background(), "owner", "repo", ".")
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 README.md, got %s", entries[0].Name)
}
}
func TestGetAllFilesInPath_File(t *testing.T) { func TestGetAllFilesInPath_File(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.URL.Path == "/api/v1/repos/owner/repo/contents/README.md" { if r.URL.Path == "/api/v1/repos/owner/repo/contents/README.md" {