fix: use OAuth credentials in provider health checks - #828
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughProvider health probing now supports OAuth credential resolution and refresh after unauthorized responses. CLI dependency wiring supplies OAuth login to connectivity probes, with tests covering bearer-token requests, token refresh, and doctor connectivity output. ChangesOAuth Provider Health Probing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DoctorCLI
participant ProviderHealth
participant OAuthLogin
participant ProviderServer
DoctorCLI->>ProviderHealth: probe connectivity
ProviderHealth->>OAuthLogin: resolve OAuth token
OAuthLogin-->>ProviderHealth: bearer token
ProviderHealth->>ProviderServer: connectivity request
ProviderServer-->>ProviderHealth: response
ProviderHealth-->>DoctorCLI: connectivity check result
🚥 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/providerhealth/providerhealth_test.go (1)
183-273: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSolid coverage of the connectivity paths; consider a companion test for the non-connectivity case.
Both new tests are useful and correctly validate the resolver-driven bearer header and the 401 refresh-and-retry flow. Neither exercises
ProbewithConnectivity: falseand anOAuthResolverthat would returnok=false— the scenario tied to the gating gap flagged inproviderhealth.go(Lines 211-230), where a config-only probe could report "has OAuth credentials configured" without ever calling the resolver.🤖 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 183 - 273, The provider health tests cover OAuth connectivity but miss the non-connectivity gating path. Add a companion test near TestProbeConnectivityUsesOAuthCredential that calls Probe with Connectivity false and an OAuthResolver returning ok=false, then assert the result reflects the unavailable OAuth credential state and confirms the resolver is invoked as required by the providerhealth.go flow.
🤖 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 211-230: The authentication flow around profileWithOAuthCredential
must resolve OAuth credentials for every probe, not only when
options.Connectivity is enabled. Invoke it whenever oauthConfigured indicates an
OAuth resolver exists, while preserving resolveErr failure handling and updating
oauthConfigured from the resolver result before credentialRequired and pass/fail
checks.
---
Nitpick comments:
In `@internal/providerhealth/providerhealth_test.go`:
- Around line 183-273: The provider health tests cover OAuth connectivity but
miss the non-connectivity gating path. Add a companion test near
TestProbeConnectivityUsesOAuthCredential that calls Probe with Connectivity
false and an OAuthResolver returning ok=false, then assert the result reflects
the unavailable OAuth credential state and confirms the resolver is invoked as
required by the providerhealth.go flow.
🪄 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: 572e0b03-771d-4544-8d58-14336b0b87a8
📒 Files selected for processing (4)
internal/cli/app.gointernal/cli/observability_test.gointernal/providerhealth/providerhealth.gointernal/providerhealth/providerhealth_test.go
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict
Changes requested. The feature is right and the wiring is correct, but one line in it silently disables redaction of the user's static API key, and I have a reproduction.
Reviewed at 10596d1, base 097c265 (level with origin/main). Author has write access, so this is the internal cycle rather than the issue-approved gate.
What I verified
- The OAuth credential does reach the probe, and
Probepasses the OAuth-mutated profile through toconnectivityCheck(providerhealth.go:239), so the first connectivity request carries the token rather than only the retry. - Redirect-stripping is recomputed after a refresh.
sensitiveAuthHeaderNames(profile, kind)is re-derived from the refreshed profile before the retry client is built (providerhealth.go:321), so the new header name is protected too. That is the easy thing to get wrong here and it is right. - Concurrency: not an issue, checked rather than assumed. I expected trouble from a 401-retry inside a probe that might run concurrently, and there is none — this is a single sequential probe.
- The double
Closeis harmless. The firstdeferbinds the original body by value, and the explicit_ = response.Body.Close()at:314closes it again;http.body.Closeis safe to call twice. Redundant, not a bug. - Gauntlet on darwin/arm64 from a non-
/tmpcheckout:make fmt-check,go vet ./...,git diff HEAD --checkall clean;go test ./internal/providerhealth/ ./internal/cli/ ./internal/providers/...green;go run ./cmd/zero-release buildandsmokeboth pass. - Passes executed: all 12, since this is credential-handling on a network path.
Blocking
-
Clearing
profile.APIKeyremoves the API key from the redaction set, and the provider's response body is echoed into the check message. —internal/providerhealth/providerhealth.go:373The trace, each hop by name:
profileWithOAuthCredentialsetsprofile.APIKey = ""(:373).- That same
profileis whatProbehands toconnectivityCheck(:239), and what everycheck(...)call receives as its last argument. checkredacts viaredact(message, profile)(:767), which builds its secret list fromproviderSecrets(profile)={profile.APIKey, profile.AuthHeaderValue}(:810).- With
APIKeyblanked, the static key is no longer in that list. - The provider's response body is placed directly into the message:
message := responseMessage(response.StatusCode, body)(:358).
So a provider that echoes the offending key in an error body — which several do on 401 — gets it printed verbatim in
zero doctor --connectivity, including--json. I ran both paths against the patched build with a static key that matches no built-in secret pattern, so only the profile-derived redaction could catch it:API-key path : static key leaked = false message: Provider endpoint returned 401: {"error":"invalid key [REDACTED]"} OAuth path (this PR) : static key leaked = true message: Provider endpoint returned 401: {"error":"invalid key STATICKEYVALUE1234567890abcdef"}Same profile, same body, same assertions — the only difference is whether
OAuthResolveris set. This is a regression against currentmainbehaviour for any user who has both a stored API key and an OAuth login.The fix is one line: drop
profile.APIKey = "". Clearing it buys nothing on the request path —ApplyAuthHeaders(internal/providers/providerio/headers.go:36-41) only consultsAPIKeywhenAuthHeaderValueis empty, and writes exactly one auth header — so the OAuth credential still wins. I applied that change and re-ran: the leak closes, and the fullinternal/providerhealthsuite includingTestProbeConnectivityUsesOAuthCredentialstill passes.OAuth path (with fix): static key leaked = false message: Provider endpoint returned 401: {"error":"invalid key [REDACTED]"}If you would rather keep the clear for defence-in-depth, then
providerSecretsneeds to retain the pre-OAuth key instead — but the redaction set must not shrink when OAuth is in play. -
This duplicates
providerio.withBearerandproviderio.SendWithAuthRetry, which already exist and already do exactly this. —internal/providerhealth/providerhealth.go:365-380and:307-330withBearer(internal/providers/providerio/auth.go:80-89) applies an OAuth credential to an auth config with the identical rule set: default the header toAuthorization, clearAPIKey, clearAuthScheme, setAuthHeaderValue.SendWithAuthRetry(:31-77) implements the same resolve-then-retry-once-on-401-with-forceRefreshloop thatconnectivityChecknow open-codes.The two implementations agree today — I compared them field by field, so this is drift risk rather than a present defect. But it is drift risk on the rule that decides which credential leaves the machine, and
withBearer's own comment states the invariant it exists to hold: "the API key is cleared so the two auth methods can never both be sent." Finding 1 is what that duplication already cost, in the form of a redaction property the original author never had to think about because their copy was not used for redaction.Health probing has real reasons not to call
SendWithAuthRetrywholesale (it must not inherit the 429/503 retry budget, and it needs its own SSRF-guarded client), so I am not asking for that. ReusingwithBearer— or exporting an equivalent — for the credential-application half would remove the duplicated rule while leaving the probe's transport alone.
Non-blocking
- The OAuth lookup error is discarded. —
internal/cli/app.go:589.options.OAuthResolver, _ = oauthLoginForProfile(options.Profile)drops the error, so a genuinely broken keyring is indistinguishable from "no OAuth login configured" and the user is told their provider requires API credentials. Diagnostics is precisely the context where that distinction is the answer the user came for. Surfacing it as a warn check would be more useful than the current silence. - The refresh reads
options.Profilewhere everything around it usesprofile. —providerhealth.go:308. These are equivalent today, becauseprofileWithOAuthCredentialoverwrites all four auth fields and nothing else mutates the profile betweenProbeand here. It is a readability trap rather than a bug: a future mutation applied toprofilebefore the connectivity check would be silently dropped on the retry path only. - The explicit close at
:314is redundant given the deferred close bound at:305. Harmless, but it reads as if one of them is load-bearing. hasCredential(profile)is already true by the time!oauthConfiguredis checked. —providerhealth.go:221. OnceprofileWithOAuthCredentialhas setAuthHeaderValue,hasCredentialreturns true (:831), so&& !oauthConfiguredis belt-and-braces. Correct, just worth a comment so nobody removes the wrong half.
Questions for the author
- Was the
profile.APIKey = ""line copied deliberately fromwithBearer, or arrived at independently? It matters for finding 2: if it was copied, the case for reusing the original rather than duplicating it is stronger.
Not covered by this PR
- No test asserts the "never send both credentials" property. I mutated the
APIKeyclear away and the wholeinternal/providerhealthsuite stayed green, which is how finding 1 surfaced. Whichever way finding 1 is resolved, a test that pins the redaction behaviour with both a static key and an OAuth login present is the one worth adding. - The 401-refresh path is exercised for success and for resolver error, but not for a refresh that returns
ok=false. That path falls through and reports the original 401, which is right — it just is not pinned.
Not verified
- The full
go test ./...run. I raninternal/providerhealth,internal/cliandinternal/providers/..., plus the release build and smoke; the whole-repo run exceeded my time budget and I would rather say so than imply it passed. - Any platform other than darwin/arm64.
- A real xAI OAuth profile end to end. The PR description reports that manually; my verification is through the package's own harness with a fake resolver and transport.
2 participants
Summary
Why
OAuth-authenticated providers worked for normal model requests, but
zero doctor --connectivityonly recognized static API keys and auth headers. A valid OAuth login was therefore reported as missing credentials before the connectivity request was attempted.User impact
Doctor and provider-health checks now use the same OAuth credential source as normal provider requests. Users receive the provider's real connectivity result instead of a false missing-credentials failure.
Verification
make fmt-checkgo vet ./...go test ./...go run ./cmd/zero-release buildgo run ./cmd/zero-release smokemake lint-staticmake vulncheckrequires API credentialsSummary by CodeRabbit