fix(gitea): normalize "." path to empty string in ListContents #72
Reference in New Issue
Block a user
Delete Branch "issue-70"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Fixes #70 —
patterns-files="."causes 500 error from Gitea API.Problem
When
patterns-filesis set to., the client passes it directly to the Gitea contents API:Gitea rejects this with HTTP 500 because
.is considered a malformed path component.Solution
Normalize
.to empty string at the start ofListContents(). This makes.behave identically to"", which correctly lists the repository root.Testing
TestListContents_DotPaththat verifies.is normalized to the root contents endpointOriginal reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
e6b1840f)Sonnet Review
Summary
Minimal, correct fix for a well-defined bug. The normalization is placed at the right boundary (entry point of ListContents), the comment explains the why, and the test directly verifies the URL path correction.
Recommendation
APPROVE — The change is correct and idiomatic. Normalizing '.' to '' at the start of ListContents is the right place — it's a single guard clause that handles a known-bad input before it reaches URL construction. CI passes. Approve as-is.
Review by sonnet
Evaluated against
e6b1840fSelf-review: PASS
Coherence: Single-purpose fix — normalizes
.to empty string. No scope creep.Fit: Matches existing pattern (empty string handling already exists at line 443). Comment explains the why.
Solution: Simplest fix per issue #70.
.should behave identically to""(both mean repo root).Completeness: Test added (
TestListContents_DotPath) that verifies the normalization and fails without the fix.No issues found — ready for human review.
Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
e6b1840f)Security Review
Summary
The change safely normalizes a '.' path to an empty string in ListContents and adds a corresponding test. There are no security implications introduced by this update.
Recommendation
APPROVE — CI passed and the modification is minimal and benign from a security perspective. The path normalization does not weaken input validation or escaping, and all URL components remain properly escaped. Approve the PR.
Review by security
Evaluated against
e6b1840fOriginal reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
e6b1840f)Gpt Review
Summary
The change correctly normalizes a dot path to the repository root for Gitea contents API calls and adds a focused test verifying the behavior. The implementation is minimal, idiomatic, and aligns with the stated problem; all tests pass.
Recommendation
APPROVE — This is a well-scoped fix for the reported issue: normalizing '.' to an empty path avoids Gitea's 500 error and preserves expected semantics. The accompanying test effectively guards against regressions. As a non-blocking enhancement, consider normalizing additional dot-segment variants (e.g., './', 'path/.') if they can surface from user inputs, potentially by applying a conservative clean for the root-only case, but this can be addressed separately. Approving as is.
Review by gpt
Evaluated against
e6b1840f@@ -437,1 +437,4 @@func (c *Client) ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error) {// Normalize "." to empty string — Gitea API rejects "." with 500if path == "." {path = ""Should this be '""' or '/'? In Rodin we needed to use a '/' as a '""' caused it to default to README.md
Re: Should this be
""or/?Empty string is correct here. In
ListContents, empty string produces:(no path suffix), which Gitea handles correctly and returns an array.
The reason gargoyle needed
/was different: the action.yml hasdefault: 'README.md'forpatterns-files. So when the workflow passespatterns-files: '', the action treats it as unset and substitutes the default. That's issue #71 (bad default).This fix (normalizing
.→"") handles the case where someone explicitly passes.meaning "repo root". Both.and""should produce the same API call:""→/contents✓.→ (after this fix)/contents✓/→/contents/(also works, but trailing slash is unnecessary)Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
b0349a22)Sonnet Review
Summary
Clean, minimal fix that normalizes '.' to empty string in ListContents before URL construction. The implementation is correct, well-placed, and accompanied by a focused unit test that verifies the exact behavior. The CI workflow addition provides an end-to-end smoke test for the fix.
Findings
.gitea/workflows/ci.ymltest-dot-pathCI job runs the full review-bot against a real patterns repo (rodin/security-patterns) on every PR. This is an integration test that consumes LLM API credits and external dependencies to verify what is already covered by the unit testTestListContents_DotPath. Once the fix is merged, this job has no ongoing value and becomes permanent noise. Consider removing it after the fix lands, or making it a one-off manual trigger rather than a permanent PR check.Recommendation
APPROVE — Approve. The code change is correct and minimal — a 3-line guard at the top of
ListContentswith a clear comment, exactly where the normalization should live. The unit test is well-structured and directly verifies the URL path that gets hit. The only concern is the new CI job: it's a reasonable way to verify the fix end-to-end for this PR, but it should be cleaned up after merge since the unit test provides sufficient ongoing coverage. This is a NIT-level process concern, not a blocking issue.Review by sonnet
Evaluated against
b0349a22@@ -67,1 +67,4 @@run: ./review-bot# Test dot path normalization with security-patterns repotest-dot-path:[NIT] The
test-dot-pathCI job runs the full review-bot against a real patterns repo (rodin/security-patterns) on every PR. This is an integration test that consumes LLM API credits and external dependencies to verify what is already covered by the unit testTestListContents_DotPath. Once the fix is merged, this job has no ongoing value and becomes permanent noise. Consider removing it after the fix lands, or making it a one-off manual trigger rather than a permanent PR check.Gpt Review
Summary
Change correctly normalizes '.' to the repository root in ListContents and adds a focused unit test. CI passed and the update aligns with error-handling and testing conventions.
Findings
gitea/client.goRecommendation
APPROVE — The fix is concise, correct, and well-tested. Normalizing '.' eliminates the Gitea 500 issue and the new unit test validates behavior. CI passed. As a minor enhancement, consider also handling './' to cover more user inputs that should map to the repo root, and add a corresponding test. Otherwise, this is good to merge.
Review by gpt
Evaluated against
b0349a22@@ -435,6 +435,10 @@ 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) {// Normalize "." to empty string — Gitea API rejects "." with 500[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.
Dot-path-test Review
Summary
Change cleanly normalizes a dot path to the repository root in ListContents and adds a focused unit test. CI passed and the implementation is small, safe, and consistent with existing patterns.
Findings
gitea/client.go.gitea/workflows/ci.ymlRecommendation
APPROVE — The fix is correct and minimal: normalizing "." to "" avoids the Gitea API 500 and aligns with the intended semantics of listing the repo root. The added unit test exercises the new behavior and the rest of the code remains unchanged. CI has passed. Optionally, you could extend the normalization to handle "./" or "/" inputs and reconsider the necessity of the extra CI job that depends on external secrets. Otherwise, this is good to merge.
Review by dot-path-test
Evaluated against
b0349a22@@ -66,2 +66,4 @@SYSTEM_PROMPT_FILE: ${{ matrix.system_prompt_file }}run: ./review-bot# Test dot path normalization with security-patterns repo[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.[MINOR] Consider also normalizing equivalent root path variants such as "./" or "/" (if ever passed) to "" for robustness. Currently only exact "." is handled.
Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
b0349a22)Security Review
Summary
The change cleanly normalizes a '.' path to the repository root in ListContents and adds a focused test. CI is passing and there are no new security vulnerabilities in the application code.
Findings
.gitea/workflows/ci.ymlRecommendation
APPROVE — The code change in gitea/client.go is correct and safe: normalizing a single '.' path to the empty string avoids Gitea API 500 responses without introducing injection or traversal risks. The added test validates the behavior. CI is green.
One minor security consideration: the newly added CI job ('test-dot-path') passes secrets to a binary built from the PR. Confirm that your CI is configured to withhold secrets for untrusted/forked PRs or restrict this job to trusted contributors/branches. Otherwise, consider using a less-privileged token for this test or running it without secrets.
No other security issues were introduced by this PR.
Review by security
Evaluated against
b0349a22@@ -65,3 +65,34 @@ jobs:LLM_TIMEOUT: "600"[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.
b0349a22a0toea74f7e088Sonnet Review
Summary
Small, well-targeted fix that normalizes '.' to empty string at the start of ListContents() to prevent Gitea API 500 errors. The change is correct, minimal, well-commented, and accompanied by a focused test. The CI workflow change correctly makes patterns_repo and patterns_files matrix-configurable.
Recommendation
APPROVE — Approve. The fix is exactly right: the normalization happens at the single entry point (ListContents), the comment explains the why, the test directly verifies the expected URL is hit, and all existing tests continue to pass per CI. No issues found.
Review by sonnet
Evaluated against
ea74f7e0Security Review
Summary
The changes normalize a '.' path to the repository root for Gitea contents listing and adjust CI to use security patterns with matrix-provided defaults. There are no security-relevant risks introduced by these changes.
Recommendation
APPROVE — Approve the PR. The normalization of '.' to an empty path in ListContents prevents a server-side error without affecting security, and the CI workflow modifications use static matrix values and safe defaults. No injection, auth/authorization, secrets exposure, or other security concerns are introduced.
Review by security
Evaluated against
ea74f7e0Gpt Review
Summary
Change correctly normalizes "." to root for Gitea contents API and adds a focused test. CI is green and the workflow updates look sound with sensible matrix defaults.
Findings
gitea/client.gogitea/client.goRecommendation
APPROVE — The implementation precisely addresses the Gitea 500 error by mapping "." to the root endpoint and includes a targeted test that verifies the request path. Workflow adjustments introduce matrix-configurable defaults for patterns without breaking existing jobs. Consider extending the normalization to also treat "./" (and potentially other benign root forms) as empty, and update the ListContents comment to reflect that "." is accepted as root. Otherwise, the changes are correct and idiomatic.
Review by gpt
Evaluated against
ea74f7e0@@ -435,6 +435,10 @@ type ContentEntry struct {// ListContents lists files and directories at a given path in a repo.[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.
@@ -435,6 +435,10 @@ type ContentEntry struct {// ListContents lists files and directories at a given path in a repo.// Pass an empty path to list the repository root.[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.