fix(gitea): handle single-object response in ListContents #74
Reference in New Issue
Block a user
Delete Branch "issue-73"
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 #73
Problem
When
ListContents()is called with a path that points to a file (not a directory), Gitea returns a single JSON object instead of an array:But the code tried to unmarshal it as an array, failing with:
Fix
This allows
patterns-filesconfig to specify individual files likeREADME.mdwithout triggering a parse error.Changes
ListContentsto handle single-object responses, added doc comment explaining the normalizationTestListContents_FilePathtest, updatedTestGetAllFilesInPath_Fileto reflect actual Gitea behavior (single object, not 404)Testing
go test ./...passesgo vet ./...passesscripts/check-deps.shpassesSelf-review: PASS
No issues found — ready for human review.
Summary
Coherence: Single purpose — handle Gitea's single-object response when path is a file.
Fit: Uses existing patterns (ContentEntry, error wrapping, httptest mocking).
Solution: Minimal fix — try array unmarshal first, fall back to single object, preserve error on both failures.
Completeness: Added
TestListContents_FilePath, updatedTestGetAllFilesInPath_Fileto reflect actual Gitea behavior.Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
1b6c3760)Sonnet Review
Summary
The change is minimal, correct, and well-tested. It handles Gitea's API inconsistency (single object vs array) with a clean try-first-then-fallback pattern, and the test coverage accurately reflects the actual Gitea behavior being fixed.
Findings
gitea/client.goContentEntry. If Gitea returns an empty JSON object{}, the unmarshal will succeed and return an entry with all zero/empty fields (Name: "", Path: "", Type: ""), which would be silently added to the results. This is an edge case but could cause confusing downstream behavior. A guard likeif single.Name == "" && single.Path == ""before wrapping would be more defensive, though arguably over-engineering for this specific Gitea API.gitea/client.goerr2shadows the outererrin a way that's slightly awkward —err(from the array unmarshal) is preserved for the outer error return whileerr2is used only for the single-object attempt. The naming is fine, but a comment noting whyerr(noterr2) is returned in the failure case would aid future readers: the original error is preferred because it describes the primary expected shape.Recommendation
APPROVE — The fix is correct and follows the codebase's established patterns — error wrapping with
fmt.Errorf("%w", err),httptest-based tests, table-driven style where applicable. CI passes. The minor finding about the empty-object edge case is a theoretical concern that doesn't warrant blocking this PR; the Gitea API won't return{}for a file path. Approve.Review by sonnet
Evaluated against
1b6c3760@@ -454,1 +456,3 @@return nil, fmt.Errorf("parse contents JSON: %w", err)// Gitea returns a single object (not an array) when path is a filevar single ContentEntryif err2 := json.Unmarshal(body, &single); err2 != nil {[MINOR] The fallback tries to unmarshal into a zero-value
ContentEntry. If Gitea returns an empty JSON object{}, the unmarshal will succeed and return an entry with all zero/empty fields (Name: "", Path: "", Type: ""), which would be silently added to the results. This is an edge case but could cause confusing downstream behavior. A guard likeif single.Name == "" && single.Path == ""before wrapping would be more defensive, though arguably over-engineering for this specific Gitea API.[NIT] The variable
err2shadows the outererrin a way that's slightly awkward —err(from the array unmarshal) is preserved for the outer error return whileerr2is used only for the single-object attempt. The naming is fine, but a comment noting whyerr(noterr2) is returned in the failure case would aid future readers: the original error is preferred because it describes the primary expected shape.Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
1b6c3760)Security Review
Summary
Change safely normalizes Gitea API responses that return either an array or a single object without introducing security risks. Uses JSON (data-only) parsing and returns minimal error information, aligning with secure error handling.
Recommendation
APPROVE — The update to ListContents adds robust handling for single-object responses and the accompanying tests reflect real Gitea behavior. No security issues were identified in these changes, and CI has passed. Proceed with merge.
Review by security
Evaluated against
1b6c3760Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
1b6c3760)Gpt Review
Summary
The change correctly normalizes Gitea’s single-object vs array responses for ListContents and updates tests to reflect real API behavior. The implementation is idiomatic, preserves original error context, and maintains existing error-handling patterns.
Findings
gitea/client.goRecommendation
APPROVE — The fix is well-implemented: it first attempts to unmarshal into a slice, then falls back to a single object and wraps it into a slice, and returns the original error if both fail, matching the documented behavior. Updated tests cover both directory and file paths and align GetAllFilesInPath with actual Gitea responses. CI is passing. I recommend merging as-is; optionally consider enhancing the error message in the dual-failure path to include both unmarshal errors for better diagnostics.
Review by gpt
Evaluated against
1b6c3760@@ -453,2 +455,3 @@if err := json.Unmarshal(body, &entries); err != nil {return nil, fmt.Errorf("parse contents JSON: %w", err)// Gitea returns a single object (not an array) when path is a filevar single ContentEntry[NIT] When both array and single-object unmarshals fail, the returned error wraps only the first error. Consider including details from the second error (e.g., via errors.Join or adding err2 to the message) to aid debugging malformed responses.
How do you know this solves the problem without any tests against the actual gitea API?
Good question. I verified this against the actual Gitea API:
File path (
/api/v1/repos/rodin/review-bot/contents/README.md):Directory path (
/api/v1/repos/rodin/review-bot/contents/gitea):The mocks in the tests match the actual API behavior:
The fix correctly handles both cases, and the httptest mocks reflect what Gitea actually returns. The original bug reported in #73 (unmarshal error for file paths) is fixed because the fallback try-single-object logic now handles the real response shape.
Addressed [MINOR] finding: guard against empty response in ListContents fallback
Added a defensive check at
gitea/client.go:461-464:Rationale: While the reviewer correctly noted that Gitea API won't return
{}for a valid file path, I opted to add the guard because:All tests pass. Commit:
c27dfd0Sonnet Review
Summary
The fix is correct, idiomatic, and well-tested. The try-array-first, fall-back-to-single-object approach handles the Gitea API quirk cleanly. The guard against empty/malformed single-object responses adds appropriate defensive behavior. Tests cover the new code path and correctly update an existing test to reflect actual Gitea behavior.
Findings
gitea/client.gosingle.Name == "" && single.Path == ""uses AND — a response where Name is populated but Path is empty (or vice versa) would pass the guard and produce a partially-valid entry. Using OR (`single.Name == ""gitea/client.goerr2from the single-object unmarshal attempt is silently discarded when returning the original array-unmarshal error. In diagnostic scenarios this loses information about why both parses failed. Consider wrapping or loggingerr2, e.g.fmt.Errorf("parse contents JSON (array: %v; object: %v)", err, err2). Low priority since the original error is the more meaningful one.Recommendation
APPROVE — Approve. CI passes, the implementation is correct, tests are adequate and follow established table-driven and httptest patterns from the project. The two nits are cosmetic and do not affect correctness or safety. The OR vs AND guard nit is the more defensible concern, but not a blocker given real-world Gitea response shapes.
Review by sonnet
Evaluated against
c27dfd0f@@ -452,3 +454,3 @@var entries []ContentEntryif err := json.Unmarshal(body, &entries); err != nil {return nil, fmt.Errorf("parse contents JSON: %w", err)// Gitea returns a single object (not an array) when path is a file[NIT] The inner error
err2from the single-object unmarshal attempt is silently discarded when returning the original array-unmarshal error. In diagnostic scenarios this loses information about why both parses failed. Consider wrapping or loggingerr2, e.g.fmt.Errorf("parse contents JSON (array: %v; object: %v)", err, err2). Low priority since the original error is the more meaningful one.@@ -455,0 +457,4 @@var single ContentEntryif err2 := json.Unmarshal(body, &single); err2 != nil {return nil, fmt.Errorf("parse contents JSON: %w", err)}[NIT] The guard condition
single.Name == "" && single.Path == ""uses AND — a response where Name is populated but Path is empty (or vice versa) would pass the guard and produce a partially-valid entry. Using OR (single.Name == "" || single.Path == "") would be more defensive, though in practice Gitea always returns both fields for a valid file entry.Security Review
Summary
Changes add safe handling for Gitea's single-object JSON response in ListContents and corresponding tests. No new security risks are introduced, and input continues to be safely URL-escaped and JSON-unmarshalled into fixed structs.
Recommendation
APPROVE — The update correctly normalizes single-object responses without introducing injection, deserialization, or error-handling weaknesses. CI has passed, and there are no security-relevant concerns in the changes. Proceed with merge.
Review by security
Evaluated against
c27dfd0fGpt Review
Summary
The changes correctly handle Gitea’s single-object response when the path points to a file, and tests comprehensively cover both directory and file cases. Error handling and docs are consistent with project conventions and Go patterns.
Recommendation
APPROVE — The implementation of ListContents now gracefully supports both array and single-object JSON responses, and the added/updated tests validate the new behavior without regressing existing functionality. CI has passed, dependencies remain within the approved set, and the code adheres to the repository’s error handling and testing patterns. Proceed with merge.
Review by gpt
Evaluated against
c27dfd0f