fix(gitea): normalize "." path to empty string in ListContents #72
@@ -38,6 +38,8 @@ jobs:
|
||||
- name: security
|
||||
token_secret: SECURITY_REVIEW_TOKEN
|
||||
model: gpt-5
|
||||
patterns_repo: rodin/security-patterns
|
||||
patterns_files: "."
|
||||
system_prompt_file: SECURITY_REVIEW.md
|
||||
steps:
|
||||
- uses: actions/checkout@v4
|
||||
@@ -60,8 +62,8 @@ jobs:
|
||||
AICORE_API_URL: ${{ secrets.AICORE_API_URL }}
|
||||
AICORE_RESOURCE_GROUP: ${{ secrets.AICORE_RESOURCE_GROUP }}
|
||||
CONVENTIONS_FILE: "CONVENTIONS.md"
|
||||
PATTERNS_REPO: "rodin/go-patterns"
|
||||
PATTERNS_FILES: "README.md,patterns/"
|
||||
PATTERNS_REPO: ${{ matrix.patterns_repo || 'rodin/go-patterns' }}
|
||||
|
security-review-bot marked this conversation as resolved
|
||||
PATTERNS_FILES: ${{ matrix.patterns_files || 'README.md,patterns/' }}
|
||||
LLM_TIMEOUT: "600"
|
||||
SYSTEM_PROMPT_FILE: ${{ matrix.system_prompt_file }}
|
||||
run: ./review-bot
|
||||
gpt-review-bot
commented
[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.
|
||||
|
||||
@@ -435,6 +435,10 @@ type ContentEntry struct {
|
||||
// ListContents lists files and directories at a given path in a repo.
|
||||
|
gpt-review-bot
commented
[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
[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.
|
||||
|
gpt-review-bot
commented
[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) {
|
||||
// Normalize "." to empty string — Gitea API rejects "." with 500
|
||||
|
gpt-review-bot
commented
[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
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
|
||||
if path == "" {
|
||||
reqURL = fmt.Sprintf("%s/api/v1/repos/%s/%s/contents", c.baseURL, url.PathEscape(owner), url.PathEscape(repo))
|
||||
|
||||
@@ -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) {
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
if r.URL.Path == "/api/v1/repos/owner/repo/contents/README.md" {
|
||||
|
||||
[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.