Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20efb95c0d
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let headers = httpResponse.allHeaderFields | ||
| let remaining = Self.intHeader(headers, "x-ratelimit-remaining-requests") | ||
| let limit = Self.intHeader(headers, "x-ratelimit-limit-requests") | ||
| let resetString = headers["x-ratelimit-reset-requests"] as? String |
There was a problem hiding this comment.
Parse reset header case-insensitively
HTTPURLResponse.allHeaderFields lookups are case-sensitive, but this code only checks the lowercase key for the reset header. If the server returns X-RateLimit-Reset-Requests (or any other casing), resetString becomes nil even though the header is present, so the app loses reset-time information in normal responses. Reusing the same case-insensitive header lookup used for remaining/limit avoids this data loss.
Useful? React with 👍 / 👎.
Use a dedicated stringHeader helper (case-insensitive) for the x-ratelimit-reset-requests header, matching how remaining/limit headers are already parsed. Addresses Codex review feedback on PR steipete#498.
|
Thanks for the PR @LeoLin990405 ! I think there’s one real correctness issue and one risky compatibility spot:
Also worth checking: on 429, |
1. Reset header lookup is now case-insensitive (new stringHeader helper), matching the existing intHeader behavior. 2. On 429 (rate-limited), apiKeyValid is set to true so the UI shows "Active" instead of "No usage data" when rate-limit headers are absent. 3. Probe multiple fallback models instead of hardcoding a single model, so keys with different entitlements or regions still work.
|
Thanks for the thorough review, @ratulsarna! I've pushed a fix addressing all three points: Changes in 3a548ce1. Reset header case-insensitivity 2. 429 → 3. Model fallback list
If a model returns 403/404 (not entitled or not found), the next model is tried. This handles the "valid key, wrong model entitlement/region" scenario you flagged.
Good catch — I kept the Beijing endpoint as the default since DashScope's compatible-mode API routes through Beijing for most regions. If there's demand for explicit region support, that could be a follow-up (e.g. reading a |
|
Thanks for the thorough review, @ratulsarna! Really appreciate you catching these — all three are valid issues. I've pushed a fix in Fix 1: Reset header case-insensitivityThe problem: The fix: Added a // Before (case-sensitive — could miss headers with different casing):
let resetString = headers["x-ratelimit-reset-requests"] as? String
// After (case-insensitive — consistent with intHeader):
let resetString = Self.stringHeader(headers, "x-ratelimit-reset-requests")Applied to both Fix 2: Treat 429 as "key is valid"The problem: On HTTP 429 (rate-limited), The fix: Treat both 200 and 429 as valid-key responses: // Before:
apiKeyValid: httpResponse.statusCode == 200
// After:
let keyValid = httpResponse.statusCode == 200 || httpResponse.statusCode == 429
apiKeyValid: keyValidNow when a 429 comes back without rate-limit headers, the UI correctly shows "Active — check dashboard for details" instead of the confusing "No usage data". Fix 3: Multi-model probe with fallbackThe problem: Both fetchers hardcoded a single probe model ( The fix: Try a list of models sequentially, falling back on 403/404:
for model in self.probeModels {
do {
return try await self.probe(apiKey: apiKey, model: model)
} catch let error as QwenUsageError {
// Model not entitled or not found → try the next one
if case let .apiError(code, _) = error, code == 404 || code == 403 {
Self.log.debug("Qwen probe model \(model) unavailable (\(code)), trying next")
lastError = error
continue
}
throw error // Non-model errors (network, 401 auth) propagate immediately
}
}This ensures we only retry on model-specific failures, while auth errors and network issues are surfaced right away without wasting retries. A note on the region concern
I kept the Beijing Build verificationCI build passes on macOS with Xcode (full Xcode toolchain, not just command-line tools): ✅ Build succeeded — Run #23040849961
All steps green. Let me know if you'd like any further changes! |
Summary
Both are popular Chinese AI coding platforms with growing user bases.
Implementation
max_tokens: 1) to read rate-limit headerssk-sp-*coding plan keys vs regular API keys, routes to appropriate endpoint/api/coding/v3/chat/completionsendpoint withdoubao-seed-2.0-codemodelTest plan
codexbar --provider qwenandcodexbar --provider doubaoshow usage