fix(config): honor the http: YAML block for upstream client timeouts#483
Merged
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds startup installation of YAML-configured HTTP timeouts into ChangesHTTP Timeout Configuration Wiring
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant App as app.New
participant Config as appCfg.HTTP
participant HTTPClient as httpclient.SetConfiguredTimeouts
participant Providers as provider construction
App->>Config: read Timeout and ResponseHeaderTimeout
App->>HTTPClient: SetConfiguredTimeouts(timeout, responseHeaderTimeout)
App->>Providers: construct providers
Providers->>HTTPClient: DefaultConfig()
HTTPClient-->>Providers: env vars, configured values, or built-in defaults
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/httpclient/client_test.go`:
- Around line 291-297: The “clear back to default” test case in DefaultConfig
only verifies Timeout and leaves ResponseHeaderTimeout unchecked, so add a
symmetric assertion for the response header timeout reset as well. Update the
final scenario around SetConfiguredTimeouts and DefaultConfig to confirm
configuredResponseHeaderTimeoutSeconds reverts to the built-in default when the
env value is cleared, using the existing DefaultConfig() and
ResponseHeaderTimeout symbols to locate the coverage gap.
In `@internal/httpclient/client.go`:
- Around line 71-81: Clarify the timeout behavior in
SetConfiguredTimeouts/configuredOrDefault: non-positive YAML-configured values
are currently treated as “unset” and fall back to the default instead of
disabling the timeout like the env-var path can. Update the
SetConfiguredTimeouts docstring (and any nearby comments on configuredOrDefault
if needed) to explicitly state that 0 or negative values do not mean “no
timeout” here and will revert to the built-in defaults, so callers understand
the asymmetry with getEnvDuration and http.Client.Timeout.
- Around line 90-100: `DefaultConfig()` and `config.Load()` are parsing
`HTTP_TIMEOUT` differently, so the env var works in
`internal/httpclient.getEnvDuration()` but fails when
`config.HTTPConfig.Timeout` is loaded as an int. Update the config-loading path
to use the same duration parsing as `DefaultConfig()` (or make both paths
seconds-only), and keep the `HTTP_TIMEOUT` handling consistent across the
`ClientConfig` and `HTTPConfig.Timeout` code paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7e939de3-8745-4f24-ba23-5d21b2d6aadc
📒 Files selected for processing (6)
CLAUDE.mdconfig/http.godocs/dev/2026-07-04_architecture-review.mdinternal/app/app.gointernal/httpclient/client.gointernal/httpclient/client_test.go
Comment on lines
+291
to
+297
| // Non-positive values clear back to the built-in default. | ||
| t.Setenv("HTTP_TIMEOUT", "") | ||
| SetConfiguredTimeouts(-1, 0) | ||
| if got := DefaultConfig().Timeout; got != 600*time.Second { | ||
| t.Fatalf("cleared Timeout = %v, want 600s", got) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Final scenario doesn't assert ResponseHeaderTimeout reverts too.
The "clear back to default" case only checks .Timeout; HTTP_RESPONSE_HEADER_TIMEOUT remains set to "60" from the earlier scenario and configuredResponseHeaderTimeoutSeconds isn't independently verified to revert. Consider asserting ResponseHeaderTimeout as well for symmetric coverage.
✅ Suggested addition
// Non-positive values clear back to the built-in default.
t.Setenv("HTTP_TIMEOUT", "")
+ t.Setenv("HTTP_RESPONSE_HEADER_TIMEOUT", "")
SetConfiguredTimeouts(-1, 0)
- if got := DefaultConfig().Timeout; got != 600*time.Second {
- t.Fatalf("cleared Timeout = %v, want 600s", got)
- }
+ cfg = DefaultConfig()
+ if cfg.Timeout != 600*time.Second {
+ t.Fatalf("cleared Timeout = %v, want 600s", cfg.Timeout)
+ }
+ if cfg.ResponseHeaderTimeout != 600*time.Second {
+ t.Fatalf("cleared ResponseHeaderTimeout = %v, want 600s", cfg.ResponseHeaderTimeout)
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/httpclient/client_test.go` around lines 291 - 297, The “clear back
to default” test case in DefaultConfig only verifies Timeout and leaves
ResponseHeaderTimeout unchecked, so add a symmetric assertion for the response
header timeout reset as well. Update the final scenario around
SetConfiguredTimeouts and DefaultConfig to confirm
configuredResponseHeaderTimeoutSeconds reverts to the built-in default when the
env value is cleared, using the existing DefaultConfig() and
ResponseHeaderTimeout symbols to locate the coverage gap.
Comment on lines
90
to
+100
| func DefaultConfig() ClientConfig { | ||
| defaultLongTimeout := 600 * time.Second | ||
| return ClientConfig{ | ||
| MaxIdleConns: 100, | ||
| MaxIdleConnsPerHost: 100, | ||
| IdleConnTimeout: 90 * time.Second, | ||
| Timeout: getEnvDuration("HTTP_TIMEOUT", defaultLongTimeout), | ||
| Timeout: getEnvDuration("HTTP_TIMEOUT", configuredOrDefault(&configuredTimeoutSeconds, defaultLongTimeout)), | ||
| DialTimeout: 30 * time.Second, | ||
| KeepAlive: 30 * time.Second, | ||
| TLSHandshakeTimeout: 10 * time.Second, | ||
| ResponseHeaderTimeout: getEnvDuration("HTTP_RESPONSE_HEADER_TIMEOUT", defaultLongTimeout), | ||
| ResponseHeaderTimeout: getEnvDuration("HTTP_RESPONSE_HEADER_TIMEOUT", configuredOrDefault(&configuredResponseHeaderTimeoutSeconds, defaultLongTimeout)), |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how config.Load() applies env overrides for HTTPConfig fields.
rg -n "applyEnvOverrides" -A 40 config/*.go
rg -n "HTTP_TIMEOUT|HTTP_RESPONSE_HEADER_TIMEOUT" config/*.go
rg -n "env:\"" config/http.goRepository: ENTERPILOT/GoModel
Length of output: 10854
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== internal/httpclient/client.go =="
sed -n '1,220p' internal/httpclient/client.go
echo
echo "== config/http.go =="
cat -n config/http.go
echo
echo "== config/env.go around int parsing =="
sed -n '80,140p' config/env.go
echo
echo "== tests touching HTTP_TIMEOUT =="
sed -n '250,330p' config/config_helpers_test.go
sed -n '1390,1435p' config/config_test.goRepository: ENTERPILOT/GoModel
Length of output: 11805
Align HTTP_TIMEOUT parsing in both config paths
config.Load() still parses config.HTTPConfig.Timeout as an int, so HTTP_TIMEOUT=2m fails during config load even though internal/httpclient.getEnvDuration() accepts Go duration strings. Make the two paths accept the same format, or document these env vars as seconds-only.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/httpclient/client.go` around lines 90 - 100, `DefaultConfig()` and
`config.Load()` are parsing `HTTP_TIMEOUT` differently, so the env var works in
`internal/httpclient.getEnvDuration()` but fails when
`config.HTTPConfig.Timeout` is loaded as an int. Update the config-loading path
to use the same duration parsing as `DefaultConfig()` (or make both paths
seconds-only), and keep the `HTTP_TIMEOUT` handling consistent across the
`ClientConfig` and `HTTPConfig.Timeout` code paths.
Contributor
Author
|
@copilot Resolve conflicts in CLAUDE.md - merging main |
Copilot stopped work on behalf of
SantiagoDePolonia due to an error
July 5, 2026 11:06
config.HTTPConfig was parsed, defaulted, and documented in config.example.yaml but never read by any code: setting http.timeout or http.response_header_timeout in YAML was silently ignored, and only the HTTP_TIMEOUT / HTTP_RESPONSE_HEADER_TIMEOUT env vars worked via httpclient's direct read. App startup now installs the YAML values into httpclient before any provider constructs a transport; env vars keep precedence per the project-wide env-over-YAML convention. Precedence is pinned by a test. Also from verifying the review's config escape-hatch findings: the OPENROUTER_SITE_URL/APP_NAME 'undocumented hidden config' claim was a false positive (both are in .env.template and provider docs) — the review doc is corrected, and CLAUDE.md's provider list gains the missing mentions (OPENROUTER_SITE_URL/APP_NAME, ANTHROPIC_DEFAULT_MAX_TOKENS). The remaining per-provider construction-time env reads are kept deliberately as the established quirk-knob convention; the anthropic request-time read keeps its warn-once fallback (plumbing judged not worth the churn), both recorded in the review doc. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
SantiagoDePolonia
force-pushed
the
refactor/config-escape-hatches
branch
from
2b33a3a to
4615b8f
Compare
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closing out the architecture review's config escape-hatch item — with a twist found during verification.
The real defect (fixed)
config.HTTPConfig(http:YAML block) was parsed, defaulted, and documented inconfig.example.yaml— but no code ever read it. Settinghttp.timeout/http.response_header_timeoutin YAML was silently ignored; only theHTTP_TIMEOUT/HTTP_RESPONSE_HEADER_TIMEOUTenv vars worked, viahttpclient's direct env read.Fix:
httpclient.SetConfiguredTimeoutsinstalls the YAML values at app startup, before any provider constructs a transport. Precedence (highest first): env var → YAMLhttp:block → built-in 600s default, matching the project-wide env-over-YAML convention. Pinned by a test covering all three tiers plus clearing.Findings re-verified, not blindly applied
OPENROUTER_SITE_URL/OPENROUTER_APP_NAMEwere "undocumented hidden config" — false positive: both are in.env.templateand the provider docs. The review doc is corrected in place.ProviderConfigwould add ceremony for no behavior gain.ANTHROPIC_DEFAULT_MAX_TOKENSread keeps its warn-once + safe-fallback behavior; threading it through provider construction wasn't worth the signature churn. Recorded in the review doc.OPENROUTER_SITE_URL/APP_NAME,ANTHROPIC_DEFAULT_MAX_TOKENS) and the HTTP bullet now notes the YAML block.User-visible impact
YAML-configured HTTP timeouts now actually apply. Anyone relying on the documented
http:block gets the behavior the docs always promised; env-var users see no change.Testing
Full
go build ./...andgo test ./...; new precedence test ininternal/httpclient;golangci-lint0 issues; pre-commitmake test-raceon the commit.🤖 Generated with Claude Code
Summary by CodeRabbit
http:configuration are now applied during startup.