models.authStatus: normalize provider ids + tighten env-backed escape hatch#67253
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a98cecf290
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Greptile SummaryThree targeted fixes to Two P2 findings:
Confidence Score: 4/5Safe to merge; the logic fixes are correct and the only concerns are a misleading comment and test no-op that could confuse future maintainers. All three core fixes (ID normalization, envBacked pre-pass, inspect-mode credential check) are logically sound and well-tested. The P2 findings are about comment accuracy and a misleading test setup — neither blocks correctness. Score is 4 rather than 5 only because the inaccurate src/gateway/server-methods/models-auth-status.ts lines 217–221 (misleading comment about inspect mode semantics) Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/gateway/server-methods/models-auth-status.ts
Line: 217-221
Comment:
**Misleading comment — `inspect` does not resolve env vars**
The comment says `inspect` mode "tells us whether the SecretRef currently resolves to a real value." It doesn't: `resolveSecretInputString` with `mode: "inspect"` checks structure only — a literal string → `available`, any `SecretRef` object → `configured_unavailable`, neither → `missing`. It never reads `process.env`, executes a command, or reads a file. A provider configured with `apiKey: { source: "env", id: "LIVE_KEY" }` where `LIVE_KEY` IS set in the environment still gets `configured_unavailable` and therefore falls through to missing synthesis, which would show a false "missing" alert on the dashboard.
If the intent is to only treat inline literals as env-backed (SecretRefs are never trusted here), the comment should say so explicitly rather than suggesting the check is equivalent to env-var presence detection.
```suggestion
// `inspect` mode never throws; it returns `available` only for a
// literal inline string, and `configured_unavailable` for any SecretRef
// shape (regardless of whether the referenced env var is set). Only skip
// the missing synthesis when status is "available" — a SecretRef-based
// apiKey cannot be verified here without runtime secret resolution, so
// it always falls through to normal synthesis.
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/gateway/server-methods/models-auth-status.test.ts
Line: 324
Comment:
**`delete process.env.…` has no effect on this test's assertion**
Because `resolveSecretInputString({ mode: "inspect" })` never reads the environment (it returns `configured_unavailable` for any `SecretRef` shape regardless of env state), removing or setting `MODELS_AUTH_STATUS_TEST_MISSING_KEY` does not change the test outcome. The assertion passes identically with or without this line, which means the test name "when apiKey SecretRef is unresolvable" is not actually verified — a SecretRef with the env var present would produce the same result.
Consider either removing the `delete` (to avoid implying it matters) or adding a complementary test that sets the var and confirms it still triggers missing synthesis, along with a comment explaining that the escape hatch is intentionally limited to literal strings.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "models.authStatus: normalize provider id..." | Re-trigger Greptile |
0df3d94 to
03d1816
Compare
|
Merged via squash as f2fdb9d. Prep changes:
Verification:
|
… hatch (openclaw#67253) Fix false-positive "missing" alerts on the Model Auth status card: - Normalize provider ids before expectsOAuth membership check (alias mismatch) - Apply env-backed escape hatch to auth.profiles loop (not just models.providers) - Check actual env var resolution for SecretRef apiKeys Co-authored-by: Omar Shahine <10343873+omarshahine@users.noreply.github.com>
… hatch (openclaw#67253) Fix false-positive "missing" alerts on the Model Auth status card: - Normalize provider ids before expectsOAuth membership check (alias mismatch) - Apply env-backed escape hatch to auth.profiles loop (not just models.providers) - Check actual env var resolution for SecretRef apiKeys Co-authored-by: Omar Shahine <10343873+omarshahine@users.noreply.github.com>
Follow-up to #66211 addressing three P2 review comments from Codex.
Changes
1. Normalize provider ids before
expectsOAuthmembership check.buildAuthHealthSummarynormalizes provider ids (e.g.z.ai→zai). TheexpectsOAuthset was storing raw config keys, soexpectsOAuthSet.has(prov.provider)silently missed when an alias was configured. Both themodels.providersandauth.profilesloops now store normalized ids.2. Apply env-backed escape hatch to
auth.profilessynthesis too.Previously the env-backed skip only applied to the
models.providersloop, so a config with resolvableapiKey+ a matchingauth.profilesmode entry re-added the provider and flipped it back to "missing". Escape hatch is now captured once up front (envBackedset) and honored by both loops.3. Check env secret resolution before treating apiKey as valid credential.
hasEnvCredential = apiKey !== undefinedwas too permissive — a SecretRef pointing at an unset env var would silently count as env-backed and suppress the "missing" alert for a genuinely broken config. Now usesresolveSecretInputString({ mode: \"inspect\" })and only skips when status isavailable. Aconfigured_unavailableSecretRef falls through to normal missing synthesis so the dashboard surfaces it.Tests
Three new tests, all passing (18 total in the suite):
Context
Shipped in `2026.4.15-beta.1` as part of #66211; the original merge happened without human review (auto-added `@openclaw/secops` reviewer slot wasn't enforced). Cc'ing the team so the combined surface can get a proper secops look:
cc @openclaw/secops