But the code tried to unmarshal it as an array, failing with:
json: cannot unmarshal object into Go value of type []gitea.ContentEntry
Fix
Try array unmarshal first (current behavior for directories)
If that fails, try single object unmarshal
If single object succeeds, wrap it in a slice
If both fail, return the original error
This allows patterns-files config to specify individual files like README.md without triggering a parse error.
Changes
gitea/client.go: Updated ListContents to handle single-object responses, added doc comment explaining the normalization
gitea/client_test.go: Added TestListContents_FilePath test, updated TestGetAllFilesInPath_File to reflect actual Gitea behavior (single object, not 404)
Testing
go test ./... passes
go vet ./... passes
scripts/check-deps.sh passes
## 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:
```json
{"name":"README.md","path":"README.md","type":"file"}
```
But the code tried to unmarshal it as an array, failing with:
```
json: cannot unmarshal object into Go value of type []gitea.ContentEntry
```
## Fix
1. Try array unmarshal first (current behavior for directories)
2. If that fails, try single object unmarshal
3. If single object succeeds, wrap it in a slice
4. If both fail, return the original error
This allows `patterns-files` config to specify individual files like `README.md` without triggering a parse error.
## Changes
- **gitea/client.go**: Updated `ListContents` to handle single-object responses, added doc comment explaining the normalization
- **gitea/client_test.go**: Added `TestListContents_FilePath` test, updated `TestGetAllFilesInPath_File` to reflect actual Gitea behavior (single object, not 404)
## Testing
- `go test ./...` passes
- `go vet ./...` passes
- `scripts/check-deps.sh` passes
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.
Previously this caused json.Unmarshal to fail with:
json: cannot unmarshal object into Go value of type []gitea.ContentEntry
Now ListContents tries array unmarshal first, and falls back to single
object unmarshal, wrapping it in a slice. This allows patterns-files
config to specify individual files like 'README.md' without triggering
a parse error.
Also updates TestGetAllFilesInPath_File to reflect actual Gitea behavior
(single object response, not 404).
Fixes#73
Solution: Minimal fix — try array unmarshal first, fall back to single object, preserve error on both failures.
Completeness: Added TestListContents_FilePath, updated TestGetAllFilesInPath_File to reflect actual Gitea behavior.
## Self-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`, updated `TestGetAllFilesInPath_File` to reflect actual Gitea behavior.
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
#
Severity
File
Line
Finding
1
[MINOR]
gitea/client.go
458
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 like if single.Name == "" && single.Path == "" before wrapping would be more defensive, though arguably over-engineering for this specific Gitea API.
2
[NIT]
gitea/client.go
458
The variable err2 shadows the outer err in a way that's slightly awkward — err (from the array unmarshal) is preserved for the outer error return while err2 is used only for the single-object attempt. The naming is fine, but a comment noting why err (not err2) 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.
~~Original review~~
**Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/74#pullrequestreview-2545) for up-to-date findings.
<details><summary>Previous findings (commit 1b6c3760)</summary>
# 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
| # | Severity | File | Line | Finding |
|---|----------|------|------|--------|
| 1 | [MINOR] | `gitea/client.go` | 458 | 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 like `if single.Name == "" && single.Path == ""` before wrapping would be more defensive, though arguably over-engineering for this specific Gitea API. |
| 2 | [NIT] | `gitea/client.go` | 458 | The variable `err2` shadows the outer `err` in a way that's slightly awkward — `err` (from the array unmarshal) is preserved for the outer error return while `err2` is used only for the single-object attempt. The naming is fine, but a comment noting why `err` (not `err2`) 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*
<!-- review-bot:sonnet -->
---
*Evaluated against 1b6c3760*
</details>
<!-- review-bot:sonnet -->
[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 like if single.Name == "" && single.Path == "" before wrapping would be more defensive, though arguably over-engineering for this specific Gitea API.
**[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 like `if single.Name == "" && single.Path == ""` before wrapping would be more defensive, though arguably over-engineering for this specific Gitea API.
[NIT] The variable err2 shadows the outer err in a way that's slightly awkward — err (from the array unmarshal) is preserved for the outer error return while err2 is used only for the single-object attempt. The naming is fine, but a comment noting why err (not err2) is returned in the failure case would aid future readers: the original error is preferred because it describes the primary expected shape.
**[NIT]** The variable `err2` shadows the outer `err` in a way that's slightly awkward — `err` (from the array unmarshal) is preserved for the outer error return while `err2` is used only for the single-object attempt. The naming is fine, but a comment noting why `err` (not `err2`) is returned in the failure case would aid future readers: the original error is preferred because it describes the primary expected shape.
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.
~~Original review~~
**Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/74#pullrequestreview-2547) for up-to-date findings.
<details><summary>Previous findings (commit 1b6c3760)</summary>
# 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*
<!-- review-bot:security -->
---
*Evaluated against 1b6c3760*
</details>
<!-- review-bot:security -->
gpt-review-bot
approved these changes 2026-05-11 14:23:09 +00:00
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
#
Severity
File
Line
Finding
1
[NIT]
gitea/client.go
457
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.
Recommendation
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.
~~Original review~~
**Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/74#pullrequestreview-2548) for up-to-date findings.
<details><summary>Previous findings (commit 1b6c3760)</summary>
# 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
| # | Severity | File | Line | Finding |
|---|----------|------|------|--------|
| 1 | [NIT] | `gitea/client.go` | 457 | 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. |
## Recommendation
**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*
<!-- review-bot:gpt -->
---
*Evaluated against 1b6c3760*
</details>
<!-- review-bot:gpt -->
[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.
**[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.
The mocks in the tests match the actual API behavior:
Directory → array
File → single object (not array)
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.
Good question. I verified this against the actual Gitea API:
**File path** (`/api/v1/repos/rodin/review-bot/contents/README.md`):
```json
{"name":"README.md","path":"README.md","sha":"cd9fc03889aa...","type":"file",...}
```
**Directory path** (`/api/v1/repos/rodin/review-bot/contents/gitea`):
```json
[{"name":"client.go","path":"gitea/client.go",...}, ...]
```
The mocks in the tests match the actual API behavior:
- Directory → array
- File → single object (not array)
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.
Add defensive check for empty Name and Path fields when unmarshaling
a single ContentEntry in the fallback path. While Gitea API won't
return empty objects for valid file paths, this guard:
- Explicitly documents the invariant we expect
- Catches potential API behavior changes early
- Costs nothing at runtime
Addresses [MINOR] from sonnet-review-bot on PR #74.
**Addressed [MINOR] finding: guard against empty response in ListContents fallback**
Added a defensive check at `gitea/client.go:461-464`:
```go
// Guard against empty/malformed responses
if single.Name == "" && single.Path == "" {
return nil, fmt.Errorf("parse contents JSON: empty response for path %q", path)
}
```
**Rationale:** While the reviewer correctly noted that Gitea API won't return `{}` for a valid file path, I opted to add the guard because:
1. **Zero runtime cost** — a single conditional check
2. **Explicit invariant** — documents what we expect from the API
3. **Future-proofing** — catches API behavior changes early rather than silently corrupting state
4. **Defensive coding** — "that can't happen" is an assumption wearing a disguise
All tests pass. Commit: c27dfd0
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
#
Severity
File
Line
Finding
1
[NIT]
gitea/client.go
460
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 == ""
2
[NIT]
gitea/client.go
456
The inner error err2 from 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 logging err2, 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.
# Sonnet 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
| # | Severity | File | Line | Finding |
|---|----------|------|------|--------|
| 1 | [NIT] | `gitea/client.go` | 460 | 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. |
| 2 | [NIT] | `gitea/client.go` | 456 | The inner error `err2` from 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 logging `err2`, 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*
<!-- review-bot:sonnet -->
---
*Evaluated against c27dfd0f*
[NIT] The inner error err2 from 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 logging err2, e.g. fmt.Errorf("parse contents JSON (array: %v; object: %v)", err, err2). Low priority since the original error is the more meaningful one.
**[NIT]** The inner error `err2` from 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 logging `err2`, e.g. `fmt.Errorf("parse contents JSON (array: %v; object: %v)", err, err2)`. Low priority since the original error is the more meaningful one.
[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.
**[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-bot
requested review from security-review-bot 2026-05-11 14:48:13 +00:00
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.
# 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*
<!-- review-bot:security -->
---
*Evaluated against c27dfd0f*
gpt-review-bot
approved these changes 2026-05-11 14:48:19 +00:00
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.
# Gpt 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*
<!-- review-bot:gpt -->
---
*Evaluated against c27dfd0f*
aweiker
merged commit 019b815280 into main2026-05-11 15:38:59 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
aweiker
sonnet-review-bot
security-review-bot
gpt-review-bot
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