feat: add context.Context + unexport client fields #14

Merged

rodin merged 8 commits from fix/context-and-encapsulation into main 2026-05-01 21:10:37 +00:00

Conversation 16 Commits 8 Files Changed 7

+184 -110

rodin commented 2026-05-01 19:40:52 +00:00

Owner

Copy Link

Copy Source

Addresses REVIEW.md findings 1-4, 14:

• All Gitea client methods now accept context.Context as first param
• All LLM client methods now accept context.Context as first param
• Use http.NewRequestWithContext for cancellation/timeout support
• Main uses 3-minute timeout context for all operations
• Unexport Client struct fields (baseURL, token, apiKey, etc.)
• Use bytes.NewReader instead of strings.NewReader(string(...))

Findings Addressed

#	Severity	Finding
1	MAJOR	No context.Context on Gitea client methods
2	MAJOR	No context.Context on LLM Complete()
3	MAJOR	Gitea Client fields exported
4	MAJOR	LLM Client fields exported
14	NIT	Unnecessary string conversion in PostReview

Addresses REVIEW.md findings 1-4, 14: - All Gitea client methods now accept `context.Context` as first param - All LLM client methods now accept `context.Context` as first param - Use `http.NewRequestWithContext` for cancellation/timeout support - Main uses 3-minute timeout context for all operations - Unexport Client struct fields (`baseURL`, `token`, `apiKey`, etc.) - Use `bytes.NewReader` instead of `strings.NewReader(string(...))` ## Findings Addressed | # | Severity | Finding | |---|----------|--------| | 1 | MAJOR | No context.Context on Gitea client methods | | 2 | MAJOR | No context.Context on LLM Complete() | | 3 | MAJOR | Gitea Client fields exported | | 4 | MAJOR | LLM Client fields exported | | 14 | NIT | Unnecessary string conversion in PostReview |

rodin added 1 commit 2026-05-01 19:40:52 +00:00

feat: add context.Context + unexport client fields ... 27e0056f29

REVIEW.md findings 1-4, 14:
- All Gitea client methods now accept context.Context as first param
- All LLM client methods now accept context.Context as first param
- Use http.NewRequestWithContext for cancellation/timeout support
- Main uses 3-minute timeout context for all operations
- Unexport Client struct fields (baseURL, token, apiKey, etc.)
- Use bytes.NewReader instead of strings.NewReader(string(...))

sonnet-review-bot approved these changes 2026-05-01 19:42:03 +00:00

Dismissed

sonnet-review-bot left a comment

First-time contributor

Copy Link

Copy Source

Summary

Well-scoped, idiomatic changes adding context support across clients and unexporting client fields. HTTP requests now respect cancellation/timeouts, and tests were updated accordingly. CI passed.

Recommendation

APPROVE — The changes correctly introduce context.Context through the Gitea and LLM clients, use http.NewRequestWithContext, unexport client fields, and replace strings.NewReader(string(data)) with bytes.NewReader(data). main now uses a 3-minute timeout context, and tests were updated to pass contexts. Everything aligns with the repository conventions and error-handling practices, and CI is green. As a future enhancement (not required for this PR), consider allowing injection of an http.Client or setting a default timeout on the client to provide sane defaults in contexts where no timeout is provided (e.g., integration tests). Overall, this is a solid improvement—approved.

## Summary Well-scoped, idiomatic changes adding context support across clients and unexporting client fields. HTTP requests now respect cancellation/timeouts, and tests were updated accordingly. CI passed. ## Recommendation **APPROVE** — The changes correctly introduce context.Context through the Gitea and LLM clients, use http.NewRequestWithContext, unexport client fields, and replace strings.NewReader(string(data)) with bytes.NewReader(data). main now uses a 3-minute timeout context, and tests were updated to pass contexts. Everything aligns with the repository conventions and error-handling practices, and CI is green. As a future enhancement (not required for this PR), consider allowing injection of an http.Client or setting a default timeout on the client to provide sane defaults in contexts where no timeout is provided (e.g., integration tests). Overall, this is a solid improvement—approved.

gpt-review-bot approved these changes 2026-05-01 19:42:30 +00:00

Dismissed

gpt-review-bot left a comment

First-time contributor

Copy Link

Copy Source

Summary

Good changes overall — context.Context propagation, request cancellation support, unexporting client fields, and switching to bytes.NewReader are implemented consistently across the codebase. Tests and integration test updates follow the API changes and CI passed.

Findings

#	Severity	File	Line	Finding
1	[MINOR]	gitea/client.go	140	GetAllFilesInPath swallows errors in a few places (e.g. skipping files/directories on error) without logging. This is likely intentional to be resilient, but consider logging debug-level information about skipped entries to aid debugging when files are unexpectedly missing or fail to load.
2	[NIT]	integration_test.go	45	Owner/repo parsing uses a manual rune loop to find '/' and slice strings. Prefer strings.SplitN(giteaRepo, "/", 2) for clarity and correctness (handles edge cases and is more idiomatic).
3	[NIT]	llm/client.go	70	ChatRequest includes Temperature with omitempty, which is correct to omit zero. Tests rely on JSON decoding to a map and comparing the raw value — that's fine, but be cautious: if the encoder/decoder behavior changes this test may become brittle. Consider adding an explicit unit test that marshals ChatRequest and asserts presence/absence of the temperature key directly.

Recommendation

APPROVE — Approve these changes. They correctly add context.Context to public methods, use http.NewRequestWithContext for cancellation, unexport client fields to encapsulate implementation details, and replace unnecessary string conversions with bytes.NewReader. The modifications to tests and integration test are consistent. Consider the minor improvements suggested: add optional debug logging when files are skipped in recursive fetching, simplify owner/repo parsing in the integration test to use strings.SplitN, and add an explicit marshal test for temperature inclusion/omission to make the behavior more robust against future changes.

