fix: quick wins (#7, #9, #13) #16

2026-05-01T21:19:47Z

rodin commented

2026-05-01 21:19:47 +00:00

Changes

#9: Add --version flag and log version on startup
#7: URL-escape ref query parameter in GetFileContentRef (branch names with /, +, spaces)
#13: Add go vet to release workflow

Details

Renamed local url variable to reqURL in GetFileContentRef to avoid shadowing the net/url package import
Version is injected via ldflags at build time (already set up in release workflow)
All tests pass, go vet clean

Closes #7, closes #9, closes #13

## Changes - **#9**: Add `--version` flag and log version on startup - **#7**: URL-escape `ref` query parameter in `GetFileContentRef` (branch names with `/`, `+`, spaces) - **#13**: Add `go vet` to release workflow ## Details - Renamed local `url` variable to `reqURL` in `GetFileContentRef` to avoid shadowing the `net/url` package import - Version is injected via ldflags at build time (already set up in release workflow) - All tests pass, `go vet` clean Closes #7, closes #9, closes #13

rodin added 1 commit 2026-05-01 21:19:47 +00:00

fix: quick wins (#7 , #9 , #13 )

CI / test (pull_request) Successful in 13s

Details

CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 59s

Details

CI / review (gpt-5-mini, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m48s

Details

b02ade4f23

- Add --version flag and log version on startup (closes #9)
- URL-escape ref query parameter in GetFileContentRef (closes #7)
- Add go vet to release workflow (closes #13)

Renamed local url variable to reqURL to avoid shadowing net/url package.

sonnet-review-bot approved these changes 2026-05-01 21:21:01 +00:00

sonnet-review-bot left a comment

First-time contributor

Summary

Changes are small, focused, and correct: version flag and startup logging are implemented cleanly, ref is properly URL-escaped to handle special characters, and go vet is added to the release workflow. CI is passing.

Recommendation

APPROVE — The updates are solid and align with the repository’s conventions. Using url.QueryEscape for the ref parameter is appropriate for query values and resolves issues with slashes, plus signs, and spaces in branch names. Adding go vet to the workflow is a welcome improvement. Consider a future cleanup to consistently avoid local variables named 'url' in gitea/client.go for readability now that the net/url package is imported, and evaluate whether path segments like 'filepath' should be path-escaped when constructing raw file URLs. These are non-blocking suggestions. Proceed to merge.

## Summary Changes are small, focused, and correct: version flag and startup logging are implemented cleanly, ref is properly URL-escaped to handle special characters, and go vet is added to the release workflow. CI is passing. ## Recommendation **APPROVE** — The updates are solid and align with the repository’s conventions. Using url.QueryEscape for the ref parameter is appropriate for query values and resolves issues with slashes, plus signs, and spaces in branch names. Adding go vet to the workflow is a welcome improvement. Consider a future cleanup to consistently avoid local variables named 'url' in gitea/client.go for readability now that the net/url package is imported, and evaluate whether path segments like 'filepath' should be path-escaped when constructing raw file URLs. These are non-blocking suggestions. Proceed to merge.

gpt-review-bot approved these changes 2026-05-01 21:21:52 +00:00

gpt-review-bot left a comment

First-time contributor

Summary

Changes are small, sensible, and improve behavior (adds a --version flag, fixes query-escaping for the ref, and runs go vet in the release workflow). No CI failures reported and the modifications avoid the url package shadowing issue.

Findings

#	Severity	File	Line	Finding
1	[MINOR]	`gitea/client.go`	118	GetFileContentRef now correctly QueryEscapes the ref and renames the local variable to reqURL to avoid shadowing the net/url package. However, filepath (and other path segments) are still embedded into the URL without path-escaping. If file paths can contain characters that need escaping, consider using url.PathEscape on path segments or building the URL with net/url.URL to be robust.
2	[NIT]	`gitea/client.go`	8	The net/url package import was added to support QueryEscape. Note that the file still has a function doGet(ctx, url string) where the parameter name url shadows the package name inside that function's scope. While harmless here, renaming that parameter (for example to reqURL) would avoid potential confusion and make the code clearer.
3	[NIT]	`cmd/review-bot/main.go`	37	A --version flag has been added which prints to stdout and exits — good addition. Consider adding it to the CLI help/usage output explicitly (or ensuring flag.Usage includes it) so users see it when running --help.
4	[NIT]	`.gitea/workflows/release.yml`	16	go vet was added before running tests in the release workflow. This is a helpful addition. Consider running 'go vet ./...' (as you do) earlier in other CI workflows as well to catch issues earlier in PRs, if not already present.

Recommendation

APPROVE — Approve these changes. The version flag and ref escaping are correct and helpful improvements. For maximum robustness, consider path-escaping filepath and other URL path segments when constructing API URLs (or use net/url.URL to compose URLs), and rename the doGet parameter named 'url' to avoid shadowing/confusion. Also consider ensuring the --version flag is documented in CLI usage output. Other than these minor suggestions, the patch looks good.

## Summary Changes are small, sensible, and improve behavior (adds a --version flag, fixes query-escaping for the ref, and runs go vet in the release workflow). No CI failures reported and the modifications avoid the url package shadowing issue. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `gitea/client.go` | 118 | GetFileContentRef now correctly QueryEscapes the ref and renames the local variable to reqURL to avoid shadowing the net/url package. However, filepath (and other path segments) are still embedded into the URL without path-escaping. If file paths can contain characters that need escaping, consider using url.PathEscape on path segments or building the URL with net/url.URL to be robust. | | 2 | [NIT] | `gitea/client.go` | 8 | The net/url package import was added to support QueryEscape. Note that the file still has a function doGet(ctx, url string) where the parameter name url shadows the package name inside that function's scope. While harmless here, renaming that parameter (for example to reqURL) would avoid potential confusion and make the code clearer. | | 3 | [NIT] | `cmd/review-bot/main.go` | 37 | A --version flag has been added which prints to stdout and exits — good addition. Consider adding it to the CLI help/usage output explicitly (or ensuring flag.Usage includes it) so users see it when running --help. | | 4 | [NIT] | `.gitea/workflows/release.yml` | 16 | go vet was added before running tests in the release workflow. This is a helpful addition. Consider running 'go vet ./...' (as you do) earlier in other CI workflows as well to catch issues earlier in PRs, if not already present. | ## Recommendation **APPROVE** — Approve these changes. The version flag and ref escaping are correct and helpful improvements. For maximum robustness, consider path-escaping filepath and other URL path segments when constructing API URLs (or use net/url.URL to compose URLs), and rename the doGet parameter named 'url' to avoid shadowing/confusion. Also consider ensuring the --version flag is documented in CLI usage output. Other than these minor suggestions, the patch looks good.

rodin merged commit fc23b6ebe9 into main

2026-05-01 21:30:57 +00:00

rodin referenced this issue from a commit

2026-05-01 21:30:58 +00:00

Merge pull request 'fix: quick wins (#7, #9, #13)' (#16) from fix/quick-wins into main

rodin referenced this issue from a commit

2026-05-01 21:33:19 +00:00

fix: path-escape file paths and eliminate url package shadowing

rodin referenced this pull request

2026-05-01 21:33:27 +00:00

fix: path-escape file paths and eliminate url package shadowing #17

Sign in to join this conversation.