Add provider health diagnostics - #151
Conversation
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
WalkthroughAdds a providerhealth module that validates provider config, checks credentials, and optionally probes provider endpoints. The CLI is wired to call it via a new deps field and ChangesProvider Connectivity Health Probing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (2)
internal/providerhealth/providerhealth_test.go (1)
15-143: ⚡ Quick winAdd a regression test for “resolved kind requires auth” when
ProviderKindis unset.Current tests don’t cover the case where runtime metadata resolves to an auth-required provider while raw
profile.ProviderKindis empty. A focused test here will prevent false-pass auth diagnostics from regressing.Suggested test shape
+func TestProbeAuthUsesResolvedProviderKind(t *testing.T) { + result := Probe(context.Background(), Options{ + Profile: config.ProviderProfile{ + // Intentionally omit ProviderKind; rely on legacy/provider string path. + Provider: "openai", + Model: "gpt-4.1", + }, + }) + + check := result.Check("provider.auth") + if check == nil || check.Status != StatusFail || check.Category != CategoryAuth { + t.Fatalf("provider.auth = %#v, want auth failure", check) + } +}🤖 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/providerhealth/providerhealth_test.go` around lines 15 - 143, Add a new unit test (e.g., TestProbeResolvedKindRequiresAuthWhenUnset) that starts an httptest.Server returning 401 with a body containing the API key, then call Probe(...) with Options where Profile.ProviderKind is empty but Profile.BaseURL points to the server (and Profile.APIKey = "sk-test-secret"), Connectivity=true and HTTPClient=server.Client(); assert Probe result.Status is StatusFail, get result.Check("provider.connectivity") and assert check.Category == CategoryAuth and that check.Message does not contain "sk-test-secret" to ensure the resolved provider kind requiring auth is classified and secrets are redacted.internal/cli/command_center_test.go (1)
447-449: ⚡ Quick winAssert the success connectivity path does not construct a runtime provider.
The success-path test currently allows
deps.newProviderto run without failing, so it won’t catch regressions that accidentally reintroduce provider construction inproviders check --connectivity.Proposed test hardening
- deps.newProvider = func(config.ProviderProfile) (zeroruntime.Provider, error) { - return commandCenterProvider{}, nil - } + deps.newProvider = func(config.ProviderProfile) (zeroruntime.Provider, error) { + t.Fatal("newProvider should not run during connectivity checks") + return nil, nil + }🤖 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/cli/command_center_test.go` around lines 447 - 449, The test should fail if the code under test constructs a runtime provider during the success connectivity path; change the test's deps.newProvider stub (currently returning commandCenterProvider{}, nil) to a function that returns a clear error (or panics) so any accidental provider construction is caught, and update assertions accordingly; locate the stub assigned to deps.newProvider in the test and replace its return with an error-returning closure (or panic) to assert providers check --connectivity does not call newProvider.
🤖 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/providerhealth/providerhealth.go`:
- Around line 147-155: The auth check is using the raw profile when calling
credentialRequired(profile) and hasCredential(profile) which can be empty for
profiles that resolve their kind via runtime metadata; update the auth logic to
use the resolved runtime kind (profile.ProviderKind) instead of the raw profile
where the requirement is evaluated: call
credentialRequired(profile.ProviderKind) and hasCredential(profile.ProviderKind)
(or adjust those functions to accept a providerKind string) so the check and
messages (providerName(profile) can remain) consistently use the resolved
provider kind when deciding if credentials are required or present.
---
Nitpick comments:
In `@internal/cli/command_center_test.go`:
- Around line 447-449: The test should fail if the code under test constructs a
runtime provider during the success connectivity path; change the test's
deps.newProvider stub (currently returning commandCenterProvider{}, nil) to a
function that returns a clear error (or panics) so any accidental provider
construction is caught, and update assertions accordingly; locate the stub
assigned to deps.newProvider in the test and replace its return with an
error-returning closure (or panic) to assert providers check --connectivity does
not call newProvider.
In `@internal/providerhealth/providerhealth_test.go`:
- Around line 15-143: Add a new unit test (e.g.,
TestProbeResolvedKindRequiresAuthWhenUnset) that starts an httptest.Server
returning 401 with a body containing the API key, then call Probe(...) with
Options where Profile.ProviderKind is empty but Profile.BaseURL points to the
server (and Profile.APIKey = "sk-test-secret"), Connectivity=true and
HTTPClient=server.Client(); assert Probe result.Status is StatusFail, get
result.Check("provider.connectivity") and assert check.Category == CategoryAuth
and that check.Message does not contain "sk-test-secret" to ensure the resolved
provider kind requiring auth is classified and secrets are redacted.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b24cf13-699f-485f-a69f-6ab83f7148dd
📒 Files selected for processing (10)
internal/cli/app.gointernal/cli/command_center.gointernal/cli/command_center_test.gointernal/cli/observability.gointernal/cli/observability_test.gointernal/cli/provider_setup.gointernal/doctor/doctor.gointernal/providerhealth/providerhealth.gointernal/providerhealth/providerhealth_additional_test.gointernal/providerhealth/providerhealth_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/providerhealth/providerhealth_test.go`:
- Around line 128-131: The test's httptest handler is calling t.Fatalf inside
its goroutine (see TestProbeResolvedKindConnectivityAuthErrorRedactsSecret and
the handler that reads r.Header.Get("Authorization")), which is unsafe; instead
capture the Authorization header value in the handler (e.g., assign it to a
variable or send it over a channel) and return normally, then after Probe
returns perform the assertion using t.Fatalf/t.Errorf from the main test
goroutine comparing the captured header to "Bearer sk-test-secret". Ensure the
captured value is safely synchronized (closure variable set before handler
returns or received from a channel) so the post-call assertion observes the
handler value.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ae7f4ce2-a133-4ee0-86f0-2491f8a8ab80
📒 Files selected for processing (3)
internal/cli/command_center_test.gointernal/providerhealth/providerhealth.gointernal/providerhealth/providerhealth_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/cli/command_center_test.go
- internal/providerhealth/providerhealth.go
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict: Request changes — solid, useful feature, but the connectivity probe has 3 High security issues (2 SSRF + 1 credential-redaction gap). I traced each against the code; details + file:line below. The good news: the codebase already has the exact SSRF mitigation in internal/tools/web_fetch.go to reuse.
High (please fix before merge)
H1 — SSRF: the probe GETs the configured baseURL verbatim, no host/IP deny-list.
internal/providerhealth/providerhealth.go:217 does client.Do(request) against resolvedBaseURL(profile,kind)+healthPath (:249-283), and resolvedBaseURL for openai-compatible/anthropic-compatible only runs providerio.NormalizeBaseURL → url.ParseRequestURI (no host/IP/scheme restriction). The provider baseURL is project-config controlled (.zero/config.json is read from the workspace root, which an agentic CLI treats as untrusted), and a credential-less openai-compatible provider bypasses the only existing baseURL guard (validateProjectProviderMerge fires only when credential material is inherited). So a malicious repo can ship {"providers":[{"name":"x","provider_kind":"openai-compatible","baseURL":"http://169.254.169.254","model":"x"}]}; zero providers check x --connectivity / zero doctor --connectivity then GETs the cloud-metadata / 127.0.0.1 / 10.x / NAT64 [64:ff9b::a9fe:a9fe] endpoint. It's blind-ish exfil too: status code + up to 64KB of the internal response body are surfaced into the check Message/Details (:225,227-230,242-246) → terminal/JSON.
H2 — SSRF + credential capture via redirects. Both call sites (cli/observability.go:55-60, cli/provider_setup.go:98-102) leave Options.HTTPClient nil, so the probe falls back to http.DefaultClient (providerhealth.go:213-216) — which follows up to 10 redirects with no per-hop re-validation. So even an official endpoint can 302 → an internal host, and because applyAuth (:262,296-328) attaches the real Authorization/x-api-key/x-goog-api-key, Go forwards those headers on a same-registered-host redirect → a malicious/compromised endpoint can capture the key. (The 5s context timeout does bound total time, so no hang DoS — just the redirect SSRF + credential forwarding.)
Fix for H1+H2 (reuse what's already here): internal/tools/web_fetch.go already has webFetchBlockedAddrPrefixes (0.0.0.0/8, 10/8, 100.64/10, 127/8, 169.254/16, 172.16/12, 192.168/16, ::1, fc00::/7, fe80::/10, multicast, NAT64 64:ff9b::/96, 6to4 2002::/16) + a metadata.google.internal host block + a safe dial context + a redirect policy. Build a dedicated http.Client for the probe that: (a) restricts scheme to http/https; (b) uses a Transport.DialContext that re-checks the dialed IP against the deny-list (defeats DNS rebinding); (c) sets CheckRedirect to re-validate every hop (or just disable redirects — a health probe doesn't need them); (d) sets an explicit Timeout. Don't use the shared http.DefaultClient.
H3 — API key embedded in a baseURL query param leaks unredacted. redactBaseURL (providerhealth.go:398-409) only strips URL userinfo (parsed.User = nil); a key in a query parameter (?key=... / ?api_key=...) survives. It then flows to redact() (:411-416), which only removes the configured secret values + known token patterns (sk-, AIza, JWT, …) — a non-standard/custom key shape in a query param reaches the Message/Details/JSON and the wrapped *url.Error strings (:229,245,346) unredacted. Strip/redact query parameters (and the whole URL on transport errors), not just userinfo.
Medium
M1 — 503 miscategorized as rate-limit/warn → exit 0. providerhealth.go:238 groups 503 Service Unavailable with 429/529 under CategoryRateLimit+StatusWarn. A 503 means the endpoint is down, not throttled, and StatusWarn exits 0 — so a hard outage reads as "ok-ish". Map 503 to a failure category/status (or at least separate it from rate-limit).
Low / Nit (worth addressing)
- status masking:
provider_setup.go:129(and the JSON path) printstatus: okwhen the connectivity result isStatusWarn, contradicting theconnectivity: warnline — surface warn in the top-level status. - scheme allow-list (defense-in-depth):
resolvedBaseURL/NormalizeBaseURLdon't constrain scheme; today Go's transport rejects non-http(s), but validate explicitly rather than relying on that. - error classification:
context.Canceledis mapped toCategoryTimeout(:332) — it's an external cancel, not a timeout. - no signal→context bridge: both call sites pass
context.Background()(observability.go:55,provider_setup.go:98); Ctrl+C won't abort an in-flight probe (the 5s timeout still bounds it). - body read error swallowed:
:225body, _ := io.ReadAll(...)can operate on a truncated body silently. - test gaps: the text-mode (
--connectivitynon-JSON) output + exit codes are untested (command_center_test.go); andTestRunDoctorConnectivityProbesProvider(observability_test.go:75) hits a real httptest server instead of injecting theprobeProviderHealthseam — add SSRF/redaction regression tests once the guard lands. - help text: the
--connectivitydescription (command_center.go:437) only mentionsproviders check, thoughdoctor --connectivityis an equal entry point.
The H1/H2 SSRF guard is the main blocker — reusing the web_fetch.go machinery should make it a contained change. Happy to re-review once that's in.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Reviewed the full diff (built + ran go test ./internal/providerhealth ./internal/doctor ./internal/cli — all green). This is a clean, well-factored package — nice work. Verdict: approve with two minor nits.
What's great
- Redaction discipline is excellent. Every message, detail value, and URL goes through
redact()/redactAny()/redactBaseURLwith the profile's secrets, and base-URL userinfo is stripped. The tests prove secrets don't leak in messages or detail URLs. - The probe is safe and bounded: context timeout,
io.LimitReader(…, 64*1024), and a non-generatingGET /models(no token cost), with an injectableHTTPClient+probeProviderHealthdep for testability. - Error categorization is thorough — auth / rate_limit / network / timeout / provider / unsupported, with sensible status mapping (rate-limit → warn, others → fail) and
classifyTransportErrorcorrectly unwrappingurl.Error/net.Error/context deadlines. - Strong scenario coverage in the tests (config-only, auth error, timeout, rate-limit, network, unsupported transport, redaction).
Minor
- Dead alias —
providerhealth.go:42:CategoryProviderError = CategoryProviderisn't referenced anywhere in the tree, and the untyped alias breaks the const block's gofmt alignment. Suggest removing it (or, if kept for API symmetry, type itCategory). PrimaryCheckprecedence —providerhealth.go:85: it returns the connectivity check regardless of its status, before scanning for fail/warn. Today the onlywarnis rate-limit on connectivity itself, so it's fine — but if a non-connectivitywarnis ever added, a passing connectivity check would mask it. Consider fail > warn across all checks first, then fall back to connectivity.
Optional suggestion
connectivityCheckwill send the API key toprofile.BaseURLeven when it'shttp://(cleartext). That's the same exposure as a real provider call (not a regression), but a health command is a natural place towarnwhen credentials would cross a non-TLS endpoint.
Nothing blocking — the two nits are quick. Thanks for the careful redaction work.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/providerhealth/providerhealth.go (1)
285-288:⚠️ Potential issue | 🟠 MajorClose SSRF TOCTOU in provider connectivity probe (redirect + dial-time enforcement)
internal/providerhealth/providerhealth.govalidates only the initial URL once (healthRequest around lines ~329), butconnectivityCheckthen usesoptions.HTTPClient(falling back tohttp.DefaultClient) and executesclient.Do(request)(around ~285) with noCheckRedirector transport/DialContext enforcement. Redirects (and any DNS re-resolution/DNS rebinding during dialing) can bypass the safety check—so add redirect-time revalidation and dial-time destination validation usingoptions.Resolver, and add tests for “allowed initial URL → blocked redirect target” and “DNS changes after validation.”🤖 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/providerhealth/providerhealth.go` around lines 285 - 288, The connectivity probe currently validates only the initial URL (healthRequest) but uses client.Do(request) in connectivityCheck without guarding redirects or dial-time resolution, allowing SSRF via redirect or DNS rebinding; update the HTTP client used by connectivityCheck to set a CheckRedirect hook that revalidates every redirect location against the same allowlist logic used by healthRequest (reject and return error if a redirect target is disallowed), and replace/override the Transport's DialContext to perform resolver-based destination validation (use options.Resolver to resolve the IP for the final host/port and enforce the same allowlist before dialing); ensure these changes are applied when falling back to http.DefaultClient, and add unit tests covering "allowed initial URL → blocked redirect target" and "DNS changes after validation" that assert the probe rejects the request on redirect or changed resolution.
🤖 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.
Outside diff comments:
In `@internal/providerhealth/providerhealth.go`:
- Around line 285-288: The connectivity probe currently validates only the
initial URL (healthRequest) but uses client.Do(request) in connectivityCheck
without guarding redirects or dial-time resolution, allowing SSRF via redirect
or DNS rebinding; update the HTTP client used by connectivityCheck to set a
CheckRedirect hook that revalidates every redirect location against the same
allowlist logic used by healthRequest (reject and return error if a redirect
target is disallowed), and replace/override the Transport's DialContext to
perform resolver-based destination validation (use options.Resolver to resolve
the IP for the final host/port and enforce the same allowlist before dialing);
ensure these changes are applied when falling back to http.DefaultClient, and
add unit tests covering "allowed initial URL → blocked redirect target" and "DNS
changes after validation" that assert the probe rejects the request on redirect
or changed resolution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 685e78a6-5240-426a-a6c9-40a8d141edbb
📒 Files selected for processing (7)
internal/cli/command_center_test.gointernal/cli/observability.gointernal/cli/observability_test.gointernal/cli/provider_setup.gointernal/providerhealth/providerhealth.gointernal/providerhealth/providerhealth_additional_test.gointernal/providerhealth/providerhealth_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/cli/observability.go
- internal/cli/provider_setup.go
- internal/providerhealth/providerhealth_test.go
- internal/providerhealth/providerhealth_additional_test.go
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Re-reviewed after your fixes (rebuilt + ran go test ./internal/providerhealth — green). Both earlier nits are resolved and the new hardening is excellent. LGTM — approving, with one optional follow-up.
Resolved ✅
- Dead
CategoryProviderErroralias removed + const block realigned. PrimaryChecknow prefers fail → warn before falling back to connectivity.
Great addition — SSRF blocklist
validateEndpoint + blockedAddrReason is thorough: IPv4/IPv6 special-use ranges, NAT64 / IPv4-mapped unwrapping via embeddedIPv4Prefixes, the localhost/.localhost check, and a literal-IP fast path. Solid defense for a probe that carries the API key to a user-configured base URL.
Optional follow-up (non-blocking) — DNS-rebinding TOCTOU
validateEndpoint resolves the host and checks the addresses, but the request is then sent via http.DefaultClient (the CLI path sets neither Options.HTTPClient nor Resolver), which resolves again at dial time — so a host can pass validation on one lookup and connect to a blocked IP on the next. To make the blocklist airtight, pin the connection to the validated address: a custom net.Dialer with a Control/DialContext that re-checks the actual dialed addr against blockedAddrReason, wired into the client's Transport. Low severity here (the base URL is the user's own config, and a 2xx body isn't surfaced — so a reachability oracle at most), but worth closing since the protection is clearly meant to be robust.
Minor
503 Service Unavailable was dropped from the rate-limit/warn bucket (now only 429/529 warn → 503 falls to provider-error/fail). Intentional? It's often a transient provider hiccup that a warn fits better than a hard fail.
Thanks for the careful hardening pass — nice work.
2 participants
Summary
zero providers check <name> --connectivity [--json]to emit structured health diagnosticszero doctor --connectivityto use the real provider health probe and surface concrete failuresTests
go test ./internal/providerhealth ./internal/cli ./internal/doctorgo test ./...go run ./cmd/zero-release buildgo run ./cmd/zero-release smokeSummary by CodeRabbit
New Features
Improvements
Tests