## Summary Good changes overall — context.Context propagation, request cancellation support, unexporting client fields, and switching to bytes.NewReader are implemented consistently across the codebase. Tests and integration test updates follow the API changes and CI passed. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `gitea/client.go` | 140 | GetAllFilesInPath swallows errors in a few places (e.g. skipping files/directories on error) without logging. This is likely intentional to be resilient, but consider logging debug-level information about skipped entries to aid debugging when files are unexpectedly missing or fail to load. | | 2 | [NIT] | `integration_test.go` | 45 | Owner/repo parsing uses a manual rune loop to find '/' and slice strings. Prefer strings.SplitN(giteaRepo, "/", 2) for clarity and correctness (handles edge cases and is more idiomatic). | | 3 | [NIT] | `llm/client.go` | 70 | ChatRequest includes Temperature with `omitempty`, which is correct to omit zero. Tests rely on JSON decoding to a map and comparing the raw value — that's fine, but be cautious: if the encoder/decoder behavior changes this test may become brittle. Consider adding an explicit unit test that marshals ChatRequest and asserts presence/absence of the temperature key directly. | ## Recommendation **APPROVE** — Approve these changes. They correctly add context.Context to public methods, use http.NewRequestWithContext for cancellation, unexport client fields to encapsulate implementation details, and replace unnecessary string conversions with bytes.NewReader. The modifications to tests and integration test are consistent. Consider the minor improvements suggested: add optional debug logging when files are skipped in recursive fetching, simplify owner/repo parsing in the integration test to use strings.SplitN, and add an explicit marshal test for temperature inclusion/omission to make the behavior more robust against future changes.

rodin added 1 commit 2026-05-01 19:45:54 +00:00

fix: log warnings instead of swallowing errors ... ecebd52371

- GetAllFilesInPath: log.Printf when file fetch or dir recursion fails
- integration_test: use strings.SplitN for owner/repo parsing (idiomatic)

Addresses GPT review findings #1, #2.

sonnet-review-bot approved these changes 2026-05-01 19:47:36 +00:00

Dismissed

sonnet-review-bot left a comment

First-time contributor

Copy Link

Copy Source

Summary

Solid refactor adding context propagation throughout HTTP calls and unexporting client fields. Tests updated accordingly and CI passed. Only small readability and integration-test parity nits remain.

Findings

#	Severity	File	Line	Finding
1	[NIT]	gitea/client.go	229	Minor indentation/readability nit: the continue inside the if err != nil block in GetAllFilesInPath is misaligned. Consider gofmt-ing to keep indentation consistent.
2	[MINOR]	integration_test.go	62	Integration test still uses older prompt builder signatures (e.g., BuildSystemPrompt with a single argument and BuildUserPrompt without file context). While behind an integration build tag and not affecting CI, updating the test to mirror current prompt builders would prevent future breakage when running integration tests.

Recommendation

APPROVE — The changes correctly introduce context-aware HTTP requests across the Gitea and LLM clients, unexport internal client fields, and replace unnecessary string conversions with bytes.NewReader. Main now scopes all operations under a sensible timeout. Unit tests have been updated to pass contexts and verify behavior, and CI is green. Please consider tidying the minor indentation nit in gitea/client.go and updating the integration test prompt calls to reflect the current prompt builder signatures to avoid issues when running with the integration build tag. Otherwise, this is good to merge.

## Summary Solid refactor adding context propagation throughout HTTP calls and unexporting client fields. Tests updated accordingly and CI passed. Only small readability and integration-test parity nits remain. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [NIT] | `gitea/client.go` | 229 | Minor indentation/readability nit: the `continue` inside the `if err != nil` block in GetAllFilesInPath is misaligned. Consider gofmt-ing to keep indentation consistent. | | 2 | [MINOR] | `integration_test.go` | 62 | Integration test still uses older prompt builder signatures (e.g., BuildSystemPrompt with a single argument and BuildUserPrompt without file context). While behind an integration build tag and not affecting CI, updating the test to mirror current prompt builders would prevent future breakage when running integration tests. | ## Recommendation **APPROVE** — The changes correctly introduce context-aware HTTP requests across the Gitea and LLM clients, unexport internal client fields, and replace unnecessary string conversions with bytes.NewReader. Main now scopes all operations under a sensible timeout. Unit tests have been updated to pass contexts and verify behavior, and CI is green. Please consider tidying the minor indentation nit in gitea/client.go and updating the integration test prompt calls to reflect the current prompt builder signatures to avoid issues when running with the integration build tag. Otherwise, this is good to merge.

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

Dismissed

gpt-review-bot left a comment

First-time contributor

Copy Link

Copy Source

Summary

The PR consistently adds context.Context to client APIs, uses request-level cancellation via http.NewRequestWithContext, unexports client internals, and replaces unnecessary string conversions. Tests and integration tests were updated accordingly and CI passed.

Findings

