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

Merged

rodin merged 4 commits from fix/url-escaping-and-shadow into main 2026-05-01 21:55:02 +00:00

Conversation 6 Commits 4 Files Changed 4

+65 -17

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

Owner

Copy Link

Copy Source

Addresses remaining findings from PR #16 review:

• [MINOR] File paths now escaped via escapePath() helper — escapes each segment individually (preserves / as separator, escapes spaces/#/? etc)
• [NIT] Renamed doGet parameter from url to reqURL to avoid shadowing net/url package
• Consistent variable naming across all URL builders

All tests pass, go vet clean.

Addresses remaining findings from PR #16 review: - **[MINOR]** File paths now escaped via `escapePath()` helper — escapes each segment individually (preserves `/` as separator, escapes spaces/`#`/`?` etc) - **[NIT]** Renamed `doGet` parameter from `url` to `reqURL` to avoid shadowing `net/url` package - Consistent variable naming across all URL builders All tests pass, `go vet` clean.

rodin added 1 commit 2026-05-01 21:33:27 +00:00

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

- Add escapePath() helper: escapes each path segment individually
  (preserves slashes as separators, escapes spaces/#/? etc)
- Apply to GetFileContent, GetFileContentRef, ListContents
- Rename doGet parameter from url to reqURL (avoids shadowing net/url)
- Rename local variables in GetFileContent/ListContents for consistency

Addresses remaining findings from PR #16 review.

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

Dismissed

sonnet-review-bot left a comment

First-time contributor

Copy Link

Copy Source

Summary

Changes correctly escape path segments and avoid net/url shadowing in modified areas. Implementation is idiomatic, error handling is preserved, and CI passed.

Findings

#	Severity	File	Line	Finding
1	[NIT]	gitea/client.go	164	Consider using the http.MethodGet constant instead of the literal "GET" when creating the request.
2	[MINOR]	gitea/client.go	108	Good rename to reqURL and path escaping here; for consistency, consider applying the same reqURL naming to other URL-building locals elsewhere in this package.
3	[NIT]	gitea/client.go	184	escapePath preserves leading/trailing slashes and empty segments (e.g., "//"), which may result in doubled slashes in constructed URLs. Consider trimming a leading slash or documenting that inputs should be relative paths.

Recommendation

APPROVE — The updates meaningfully improve URL handling by escaping path segments and avoid net/url shadowing where it mattered. With CI passing and no functional issues identified, this is good to merge. Optionally, for follow-up polish, consider standardizing on reqURL naming across all URL-building locals, using http.Method* constants in requests, and clarifying/normalizing leading slashes in escapePath.

## Summary Changes correctly escape path segments and avoid net/url shadowing in modified areas. Implementation is idiomatic, error handling is preserved, and CI passed. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [NIT] | `gitea/client.go` | 164 | Consider using the http.MethodGet constant instead of the literal "GET" when creating the request. | | 2 | [MINOR] | `gitea/client.go` | 108 | Good rename to reqURL and path escaping here; for consistency, consider applying the same reqURL naming to other URL-building locals elsewhere in this package. | | 3 | [NIT] | `gitea/client.go` | 184 | escapePath preserves leading/trailing slashes and empty segments (e.g., "//"), which may result in doubled slashes in constructed URLs. Consider trimming a leading slash or documenting that inputs should be relative paths. | ## Recommendation **APPROVE** — The updates meaningfully improve URL handling by escaping path segments and avoid net/url shadowing where it mattered. With CI passing and no functional issues identified, this is good to merge. Optionally, for follow-up polish, consider standardizing on reqURL naming across all URL-building locals, using http.Method* constants in requests, and clarifying/normalizing leading slashes in escapePath.

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

Dismissed

gpt-review-bot left a comment

First-time contributor

Copy Link

Copy Source

Summary

The change correctly introduces escapePath to safely escape file path segments and avoids package shadowing in doGet by renaming the parameter. The modifications are small, targeted, and appear correct; CI passed and go vet is clean.

Findings

#	Severity	File	Line	Finding
1	[MINOR]	gitea/client.go	108	GetFileContent and GetFileContentRef now use escapePath to encode each path segment — this is the right approach. Consider adding unit tests for escapePath to cover edge cases (empty path, leading/trailing slashes, already-encoded segments) to make behavior explicit.
2	[MINOR]	gitea/client.go	164	doGet parameter renamed from url to reqURL to avoid shadowing the net/url package — good change. There are no other code references requiring updates due to this rename.
3	[MINOR]	gitea/client.go	184	escapePath implementation splits on '/' and applies url.PathEscape to each segment, then rejoins. This preserves slashes and escapes special characters. Be aware this will re-escape any already percent-encoded segments (i.e., it will percent-encode '%' if present), which is usually desired but should be documented and tested.
4	[NIT]	gitea/client.go	193	You updated several URL builder variables to reqURL for consistency (ListContents, GetFileContent, etc.). A small follow-up improvement would be to consistently rename other local variables named 'url' (e.g. in GetPullRequest, PostReview, GetCommitStatuses) to avoid any future confusion or accidental shadowing of the net/url package.

Recommendation

APPROVE — Approve these changes. They fix the important issue of safely escaping file path segments and remove a potential package-shadowing pitfall in doGet. Before merging, consider adding unit tests for escapePath to assert behavior with empty paths, leading/trailing slashes, and already-encoded input, and optionally standardize the request URL local variable name across all URL-building functions for consistency.

## Summary The change correctly introduces escapePath to safely escape file path segments and avoids package shadowing in doGet by renaming the parameter. The modifications are small, targeted, and appear correct; CI passed and go vet is clean. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `gitea/client.go` | 108 | GetFileContent and GetFileContentRef now use escapePath to encode each path segment — this is the right approach. Consider adding unit tests for escapePath to cover edge cases (empty path, leading/trailing slashes, already-encoded segments) to make behavior explicit. | | 2 | [MINOR] | `gitea/client.go` | 164 | doGet parameter renamed from url to reqURL to avoid shadowing the net/url package — good change. There are no other code references requiring updates due to this rename. | | 3 | [MINOR] | `gitea/client.go` | 184 | escapePath implementation splits on '/' and applies url.PathEscape to each segment, then rejoins. This preserves slashes and escapes special characters. Be aware this will re-escape any already percent-encoded segments (i.e., it will percent-encode '%' if present), which is usually desired but should be documented and tested. | | 4 | [NIT] | `gitea/client.go` | 193 | You updated several URL builder variables to reqURL for consistency (ListContents, GetFileContent, etc.). A small follow-up improvement would be to consistently rename other local variables named 'url' (e.g. in GetPullRequest, PostReview, GetCommitStatuses) to avoid any future confusion or accidental shadowing of the net/url package. | ## Recommendation **APPROVE** — Approve these changes. They fix the important issue of safely escaping file path segments and remove a potential package-shadowing pitfall in doGet. Before merging, consider adding unit tests for escapePath to assert behavior with empty paths, leading/trailing slashes, and already-encoded input, and optionally standardize the request URL local variable name across all URL-building functions for consistency.

rodin referenced this issue from a commit 2026-05-01 21:40:22 +00:00

fix: address all review findings from PR #17

rodin added 1 commit 2026-05-01 21:40:22 +00:00

fix: address all review findings from PR #17 ... dd2661fe14

- Rename all remaining url locals to reqURL (consistency)
- Use http.MethodGet/http.MethodPost constants
- Document escapePath: relative paths only, double-encoding expected
- Add TestEscapePath with 7 edge cases (empty, spaces, #, deep, encoded)

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

Dismissed

sonnet-review-bot left a comment

First-time contributor

Copy Link

Copy Source

Summary

Changes are correct and idiomatic: variable renaming avoids package shadowing, HTTP method constants are used, and file path segments are safely escaped. Added tests for escapePath are appropriate and CI passed.

Recommendation

APPROVE — The PR improves robustness by escaping file path segments with url.PathEscape and clarifies code by avoiding net/url shadowing. Consistent use of http.Method* constants is a nice touch. Tests cover the new helper well, and overall behavior remains clear and correct. No further changes requested.

## Summary Changes are correct and idiomatic: variable renaming avoids package shadowing, HTTP method constants are used, and file path segments are safely escaped. Added tests for escapePath are appropriate and CI passed. ## Recommendation **APPROVE** — The PR improves robustness by escaping file path segments with url.PathEscape and clarifies code by avoiding net/url shadowing. Consistent use of http.Method* constants is a nice touch. Tests cover the new helper well, and overall behavior remains clear and correct. No further changes requested.

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

Dismissed

gpt-review-bot left a comment

First-time contributor

Copy Link

Copy Source

Summary

The changes correctly avoid shadowing the net/url package, consistently use http.Method* constants, and add a robust escapePath helper with accompanying tests. Code is idiomatic, well-documented, and tests/CI pass.

Findings

#	Severity	File	Line	Finding
1	[NIT]	gitea/client.go	56	Renamed local URL variables to reqURL to avoid shadowing the net/url package — good change. Consider keeping this convention consistently across future additions.
2	[MINOR]	gitea/client.go	184	escapePath double-encodes already-encoded segments (e.g. %20 -> %2520). The comment documents this as intentional, but this behavior can be surprising in some contexts; ensure callers never pass already-escaped segments unintentionally.
3	[MINOR]	gitea/client.go	205	ListContents builds the URL with '/contents/%s' and passes escapePath(path). If path is empty (root), this yields '/contents/' (trailing slash) instead of '/contents'. Although many servers accept both, the API may expect '/contents' for root — consider special-casing empty path to avoid a trailing slash if you want exact behavior.
4	[NIT]	gitea/client.go	144	Good use of http.MethodPost and http.MethodGet rather than raw strings — this is more idiomatic and avoids typos.
5	[NIT]	gitea/client_test.go	294	Added TestEscapePath which covers multiple cases including already-encoded inputs — good coverage. Consider adding a test for an empty path to assert ListContents behavior for root if that case is important.

Recommendation

APPROVE — Approve this PR. The rename to reqURL prevents shadowing issues and the new escapePath helper correctly escapes individual path segments. The use of http.Method* is more idiomatic and safer. Consider deciding and documenting the desired behavior for empty path inputs (root) and whether double-encoding already-encoded segments is always desired; if you prefer to avoid double-encoding in some contexts, provide a separate helper or document caller responsibilities. Otherwise, this change is clean and ready to merge.

## Summary The changes correctly avoid shadowing the net/url package, consistently use http.Method* constants, and add a robust escapePath helper with accompanying tests. Code is idiomatic, well-documented, and tests/CI pass. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [NIT] | `gitea/client.go` | 56 | Renamed local URL variables to reqURL to avoid shadowing the net/url package — good change. Consider keeping this convention consistently across future additions. | | 2 | [MINOR] | `gitea/client.go` | 184 | escapePath double-encodes already-encoded segments (e.g. %20 -> %2520). The comment documents this as intentional, but this behavior can be surprising in some contexts; ensure callers never pass already-escaped segments unintentionally. | | 3 | [MINOR] | `gitea/client.go` | 205 | ListContents builds the URL with '/contents/%s' and passes escapePath(path). If path is empty (root), this yields '/contents/' (trailing slash) instead of '/contents'. Although many servers accept both, the API may expect '/contents' for root — consider special-casing empty path to avoid a trailing slash if you want exact behavior. | | 4 | [NIT] | `gitea/client.go` | 144 | Good use of http.MethodPost and http.MethodGet rather than raw strings — this is more idiomatic and avoids typos. | | 5 | [NIT] | `gitea/client_test.go` | 294 | Added TestEscapePath which covers multiple cases including already-encoded inputs — good coverage. Consider adding a test for an empty path to assert ListContents behavior for root if that case is important. | ## Recommendation **APPROVE** — Approve this PR. The rename to reqURL prevents shadowing issues and the new escapePath helper correctly escapes individual path segments. The use of http.Method* is more idiomatic and safer. Consider deciding and documenting the desired behavior for empty path inputs (root) and whether double-encoding already-encoded segments is always desired; if you prefer to avoid double-encoding in some contexts, provide a separate helper or document caller responsibilities. Otherwise, this change is clean and ready to merge.

rodin added 1 commit 2026-05-01 21:46:43 +00:00

fix: handle empty path in ListContents (root listing) ... 7b42de67ca

Empty path now yields /contents instead of /contents/ (trailing slash).
Added doc comment noting empty path = repo root.

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

sonnet-review-bot left a comment

First-time contributor

Copy Link

Copy Source

Summary

Solid improvements: path segments are now correctly escaped, URL variable shadowing is removed, and HTTP method constants are used. Changes are correct and idiomatic; tests pass and CI is green.

Findings

#	Severity	File	Line	Finding
1	[MINOR]	gitea/client.go	111	Owner and repo path segments are interpolated without escaping. While Gitea typically restricts characters, using url.PathEscape for owner and repo would make these URLs robust if names contain any special characters.
2	[MINOR]	gitea/client.go	170	Errors in doGet are returned without context (e.g., request construction error). Per repository conventions, wrap errors with context using fmt.Errorf("context: %w", err).
3	[MINOR]	gitea/client.go	176	Errors from c.http.Do(req) in doGet are returned without context. Consider wrapping, e.g., fmt.Errorf("GET %s: %w", reqURL, err).
4	[NIT]	gitea/client.go	207	ListContents now documents empty path behavior; consider adding a test to assert that ListContents(ctx, owner, repo, "") calls /contents (no trailing slash) and works as expected.
5	[NIT]	gitea/client_test.go	1	Consider adding integration-style unit tests that verify escapePath is applied in higher-level calls (e.g., GetFileContentRef and ListContents) by using file paths with spaces or '#' and asserting the server sees encoded path segments.

Recommendation

APPROVE — The refactor successfully avoids net/url shadowing, uses HTTP method constants, and correctly escapes file path segments. Behavior changes in ListContents for empty paths are sensible. I recommend merging as-is. As follow-ups, consider: (1) wrapping doGet errors with contextual messages, (2) optionally PathEscape owner/repo segments for extra robustness, and (3) adding tests to verify empty path handling in ListContents and that path escaping is exercised via the public API methods.

## Summary Solid improvements: path segments are now correctly escaped, URL variable shadowing is removed, and HTTP method constants are used. Changes are correct and idiomatic; tests pass and CI is green. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `gitea/client.go` | 111 | Owner and repo path segments are interpolated without escaping. While Gitea typically restricts characters, using url.PathEscape for owner and repo would make these URLs robust if names contain any special characters. | | 2 | [MINOR] | `gitea/client.go` | 170 | Errors in doGet are returned without context (e.g., request construction error). Per repository conventions, wrap errors with context using fmt.Errorf("context: %w", err). | | 3 | [MINOR] | `gitea/client.go` | 176 | Errors from c.http.Do(req) in doGet are returned without context. Consider wrapping, e.g., fmt.Errorf("GET %s: %w", reqURL, err). | | 4 | [NIT] | `gitea/client.go` | 207 | ListContents now documents empty path behavior; consider adding a test to assert that ListContents(ctx, owner, repo, "") calls /contents (no trailing slash) and works as expected. | | 5 | [NIT] | `gitea/client_test.go` | 1 | Consider adding integration-style unit tests that verify escapePath is applied in higher-level calls (e.g., GetFileContentRef and ListContents) by using file paths with spaces or '#' and asserting the server sees encoded path segments. | ## Recommendation **APPROVE** — The refactor successfully avoids net/url shadowing, uses HTTP method constants, and correctly escapes file path segments. Behavior changes in ListContents for empty paths are sensible. I recommend merging as-is. As follow-ups, consider: (1) wrapping doGet errors with contextual messages, (2) optionally PathEscape owner/repo segments for extra robustness, and (3) adding tests to verify empty path handling in ListContents and that path escaping is exercised via the public API methods.

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

gpt-review-bot left a comment

First-time contributor

Copy Link

Copy Source

Summary

The changes correctly avoid shadowing the net/url package, introduce a safe escapePath helper to percent-encode individual path segments, and consistently use http.MethodGet/POST. Tests were added for escapePath and CI passed.

Findings

#	Severity	File	Line	Finding
1	[MINOR]	gitea/client.go	184	The escapePath helper will double-encode already-encoded segments (e.g. "file%20name" -> "file%2520name"). The comment documents this as desired behavior, but callers must ensure they do not pre-encode paths if that is not intended.
2	[MINOR]	gitea/client.go	118	GetFileContentRef uses url.QueryEscape for the ref query parameter. QueryEscape encodes spaces as '+' (form-style), while PathEscape would percent-encode as '%20'. This is correct for query values but be mindful of subtle differences if the server expects percent-encoding.
3	[NIT]	gitea/client.go	56	Renaming the local URL variables to reqURL to avoid shadowing net/url is a good change for clarity. No action required.
4	[NIT]	gitea/client_test.go	294	TestEscapePath was added and covers a good set of cases. Consider adding a test for a leading slash input if the function should explicitly reject or handle leading slashes.

Recommendation

APPROVE — Approve these changes. They fix the url package shadowing and add robust per-segment path escaping with accompanying tests. The escapePath behavior (double-encoding) is documented — ensure callers do not pre-encode paths if that would be undesired. Optionally, add a test for leading-slash inputs to clarify behavior for those cases, and verify the server's expectations for query encoding (QueryEscape vs percent-encoding) for the ref parameter if interoperability issues arise.

## Summary The changes correctly avoid shadowing the net/url package, introduce a safe escapePath helper to percent-encode individual path segments, and consistently use http.MethodGet/POST. Tests were added for escapePath and CI passed. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `gitea/client.go` | 184 | The escapePath helper will double-encode already-encoded segments (e.g. "file%20name" -> "file%2520name"). The comment documents this as desired behavior, but callers must ensure they do not pre-encode paths if that is not intended. | | 2 | [MINOR] | `gitea/client.go` | 118 | GetFileContentRef uses url.QueryEscape for the ref query parameter. QueryEscape encodes spaces as '+' (form-style), while PathEscape would percent-encode as '%20'. This is correct for query values but be mindful of subtle differences if the server expects percent-encoding. | | 3 | [NIT] | `gitea/client.go` | 56 | Renaming the local URL variables to reqURL to avoid shadowing net/url is a good change for clarity. No action required. | | 4 | [NIT] | `gitea/client_test.go` | 294 | TestEscapePath was added and covers a good set of cases. Consider adding a test for a leading slash input if the function should explicitly reject or handle leading slashes. | ## Recommendation **APPROVE** — Approve these changes. They fix the url package shadowing and add robust per-segment path escaping with accompanying tests. The escapePath behavior (double-encoding) is documented — ensure callers do not pre-encode paths if that would be undesired. Optionally, add a test for leading-slash inputs to clarify behavior for those cases, and verify the server's expectations for query encoding (QueryEscape vs percent-encoding) for the ref parameter if interoperability issues arise.

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

docs: add package-level documentation ... aade891129

Per go-patterns/package-design.md, every package needs a doc comment.
Added to gitea, llm, and review packages.

rodin merged commit ef3e6d5e87 into main 2026-05-01 21:55:02 +00:00

rodin referenced this issue from a commit 2026-05-01 21:55:03 +00:00

Merge pull request 'fix: path-escape file paths and eliminate url package shadowing' (#17) from fix/url-escaping-and-shadow into main

rodin referenced this pull request 2026-05-02 04:02:36 +00:00

Consistent url.PathEscape across all Gitea client endpoints #24

rodin referenced this issue from a commit 2026-05-02 09:41:57 +00:00

fix: consistent url.PathEscape across all Gitea client endpoints

rodin referenced this pull request 2026-05-02 09:42:04 +00:00

fix: consistent url.PathEscape across all Gitea client endpoints #30

Sign in to join this conversation.

Reviewers

No Reviewers

sonnet-review-bot

gpt-review-bot

Labels

Clear labels

ai-review

bug

needs-review

ready

Ready for human review

self-reviewed

Self-review passed since last push

wip

Work in progress - do not start new work on this PR

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

security-review-bot rodin (Rodin)

No Assignees

3 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: rodin/review-bot#17