Introduced a shared HTTP client with optimized settings - #75
Conversation
WalkthroughCentralized HTTP client management was introduced via internal/http/client.go. Existing providers (Claude, Grok, Groq) were refactored to use the shared client getter. Ollama integration now uses a dedicated long-timeout client and explicit http.Request construction with headers. Redundant per-file client setups and TLS configs were removed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Provider as Provider (Claude/Grok/Groq)
participant HTTPPkg as internal/http
participant Net as net/http
Caller->>Provider: GenerateCommitMessage(...)
Provider->>HTTPPkg: GetClient()
activate HTTPPkg
alt First call
HTTPPkg->>HTTPPkg: init sharedClient (sync.Once)
note over HTTPPkg: Timeout: 30s<br/>Shared Transport
else Subsequent calls
HTTPPkg-->>Provider: return sharedClient
end
deactivate HTTPPkg
Provider->>Net: sharedClient.Do(req)
Net-->>Provider: resp / error
Provider-->>Caller: result / error
sequenceDiagram
autonumber
actor Caller
participant Ollama as internal/ollama
participant HTTPPkg as internal/http
participant Net as net/http
Caller->>Ollama: GenerateCommitMessage(...)
Ollama->>Ollama: http.NewRequest(POST, url, body)
Ollama->>Ollama: req.Header["Content-Type"] = application/json
Ollama->>HTTPPkg: GetOllamaClient()
activate HTTPPkg
alt First call
HTTPPkg->>HTTPPkg: init ollamaClient (sync.Once)
note over HTTPPkg: Timeout: 10m<br/>Shared Transport
end
HTTPPkg-->>Ollama: ollamaClient
deactivate HTTPPkg
Ollama->>Net: ollamaClient.Do(req)
Net-->>Ollama: resp
Ollama->>Ollama: read body, status check, JSON decode
Ollama-->>Caller: result / error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🪛 ast-grep (0.39.5)internal/http/client.go[warning] 24-26: MinVersion (missing-ssl-minversion-go) 🔇 Additional comments (2)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
internal/claude/claude.go(2 hunks)internal/grok/grok.go(2 hunks)internal/groq/groq.go(3 hunks)internal/http/client.go(1 hunks)internal/ollama/ollama.go(2 hunks)test2.txt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
internal/grok/grok.go (1)
internal/http/client.go (1)
GetClient(19-37)
internal/claude/claude.go (1)
internal/http/client.go (1)
GetClient(19-37)
internal/ollama/ollama.go (1)
internal/http/client.go (1)
GetOllamaClient(40-58)
internal/groq/groq.go (1)
internal/http/client.go (1)
GetClient(19-37)
🪛 ast-grep (0.39.5)
internal/http/client.go
[warning] 25-27: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: false,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
[warning] 46-48: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: false,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🔇 Additional comments (5)
internal/groq/groq.go (1)
85-88: Shared client usage looks solidGreat to see this path now using the centralized HTTP client; the swap is clean and keeps the existing flow intact.
internal/claude/claude.go (1)
62-66: Centralized client integration looks goodThe switch to the shared client is straightforward and keeps the Claude call path unchanged otherwise. Nicely aligned with the new factory.
internal/ollama/ollama.go (1)
50-59: Ollama request refactor looks correctBuilding the request explicitly and routing it through
GetOllamaClient()keeps behavior while benefiting from the dedicated timeout—nice cleanup.internal/grok/grok.go (1)
48-52: Shared client adoption confirmedThis path now leverages the centralized client without altering the rest of the flow—exactly what we wanted.
internal/http/client.go (1)
10-16: LGTM! Singleton pattern correctly implemented.The use of separate
sync.Onceinstances for each client ensures thread-safe lazy initialization without race conditions.
| // GetClient returns a shared HTTP client with optimized settings for cloud APIs | ||
| func GetClient() *http.Client { | ||
| clientOnce.Do(func() { | ||
| transport := &http.Transport{ | ||
| TLSHandshakeTimeout: 10 * time.Second, | ||
| MaxIdleConns: 10, | ||
| IdleConnTimeout: 30 * time.Second, | ||
| DisableCompression: true, | ||
| TLSClientConfig: &tls.Config{ | ||
| InsecureSkipVerify: false, | ||
| }, | ||
| } | ||
|
|
||
| sharedClient = &http.Client{ | ||
| Timeout: 30 * time.Second, | ||
| Transport: transport, | ||
| } | ||
| }) | ||
| return sharedClient | ||
| } |
There was a problem hiding this comment.
Add explicit TLS MinVersion and extract common transport logic.
Two issues:
- Security gap: The TLS configuration lacks an explicit
MinVersion, defaulting to TLS 1.2. Modern applications should enforce TLS 1.3. - Code duplication: Transport settings are duplicated in
GetOllamaClient. Extract to a helper function.
As per static analysis hints.
Apply this diff to address both issues:
+// createTransport returns a configured HTTP transport with secure TLS settings
+func createTransport() *http.Transport {
+ return &http.Transport{
+ TLSHandshakeTimeout: 10 * time.Second,
+ MaxIdleConns: 10,
+ IdleConnTimeout: 30 * time.Second,
+ DisableCompression: true,
+ TLSClientConfig: &tls.Config{
+ InsecureSkipVerify: false,
+ MinVersion: tls.VersionTLS13,
+ },
+ }
+}
+
// GetClient returns a shared HTTP client with optimized settings for cloud APIs
func GetClient() *http.Client {
clientOnce.Do(func() {
- transport := &http.Transport{
- TLSHandshakeTimeout: 10 * time.Second,
- MaxIdleConns: 10,
- IdleConnTimeout: 30 * time.Second,
- DisableCompression: true,
- TLSClientConfig: &tls.Config{
- InsecureSkipVerify: false,
- },
- }
-
sharedClient = &http.Client{
Timeout: 30 * time.Second,
- Transport: transport,
+ Transport: createTransport(),
}
})
return sharedClient
}🧰 Tools
🪛 ast-grep (0.39.5)
[warning] 25-27: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: false,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🤖 Prompt for AI Agents
In internal/http/client.go around lines 18 to 37, the TLS config is missing an
explicit MinVersion (defaulting to TLS1.2) and the transport creation is
duplicated elsewhere; update the TLSClientConfig to set MinVersion:
tls.VersionTLS13 and extract the transport construction into a new helper
function (e.g., newSharedTransport) that returns *http.Transport with
TLSHandshakeTimeout, MaxIdleConns, IdleConnTimeout, DisableCompression and
TLSClientConfig (including InsecureSkipVerify:false and MinVersion), then call
that helper from GetClient (and from GetOllamaClient) so both use the same
centralized transport-building logic.
DFanso
left a comment
DFanso
left a comment
There was a problem hiding this comment.
hey @vinyas-bharadwaj can u remove the test2.txt file
DFanso
added
bug
2 participants
Description
Type of Change
Related Issue
Fixes #60
Changes Made
Testing
Checklist
Screenshots (if applicable)
Additional Notes
For Hacktoberfest Participants
Thank you for your contribution! 🎉
Summary by CodeRabbit
Refactor
Performance
Chores