#	Severity	File	Line	Finding
1	[MINOR]	gitea/client.go	18	NewClient initializes http.Client with no Timeout. While request-level contexts are now used (good), consider also setting a sensible client-side Timeout (or documenting why it's omitted) to guard against transports that may not fully respect contexts in all edge cases.
2	[MINOR]	gitea/client.go	223	In GetAllFilesInPath the error handling for fetching file contents was changed to log a warning then continue; the 'continue' line in the diff appears mis-indented. While gofmt will likely normalize this, ensure the if-block and indentation are correct (log then continue) to avoid accidental logic/formatting issues.
3	[NIT]	llm/client.go	12	Client struct fields were unexported (good). Consider adding a small comment on the Client type describing thread-safety expectations (e.g., it is safe to reuse the Client across goroutines provided you don't mutate fields after use).
4	[NIT]	cmd/review-bot/main.go	66	A top-level 3-minute context timeout is introduced (good). Be aware this timeout applies to the entire run; if any specific operation could reasonably take longer, consider scoping per-operation timeouts or documenting the decision.

Recommendation

APPROVE — Approve: the changes are correct and idiomatic. The main improvements (adding context, request-level cancellation, unexporting fields, and removing unnecessary string conversions) are properly implemented and tests were updated. Address the minor suggestions: consider documenting client reuse/thread-safety, optionally set a default http.Client timeout or document why it's omitted, and verify formatting/indentation around the added log + continue in GetAllFilesInPath. Other than these small improvements, the PR is ready to merge.

## Summary The PR consistently adds context.Context to client APIs, uses request-level cancellation via http.NewRequestWithContext, unexports client internals, and replaces unnecessary string conversions. Tests and integration tests were updated accordingly and CI passed. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `gitea/client.go` | 18 | NewClient initializes http.Client with no Timeout. While request-level contexts are now used (good), consider also setting a sensible client-side Timeout (or documenting why it's omitted) to guard against transports that may not fully respect contexts in all edge cases. | | 2 | [MINOR] | `gitea/client.go` | 223 | In GetAllFilesInPath the error handling for fetching file contents was changed to log a warning then continue; the 'continue' line in the diff appears mis-indented. While gofmt will likely normalize this, ensure the if-block and indentation are correct (log then continue) to avoid accidental logic/formatting issues. | | 3 | [NIT] | `llm/client.go` | 12 | Client struct fields were unexported (good). Consider adding a small comment on the Client type describing thread-safety expectations (e.g., it is safe to reuse the Client across goroutines provided you don't mutate fields after use). | | 4 | [NIT] | `cmd/review-bot/main.go` | 66 | A top-level 3-minute context timeout is introduced (good). Be aware this timeout applies to the entire run; if any specific operation could reasonably take longer, consider scoping per-operation timeouts or documenting the decision. | ## Recommendation **APPROVE** — Approve: the changes are correct and idiomatic. The main improvements (adding context, request-level cancellation, unexporting fields, and removing unnecessary string conversions) are properly implemented and tests were updated. Address the minor suggestions: consider documenting client reuse/thread-safety, optionally set a default http.Client timeout or document why it's omitted, and verify formatting/indentation around the added log + continue in GetAllFilesInPath. Other than these small improvements, the PR is ready to merge.

rodin referenced this issue from a commit 2026-05-01 19:56:09 +00:00

fix: address all review findings on PR #14

rodin added 1 commit 2026-05-01 19:56:09 +00:00

fix: address all review findings on PR #14 ... cedb5e7b90

- gitea.Client: add concurrency safety doc comment
- gitea.Client: set 30s HTTP client timeout as safety net
- llm.Client: add concurrency safety doc comment
- llm.Client: set 2min HTTP client timeout (LLM calls are slow)
- gitea/client.go: gofmt to fix indentation
- integration_test: update to current BuildSystemPrompt/BuildUserPrompt signatures
- integration_test: use strings.SplitN for owner/repo parsing

sonnet-review-bot approved these changes 2026-05-01 19:57:11 +00:00

Dismissed

sonnet-review-bot left a comment

First-time contributor

Copy Link

Copy Source

Summary

Solid update adding context.Context to all client methods, unexporting client fields, and improving HTTP request construction and timeouts. Tests and integration usage are updated consistently, and CI has passed.

Recommendation

APPROVE — The changes are correct and idiomatic: context propagation is implemented properly, http.NewRequestWithContext is used, client fields are unexported, and the unnecessary string conversion is removed. HTTP client timeouts were added, and tests were updated accordingly. No issues found; proceed with merge.

## Summary Solid update adding context.Context to all client methods, unexporting client fields, and improving HTTP request construction and timeouts. Tests and integration usage are updated consistently, and CI has passed. ## Recommendation **APPROVE** — The changes are correct and idiomatic: context propagation is implemented properly, http.NewRequestWithContext is used, client fields are unexported, and the unnecessary string conversion is removed. HTTP client timeouts were added, and tests were updated accordingly. No issues found; proceed with merge.

rodin added 1 commit 2026-05-01 20:00:38 +00:00

fix: increase LLM client timeout to 5 minutes ... 401e94d3e4

GPT-5-mini timed out on larger diffs (2min was too short).
LLM calls for code review with full file context can take 2-4min.

sonnet-review-bot approved these changes 2026-05-01 20:02:06 +00:00

Dismissed

sonnet-review-bot left a comment

First-time contributor

Copy Link

Copy Source

Summary

Solid, idiomatic changes adding context.Context to all client methods, unexporting client fields, and improving HTTP request handling. Tests and integration test were updated accordingly, and CI has passed.

Recommendation

APPROVE — The changes are correct and align with Go best practices and the repository conventions: context propagation, using http.NewRequestWithContext, unexported client internals, and better request body handling. The updates to tests and integration flow look consistent and comprehensive. With CI passing and no functional or design issues observed, this is good to merge.

## Summary Solid, idiomatic changes adding context.Context to all client methods, unexporting client fields, and improving HTTP request handling. Tests and integration test were updated accordingly, and CI has passed. ## Recommendation **APPROVE** — The changes are correct and align with Go best practices and the repository conventions: context propagation, using http.NewRequestWithContext, unexported client internals, and better request body handling. The updates to tests and integration flow look consistent and comprehensive. With CI passing and no functional or design issues observed, this is good to merge.

gpt-review-bot approved these changes 2026-05-01 20:02:36 +00:00

Dismissed

gpt-review-bot left a comment

First-time contributor

Copy Link

Copy Source

Summary

Good changes: context.Context propagated throughout client APIs, HTTP requests use NewRequestWithContext, client fields are unexported, and tests updated accordingly. The modifications are correct and improve cancellation/timeout behavior; CI passed.

Findings

#	Severity	File	Line	Finding
1	[MINOR]	llm/client.go	15	The file comment states the Client is safe for concurrent use after construction, but WithTemperature mutates the client's temperature field and returns the same client. If WithTemperature is called concurrently after the client is in use it could introduce a data race. Either document that WithTemperature must be called only during setup, make it return a copy, or make the field immutable after construction.
2	[NIT]	cmd/review-bot/main.go	66	You create the top-level context with a 3-minute timeout which is good. Small nit: WithTemperature is called before the context is constructed; this is harmless but consider constructing the top-level context earlier if you prefer establishing request lifecycle at program start.

Recommendation

APPROVE — Approve. The changes are correct and idiomatic: contexts are added to public client methods, HTTP requests use NewRequestWithContext, clients have sensible per-client timeouts, fields are now unexported, and tests updated. Consider documenting that WithTemperature must be called during client setup (or change it to return an immutable copy) to avoid potential data races in concurrent scenarios. The other nits are optional stylistic suggestions.

## Summary Good changes: context.Context propagated throughout client APIs, HTTP requests use NewRequestWithContext, client fields are unexported, and tests updated accordingly. The modifications are correct and improve cancellation/timeout behavior; CI passed. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `llm/client.go` | 15 | The file comment states the Client is safe for concurrent use after construction, but WithTemperature mutates the client's temperature field and returns the same client. If WithTemperature is called concurrently after the client is in use it could introduce a data race. Either document that WithTemperature must be called only during setup, make it return a copy, or make the field immutable after construction. | | 2 | [NIT] | `cmd/review-bot/main.go` | 66 | You create the top-level context with a 3-minute timeout which is good. Small nit: WithTemperature is called before the context is constructed; this is harmless but consider constructing the top-level context earlier if you prefer establishing request lifecycle at program start. | ## Recommendation **APPROVE** — Approve. The changes are correct and idiomatic: contexts are added to public client methods, HTTP requests use NewRequestWithContext, clients have sensible per-client timeouts, fields are now unexported, and tests updated. Consider documenting that WithTemperature must be called during client setup (or change it to return an immutable copy) to avoid potential data races in concurrent scenarios. The other nits are optional stylistic suggestions.

rodin added 1 commit 2026-05-01 20:04:01 +00:00

feat: make LLM timeout configurable (default 5min) ... 1da61e514d

New flag: --llm-timeout / LLM_TIMEOUT (seconds, default 300)
New builder: llmClient.WithTimeout(duration)
Composite action: new timeout input

Keeps 5 minutes as the sensible default but allows tuning for
larger repos or slower models.

sonnet-review-bot requested changes 2026-05-01 20:05:23 +00:00

Dismissed

sonnet-review-bot left a comment

First-time contributor

Copy Link

Copy Source

Summary

The Go changes are solid: context propagation is correctly added throughout, client fields are unexported, HTTP requests use context-aware constructors, and tests are updated accordingly. However, the composite action YAML was corrupted with duplicated and misplaced inputs, which will likely break the action metadata.

Findings

#	Severity	File	Line	Finding
1	[MAJOR]	.gitea/actions/review/action.yml	46	Inputs section is malformed: multiple duplicate 'timeout' inputs were inserted, the 'temperature' input's description was displaced, and there are repeated/contradictory fields (e.g., multiple 'required' and 'default' entries). This corrupts the composite action metadata and may cause the action to fail or behave unpredictably. Define each input once (temperature with default 0; timeout with default 300) and avoid duplicate keys.

Recommendation

REQUEST_CHANGES — Overall the Go implementation and tests look correct and idiomatic: context.Context is threaded through all HTTP calls, clients use http.NewRequestWithContext, unexported fields improve encapsulation, and tests were updated to match the new signatures. However, please fix the composite action YAML before merging. The inputs block must declare temperature and timeout exactly once each, with proper descriptions/defaults, and without duplicate keys. Also ensure the environment mapping (LLM_TIMEOUT: ${{ inputs.timeout }}) aligns with the corrected inputs. Once the action.yml is corrected, this can be approved.

## Summary The Go changes are solid: context propagation is correctly added throughout, client fields are unexported, HTTP requests use context-aware constructors, and tests are updated accordingly. However, the composite action YAML was corrupted with duplicated and misplaced inputs, which will likely break the action metadata. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MAJOR] | `.gitea/actions/review/action.yml` | 46 | Inputs section is malformed: multiple duplicate 'timeout' inputs were inserted, the 'temperature' input's description was displaced, and there are repeated/contradictory fields (e.g., multiple 'required' and 'default' entries). This corrupts the composite action metadata and may cause the action to fail or behave unpredictably. Define each input once (temperature with default 0; timeout with default 300) and avoid duplicate keys. | ## Recommendation **REQUEST_CHANGES** — Overall the Go implementation and tests look correct and idiomatic: context.Context is threaded through all HTTP calls, clients use http.NewRequestWithContext, unexported fields improve encapsulation, and tests were updated to match the new signatures. However, please fix the composite action YAML before merging. The inputs block must declare temperature and timeout exactly once each, with proper descriptions/defaults, and without duplicate keys. Also ensure the environment mapping (LLM_TIMEOUT: ${{ inputs.timeout }}) aligns with the corrected inputs. Once the action.yml is corrected, this can be approved.

gpt-review-bot requested changes 2026-05-01 20:05:32 +00:00

Dismissed

gpt-review-bot left a comment

First-time contributor

Copy Link

Copy Source

Summary

Overall the changes correctly add context.Context propagation, unexport client fields, proper use of http.NewRequestWithContext, and bytes.NewReader usage. However, the .gitea/actions/review/action.yml was corrupted with many duplicated/incorrect keys which will likely break the action definition and should be fixed before merge. A few minor cleanups / clarifications are also recommended.

Findings

#	Severity	File	Line	Finding
1	[MAJOR]	.gitea/actions/review/action.yml	46	The action.yml changes introduce many duplicated and inconsistent 'timeout' / 'temperature' input keys and repeated 'description'/'required'/'default' entries. This results in invalid or ambiguous YAML (duplicate keys) and will break the composite action. Please clean up and produce a single, consistent set of inputs (e.g. one 'temperature' and one 'timeout' input).
2	[MINOR]	gitea/client.go	1	Good: client methods now accept context.Context and http.Client timeouts are set. One minor note: doGet and other methods now return errors that include the original HTTP error text; consider wrapping with additional context where helpful (you already do this in many places).
3	[MINOR]	llm/client.go	66	Good: Complete now accepts a context and uses NewRequestWithContext; WithTimeout was added to tune HTTP client timeout. Consider returning the client from WithTimeout (it already does) — tests and callers ignore the returned value which is fine, but call-sites could chain .WithTimeout().WithTemperature().
4	[NIT]	gitea/client.go	220	GetAllFilesInPath now logs a warning for files that can't be read instead of silently skipping them. This is fine, but consider keeping error behavior consistent across the package (e.g. include which call failed when recursing directories).
5	[NIT]	cmd/review-bot/main.go	32	Good: a top-level 3-minute context was added and passed to all client calls for cancellation. Consider documenting in README/usage that the tool enforces a 3-minute overall timeout and that LLM timeout is configurable separately.

Recommendation

REQUEST_CHANGES — Fix the .gitea/actions/review/action.yml to remove duplicate/contradictory input definitions and ensure each input key appears only once (e.g. a single 'temperature' and a single 'timeout' with correct default and description). After cleaning the YAML, re-run CI to confirm no other issues. The Go changes are largely correct and idiomatic — keep the context propagation and unexported fields. Consider minor polish on logging messages and ensure callers consistently handle/chain WithTimeout/WithTemperature if desired.

## Summary Overall the changes correctly add context.Context propagation, unexport client fields, proper use of http.NewRequestWithContext, and bytes.NewReader usage. However, the .gitea/actions/review/action.yml was corrupted with many duplicated/incorrect keys which will likely break the action definition and should be fixed before merge. A few minor cleanups / clarifications are also recommended. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MAJOR] | `.gitea/actions/review/action.yml` | 46 | The action.yml changes introduce many duplicated and inconsistent 'timeout' / 'temperature' input keys and repeated 'description'/'required'/'default' entries. This results in invalid or ambiguous YAML (duplicate keys) and will break the composite action. Please clean up and produce a single, consistent set of inputs (e.g. one 'temperature' and one 'timeout' input). | | 2 | [MINOR] | `gitea/client.go` | 1 | Good: client methods now accept context.Context and http.Client timeouts are set. One minor note: doGet and other methods now return errors that include the original HTTP error text; consider wrapping with additional context where helpful (you already do this in many places). | | 3 | [MINOR] | `llm/client.go` | 66 | Good: Complete now accepts a context and uses NewRequestWithContext; WithTimeout was added to tune HTTP client timeout. Consider returning the client from WithTimeout (it already does) — tests and callers ignore the returned value which is fine, but call-sites could chain .WithTimeout().WithTemperature(). | | 4 | [NIT] | `gitea/client.go` | 220 | GetAllFilesInPath now logs a warning for files that can't be read instead of silently skipping them. This is fine, but consider keeping error behavior consistent across the package (e.g. include which call failed when recursing directories). | | 5 | [NIT] | `cmd/review-bot/main.go` | 32 | Good: a top-level 3-minute context was added and passed to all client calls for cancellation. Consider documenting in README/usage that the tool enforces a 3-minute overall timeout and that LLM timeout is configurable separately. | ## Recommendation **REQUEST_CHANGES** — Fix the .gitea/actions/review/action.yml to remove duplicate/contradictory input definitions and ensure each input key appears only once (e.g. a single 'temperature' and a single 'timeout' with correct default and description). After cleaning the YAML, re-run CI to confirm no other issues. The Go changes are largely correct and idiomatic — keep the context propagation and unexported fields. Consider minor polish on logging messages and ensure callers consistently handle/chain WithTimeout/WithTemperature if desired.

rodin added 1 commit 2026-05-01 20:08:20 +00:00

fix: rewrite action.yml (was corrupted with duplicate keys) ... 43041a00f5

Clean single definition of all inputs: temperature, timeout,
patterns-repo, patterns-files. Also added runner requirements
comment at the top.

sonnet-review-bot approved these changes 2026-05-01 20:10:07 +00:00

Dismissed

sonnet-review-bot left a comment

First-time contributor

Copy Link

Copy Source

Summary

Solid refactor adding context support across clients, unexporting client fields, introducing sane HTTP timeouts, and fixing minor inefficiencies. Tests and integration code are updated accordingly and CI passed.

Findings

#	Severity	File	Line	Finding
1	[MINOR]	llm/client.go	30	The doc comments for WithTemperature/WithTimeout are misaligned: the comment block above WithTimeout mentions WithTemperature, and WithTemperature lacks its own doc comment. This can be confusing and is non-idiomatic.
2	[NIT]	gitea/client.go	215	In GetAllFilesInPath, when both listing and direct file fetch fail, the returned error wraps the directory listing error (err) rather than the direct file fetch error (fileErr). Returning or combining both errors could provide clearer diagnostics.

Recommendation

APPROVE — The changes are well-executed: context is threaded through all network operations, HTTP requests use NewRequestWithContext for proper cancellation, client fields are unexported, and the action and main package expose a configurable LLM timeout. Tests are updated to use context and still validate behavior, and CI passed. Consider aligning the doc comments in llm/client.go so each exported method has an accurate, self-referential comment, and optionally improve the error reporting in GetAllFilesInPath to surface the most relevant underlying error. Otherwise, this is ready to merge.

## Summary Solid refactor adding context support across clients, unexporting client fields, introducing sane HTTP timeouts, and fixing minor inefficiencies. Tests and integration code are updated accordingly and CI passed. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `llm/client.go` | 30 | The doc comments for WithTemperature/WithTimeout are misaligned: the comment block above WithTimeout mentions WithTemperature, and WithTemperature lacks its own doc comment. This can be confusing and is non-idiomatic. | | 2 | [NIT] | `gitea/client.go` | 215 | In GetAllFilesInPath, when both listing and direct file fetch fail, the returned error wraps the directory listing error (err) rather than the direct file fetch error (fileErr). Returning or combining both errors could provide clearer diagnostics. | ## Recommendation **APPROVE** — The changes are well-executed: context is threaded through all network operations, HTTP requests use NewRequestWithContext for proper cancellation, client fields are unexported, and the action and main package expose a configurable LLM timeout. Tests are updated to use context and still validate behavior, and CI passed. Consider aligning the doc comments in llm/client.go so each exported method has an accurate, self-referential comment, and optionally improve the error reporting in GetAllFilesInPath to surface the most relevant underlying error. Otherwise, this is ready to merge.

gpt-review-bot approved these changes 2026-05-01 20:10:07 +00:00

Dismissed

gpt-review-bot left a comment

First-time contributor

Copy Link

Copy Source

Summary

Changes correctly add context.Context propagation, use context-aware HTTP requests, unexport client internals, and replace unnecessary string conversions. Tests and integration tests were updated accordingly and CI passed.

Findings

#	Severity	File	Line	Finding
1	[MINOR]	gitea/client.go	20	Unexporting Client fields (baseURL, token, http) is a reasonable encapsulation improvement, but it is a breaking API change for any external callers that previously accessed these fields directly. Consider calling this out in a changelog or release notes so downstream consumers can adapt.
2	[MINOR]	llm/client.go	16	New WithTimeout(d time.Duration) method sets the HTTP client's Timeout which is useful. There are no unit tests validating WithTimeout behavior (e.g. that the underlying http.Client timeout is applied). Consider adding a test to assert WithTimeout modifies the client's http timeout value and/or that a short timeout causes requests to fail as expected.
3	[NIT]	gitea/client.go	225	In GetAllFilesInPath, when recursion into a subdirectory returns an error the code silently continues. For visibility it would be helpful to log a warning similar to file fetch failures so it's easier to debug missing pattern files in callers.

Recommendation

APPROVE — Approve these changes. They improve context propagation and cancellation support and tidy up client encapsulation. Before merging, document the breaking change of unexported client fields in release notes. Optionally add a unit test for llm.WithTimeout and add a small warning log when directory recursion in GetAllFilesInPath fails to aid debugging.

## Summary Changes correctly add context.Context propagation, use context-aware HTTP requests, unexport client internals, and replace unnecessary string conversions. Tests and integration tests were updated accordingly and CI passed. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `gitea/client.go` | 20 | Unexporting Client fields (baseURL, token, http) is a reasonable encapsulation improvement, but it is a breaking API change for any external callers that previously accessed these fields directly. Consider calling this out in a changelog or release notes so downstream consumers can adapt. | | 2 | [MINOR] | `llm/client.go` | 16 | New WithTimeout(d time.Duration) method sets the HTTP client's Timeout which is useful. There are no unit tests validating WithTimeout behavior (e.g. that the underlying http.Client timeout is applied). Consider adding a test to assert WithTimeout modifies the client's http timeout value and/or that a short timeout causes requests to fail as expected. | | 3 | [NIT] | `gitea/client.go` | 225 | In GetAllFilesInPath, when recursion into a subdirectory returns an error the code silently continues. For visibility it would be helpful to log a warning similar to file fetch failures so it's easier to debug missing pattern files in callers. | ## Recommendation **APPROVE** — Approve these changes. They improve context propagation and cancellation support and tidy up client encapsulation. Before merging, document the breaking change of unexported client fields in release notes. Optionally add a unit test for llm.WithTimeout and add a small warning log when directory recursion in GetAllFilesInPath fails to aid debugging.

rodin referenced this issue from a commit 2026-05-01 20:17:41 +00:00

fix: address all remaining review findings on PR #14

rodin added 1 commit 2026-05-01 20:17:42 +00:00

fix: address all remaining review findings on PR #14 ... 0cca44b65a

- Fix doc comments: WithTimeout and WithTemperature each get their own
- Add TestWithTimeout (verifies short timeout causes request failure)
- Log warning on directory recursion failure in GetAllFilesInPath
- Note: unexported fields is a breaking change, will document in release notes

sonnet-review-bot approved these changes 2026-05-01 20:19:26 +00:00

Dismissed

sonnet-review-bot left a comment

First-time contributor

Copy Link

Copy Source

Summary

Solid refactor adding context support, unexporting client fields, and improving HTTP handling. Changes are idiomatic, tests were updated accordingly, and CI passed.

Findings

#	Severity	File	Line	Finding
1	[MINOR]	cmd/review-bot/main.go	74	The fixed 3-minute top-level context timeout may conflict with or override a larger user-specified LLM timeout. Consider making the overall timeout configurable or deriving it from the longest operation to avoid unexpected cancellations.
2	[MINOR]	llm/client.go	15	The comment claims the client is safe for concurrent use after construction, but WithTimeout and WithTemperature mutate internal state. Consider clarifying in documentation that these should be called during setup (before concurrent use), or guard mutations to avoid data races if used concurrently.
3	[NIT]	gitea/client.go	230	GetAllFilesInPath logs warnings internally. Library packages typically return errors and let callers decide how to log. Consider returning errors (or an aggregated error) instead of logging directly, or make logging configurable.
4	[MINOR]	gitea/client.go	28	The default HTTP client timeout is set to 30s. For large diffs or slow instances, this might be too short. Consider exposing a configuration method (e.g., WithTimeout) similar to the LLM client, or relying solely on the passed context for cancellation.
5	[NIT]	cmd/review-bot/main.go	178	In fetchFileContext and fetchPatterns, the loops continue even if the context is canceled; adding a ctx.Err() check to break early could avoid unnecessary iterations and log noise under cancellation.

Recommendation

APPROVE — The refactor is well-executed: context is threaded through all network calls, client fields are properly unexported, HTTP requests use context-aware constructors, and tests are updated and expanded (including timeouts). CI passed. I recommend merging as-is. As follow-ups, consider clarifying concurrency guarantees for the LLM client’s mutating configuration methods, making the overall timeout configurable or derived from inputs, exposing a timeout configuration for the Gitea client, and optionally breaking early in helper loops when the context is canceled to reduce unnecessary work.

## Summary Solid refactor adding context support, unexporting client fields, and improving HTTP handling. Changes are idiomatic, tests were updated accordingly, and CI passed. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `cmd/review-bot/main.go` | 74 | The fixed 3-minute top-level context timeout may conflict with or override a larger user-specified LLM timeout. Consider making the overall timeout configurable or deriving it from the longest operation to avoid unexpected cancellations. | | 2 | [MINOR] | `llm/client.go` | 15 | The comment claims the client is safe for concurrent use after construction, but WithTimeout and WithTemperature mutate internal state. Consider clarifying in documentation that these should be called during setup (before concurrent use), or guard mutations to avoid data races if used concurrently. | | 3 | [NIT] | `gitea/client.go` | 230 | GetAllFilesInPath logs warnings internally. Library packages typically return errors and let callers decide how to log. Consider returning errors (or an aggregated error) instead of logging directly, or make logging configurable. | | 4 | [MINOR] | `gitea/client.go` | 28 | The default HTTP client timeout is set to 30s. For large diffs or slow instances, this might be too short. Consider exposing a configuration method (e.g., WithTimeout) similar to the LLM client, or relying solely on the passed context for cancellation. | | 5 | [NIT] | `cmd/review-bot/main.go` | 178 | In fetchFileContext and fetchPatterns, the loops continue even if the context is canceled; adding a ctx.Err() check to break early could avoid unnecessary iterations and log noise under cancellation. | ## Recommendation **APPROVE** — The refactor is well-executed: context is threaded through all network calls, client fields are properly unexported, HTTP requests use context-aware constructors, and tests are updated and expanded (including timeouts). CI passed. I recommend merging as-is. As follow-ups, consider clarifying concurrency guarantees for the LLM client’s mutating configuration methods, making the overall timeout configurable or derived from inputs, exposing a timeout configuration for the Gitea client, and optionally breaking early in helper loops when the context is canceled to reduce unnecessary work.

gpt-review-bot approved these changes 2026-05-01 20:19:34 +00:00

Dismissed

gpt-review-bot left a comment

First-time contributor

Copy Link

Copy Source

Summary

The change cleanly adds context.Context propagation, request cancellation via http.NewRequestWithContext, unexports client internals, and replaces unnecessary string conversions. Tests were updated accordingly and CI passed.

Findings

#	Severity	File	Line	Finding
1	[MINOR]	.gitea/actions/review/action.yml	139	Added LLM_TIMEOUT environment mapping and a timeout input for the action — good addition. Consider documenting units (seconds) more prominently in the action description to avoid operator confusion.
2	[MINOR]	cmd/review-bot/main.go	72	A top-level context with a 3-minute timeout is created and reused for all operations. This is reasonable, but consider whether certain long-lived operations (e.g. large pattern fetches) should have separate, longer timeouts or cancellable child contexts to avoid a single timeout cancelling unrelated short operations.
3	[MINOR]	gitea/client.go	143	PostReview now uses http.NewRequestWithContext and bytes.NewReader(data) which is correct. Note: NewClient sets an HTTP client Timeout (30s) in addition to relying on contexts; this is fine but make sure callers expect both context and client-level timeouts to interact (client Timeout applies regardless of context).
4	[MINOR]	llm/client.go	78	Complete now accepts a context and uses NewRequestWithContext — good. The Client supports both WithTimeout (http.Client timeout) and context cancellation; consider documenting precedence and interactions between these two in the package docs so users understand which will trigger first.
5	[NIT]	llm/client_test.go	182	A new TestWithTimeout was added which intentionally asserts timeout behaviour. Tests rely on real time.Sleep durations; this is fine but can make tests slower/flaky on loaded CI runners. If stability becomes an issue, consider using httptest.Server with a channel delay control to avoid fixed sleeps.

Recommendation

APPROVE — Approve: The PR correctly implements context propagation across the Gitea and LLM clients, improves request cancellation support, hides internal client fields, and replaces unnecessary string conversions. The changes are well-scoped and tests were updated to match the API changes. I recommend merging as-is. As follow-ups, consider documenting the interaction between context-based cancellation and client-level timeouts in package comments, and consider whether some long-running operations should use separate child contexts with tailored timeouts to avoid a single top-level timeout cancelling unrelated operations.

## Summary The change cleanly adds context.Context propagation, request cancellation via http.NewRequestWithContext, unexports client internals, and replaces unnecessary string conversions. Tests were updated accordingly and CI passed. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `.gitea/actions/review/action.yml` | 139 | Added LLM_TIMEOUT environment mapping and a timeout input for the action — good addition. Consider documenting units (seconds) more prominently in the action description to avoid operator confusion. | | 2 | [MINOR] | `cmd/review-bot/main.go` | 72 | A top-level context with a 3-minute timeout is created and reused for all operations. This is reasonable, but consider whether certain long-lived operations (e.g. large pattern fetches) should have separate, longer timeouts or cancellable child contexts to avoid a single timeout cancelling unrelated short operations. | | 3 | [MINOR] | `gitea/client.go` | 143 | PostReview now uses http.NewRequestWithContext and bytes.NewReader(data) which is correct. Note: NewClient sets an HTTP client Timeout (30s) in addition to relying on contexts; this is fine but make sure callers expect both context and client-level timeouts to interact (client Timeout applies regardless of context). | | 4 | [MINOR] | `llm/client.go` | 78 | Complete now accepts a context and uses NewRequestWithContext — good. The Client supports both WithTimeout (http.Client timeout) and context cancellation; consider documenting precedence and interactions between these two in the package docs so users understand which will trigger first. | | 5 | [NIT] | `llm/client_test.go` | 182 | A new TestWithTimeout was added which intentionally asserts timeout behaviour. Tests rely on real time.Sleep durations; this is fine but can make tests slower/flaky on loaded CI runners. If stability becomes an issue, consider using httptest.Server with a channel delay control to avoid fixed sleeps. | ## Recommendation **APPROVE** — Approve: The PR correctly implements context propagation across the Gitea and LLM clients, improves request cancellation support, hides internal client fields, and replaces unnecessary string conversions. The changes are well-scoped and tests were updated to match the API changes. I recommend merging as-is. As follow-ups, consider documenting the interaction between context-based cancellation and client-level timeouts in package comments, and consider whether some long-running operations should use separate child contexts with tailored timeouts to avoid a single top-level timeout cancelling unrelated operations.

rodin added 1 commit 2026-05-01 20:26:22 +00:00

fix: address all review findings (context timeout, docs, early exit) ... 69e70466fd

- Overall context timeout now derived from LLM timeout + 1 minute
  (no longer hardcoded 3min that could conflict with longer LLM timeouts)
- Clarify concurrency docs: With* methods are setup-only, not concurrent
- Add ctx.Err() checks in fetchFileContext and fetchPatterns loops
  (break early on cancellation instead of making unnecessary requests)

sonnet-review-bot approved these changes 2026-05-01 20:27:46 +00:00

sonnet-review-bot left a comment

First-time contributor

Copy Link

Copy Source

Summary

Solid refactor adding context support across clients, unexporting client fields, and improving HTTP handling. Tests and integration points were updated consistently, and CI passed.

Recommendation

APPROVE — The changes are cohesive and idiomatic: context propagation is properly threaded through all HTTP calls, http.NewRequestWithContext is used, client fields are unexported, and bytes.NewReader replaces unnecessary string conversions. Timeouts are sensibly introduced for both Gitea and LLM clients, and main establishes a top-level context with a reasonable deadline. Unit and integration tests were updated accordingly and continue to validate behavior. As optional polish, consider exposing a way to configure the Gitea HTTP client timeout similar to the LLM client, and minimize logging inside the gitea package in favor of returning errors for callers to decide on logging. Otherwise, this is ready to merge.

## Summary Solid refactor adding context support across clients, unexporting client fields, and improving HTTP handling. Tests and integration points were updated consistently, and CI passed. ## Recommendation **APPROVE** — The changes are cohesive and idiomatic: context propagation is properly threaded through all HTTP calls, http.NewRequestWithContext is used, client fields are unexported, and bytes.NewReader replaces unnecessary string conversions. Timeouts are sensibly introduced for both Gitea and LLM clients, and main establishes a top-level context with a reasonable deadline. Unit and integration tests were updated accordingly and continue to validate behavior. As optional polish, consider exposing a way to configure the Gitea HTTP client timeout similar to the LLM client, and minimize logging inside the gitea package in favor of returning errors for callers to decide on logging. Otherwise, this is ready to merge.

gpt-review-bot approved these changes 2026-05-01 20:28:18 +00:00

gpt-review-bot left a comment

First-time contributor

Copy Link

Copy Source

Summary

The PR cleanly adds context.Context support across Gitea and LLM clients, unexports client internals, uses request contexts for cancellation, and replaces unnecessary string conversions with bytes.Reader. Tests and integration code have been updated accordingly and CI passed.

Findings

#	Severity	File	Line	Finding
1	[MINOR]	gitea/client.go	18	The package-level client methods now log warnings (log.Printf) when individual file fetches fail (GetAllFilesInPath and related). Libraries generally avoid logging and return errors to callers; consider returning errors or exposing them so callers can decide how to surface warnings instead of logging from the library.
2	[MINOR]	llm/client.go	67	WithTimeout is documented as needing to be called during setup before concurrent use, which is reasonable but not enforced. Consider documenting concurrency guarantees more explicitly or making the Client immutable after construction to avoid misuse in concurrent code.
3	[NIT]	.gitea/actions/review/action.yml	2	A small documentation line 'Requirements: python3, sha256sum, curl (all present on ubuntu-* runners).' was moved/added to the header comment — harmless but purely cosmetic.
4	[NIT]	cmd/review-bot/main.go	66	The top-level timeout is computed as llm timeout + 1 minute. This is fine, but you may want to document this behavior in the CLI help so callers understand how the overall timeout is derived from LLM timeout.

Recommendation

APPROVE — This PR is well implemented: context.Context has been threaded through all network calls, http.NewRequestWithContext is used correctly, client internals are unexported, and tests updated accordingly. Since CI passed and there are no blocking issues, approve the change. Consider the minor suggestions: avoid logging from library code (return warnings/errors to callers), and clarify the expectations around WithTimeout usage and the derived overall timeout in the CLI help or README to reduce surprise for users.

## Summary The PR cleanly adds context.Context support across Gitea and LLM clients, unexports client internals, uses request contexts for cancellation, and replaces unnecessary string conversions with bytes.Reader. Tests and integration code have been updated accordingly and CI passed. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `gitea/client.go` | 18 | The package-level client methods now log warnings (log.Printf) when individual file fetches fail (GetAllFilesInPath and related). Libraries generally avoid logging and return errors to callers; consider returning errors or exposing them so callers can decide how to surface warnings instead of logging from the library. | | 2 | [MINOR] | `llm/client.go` | 67 | WithTimeout is documented as needing to be called during setup before concurrent use, which is reasonable but not enforced. Consider documenting concurrency guarantees more explicitly or making the Client immutable after construction to avoid misuse in concurrent code. | | 3 | [NIT] | `.gitea/actions/review/action.yml` | 2 | A small documentation line 'Requirements: python3, sha256sum, curl (all present on ubuntu-* runners).' was moved/added to the header comment — harmless but purely cosmetic. | | 4 | [NIT] | `cmd/review-bot/main.go` | 66 | The top-level timeout is computed as llm timeout + 1 minute. This is fine, but you may want to document this behavior in the CLI help so callers understand how the overall timeout is derived from LLM timeout. | ## Recommendation **APPROVE** — This PR is well implemented: context.Context has been threaded through all network calls, http.NewRequestWithContext is used correctly, client internals are unexported, and tests updated accordingly. Since CI passed and there are no blocking issues, approve the change. Consider the minor suggestions: avoid logging from library code (return warnings/errors to callers), and clarify the expectations around WithTimeout usage and the derived overall timeout in the CLI help or README to reduce surprise for users.

aweiker approved these changes 2026-05-01 20:29:03 +00:00

rodin merged commit f8e77cf7e3 into main 2026-05-01 21:10:37 +00:00

rodin referenced this issue from a commit 2026-05-01 21:10:39 +00:00

Merge pull request 'feat: add context.Context + unexport client fields' (#14) from fix/context-and-encapsulation into main

Sign in to join this conversation.

Reviewers

No Reviewers

sonnet-review-bot

gpt-review-bot

aweiker

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

4 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: rodin/review-bot#14