Fix provider fetch stability issues - #143
Conversation
✅ AI Code Review CompletedReview finished. Check the PR for inline comments. 📋 View Logs | 🤖 Model: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e161afafeb
ℹ️ 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".
| limits = (try? container.decodeIfPresent([SelfServiceLimit].self, forKey: .limits)) ?? [] | ||
| let localLimits = (try? container.decodeIfPresent([SelfServiceLimit].self, forKey: .limits)) ?? [] | ||
| let upstreamLimits = (try? container.decodeIfPresent([SelfServiceLimit].self, forKey: .upstreamLimits)) ?? [] | ||
| limits = localLimits + upstreamLimits |
There was a problem hiding this comment.
When a codex-lb API-key response contains both per-key limits and account-level upstream_limits, concatenating them into one untagged list makes partitionSelfServiceLimits(...).base.first/last choose a single primary/secondary window from the mixed set. If the key has its own weekly or monthly budget, the dropdown can show that API-key budget instead of the upstream 5h/7d Codex account quota this change is trying to expose; keep the two sources separate or prefer upstream windows for the Codex quota display.
Useful? React with 👍 / 👎.
![op-gg-ai-devops-refugee[bot]](https://avatars.githubusercontent.com/in/4025946?s=60&v=4){kind=small}
There was a problem hiding this comment.
Provider fetch stability — solid core, two risky shortcuts
Honestly the OpenCodeZen rework is the best part of this PR and it's properly done. I traced every exit path of runOpenCodeStats:
withCheckedThrowingContinuationdoesn't observe Task cancellation, so a hungopencode statsused to leak the continuation until the process died on its own. The newfinish()(NSLock +didResumeguard) + theasyncAftertimeout thatterminate()s thenSIGKILLs the pid actually fixes that. The lock guarantees exactly oneresume, and the lateterminationHandlerafter a timeout is a clean no-op. No double-resume, no leak. Nice.- Draining
psstdout beforewaitUntilExit()removes a real pipe-buffer deadlock. Good catch. etimes->etimeis the right fix — macOSpshas noetimeskeyword, so the old code was silently parsing nothing on macOS.parsePSElapsedSecondshandles[[dd-]hh:]mm:sscorrectly.
The Z.AI retry/backoff + 30s fetchTimeout + isTransientNetworkError is also a clean, on-plan stability win.
That said, two changes trade an honest failure for a possibly-wrong result, and I can't sign off on those as-is.
Blocking
"default"GCP project (Gemini + Antigravity). Both providers now POST{"project":"default"}toretrieveUserQuotawhen the real project id is missing."default"isn't a real GCP project, and per the existing convention (and the comment you just softened) theprojectparam is required to get full model coverage incl. gemini-3. Best case the API rejects it and Antigravity — which used to returnnilcleanly — does extra work to fail anyway; worst case it returns a reduced/default quota set and the menu shows plausible-but-wrong numbers with no error. The previousthrow/nilwas the correct behavior. Either keep the hard failure or verify against the live API that"default"returns complete data (and surface a visible "partial data" state).- Codex
limits + upstream_limitsconcat.partitionSelfServiceLimitsnever dedupes, andbuildSelfServicePayloadreadsbase.first/base.last. If both arrays carry the same window you either render it twice (primary == secondary) or silently drop one source's value. Dedupe by window key before partitioning.
CI is red
SwiftLint is failing on duplicate_imports because of the duplicated import Foundation in ZaiCodingPlanProvider.swift. That one-line fix unblocks the lint job.
Non-blocking / FYI
- The new
parsePSElapsedSecondsandisTransientNetworkErrorare deterministic, edge-case-prone helpers and the repo already hasOpenCodeZenProviderTests/ZaiCodingPlanProviderTests— a couple of table-driven tests ("03:14","1:02:03","2-01:05:30", malformed; transient vs non-transient) would be cheap insurance. - OpenCodeZen's process-timeout/kill logic now overlaps with KiroProvider's task-group approach. Not blocking, but a shared
runProcessWithTimeouthelper would stop the two from drifting. getOpenAIAccountsprepending a config-derived API-key account is dedupe-safe against the same key inauth.json(token:<key>), but it renders with no email/accountId — confirm it won't show a confusing extra "Unknown" row next to a real OAuth Codex login.
Inline comments have the specifics.
| throw ProviderError.authenticationFailed("Missing project ID for account #\(accountIndex + 1)") | ||
| } | ||
| let configuredProjectId = account.projectId.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| let projectId = configuredProjectId.isEmpty ? "default" : configuredProjectId |
There was a problem hiding this comment.
"default" isn't a valid GCP project id, and this used to throw when the project was missing. Now you POST {"project":"default"} to retrieveUserQuota. The existing convention (and the comment you softened on line 286) says project is required for full model coverage including gemini-3 variants — so this can silently return a reduced quota set and the menu shows wrong-but-plausible numbers with no error.
Keep the hard failure when the project id is genuinely missing, or verify against the live API that "default" returns complete data and surface a visible "partial data" indicator. Either way please restore the accurate "required" wording in the comment.
| limits = (try? container.decodeIfPresent([SelfServiceLimit].self, forKey: .limits)) ?? [] | ||
| let localLimits = (try? container.decodeIfPresent([SelfServiceLimit].self, forKey: .limits)) ?? [] | ||
| let upstreamLimits = (try? container.decodeIfPresent([SelfServiceLimit].self, forKey: .upstreamLimits)) ?? [] | ||
| limits = localLimits + upstreamLimits |
There was a problem hiding this comment.
Concatenating limits and upstream_limits without dedupe is risky: partitionSelfServiceLimits sorts but never dedupes, and buildSelfServicePayload reads base.first/base.last. If both arrays describe the same window (e.g. both carry a 5h entry), you either render the same window twice as primary+secondary, or silently keep one source's value and drop the other's.
Dedupe on merge by a window key (limitWindow + modelFilter/limitType), preferring one source deterministically, before partitioning — or document why overlap is impossible.
| } else if !fallbackProjectId.isEmpty { | ||
| projectId = fallbackProjectId | ||
| } else { | ||
| projectId = "default" |
There was a problem hiding this comment.
Same "default" concern as in GeminiCLIProvider. This path previously returned nil (no fallback). Substituting a literal "default" project id means the fallback either fails anyway after extra work, or returns a default-tier quota that gets shown as if it were real. If "default" is not verified to return complete data from retrieveUserQuota, keep returning nil here.
| @@ -1,4 +1,5 @@ | |||
| import Foundation | |||
| import Foundation | |||
There was a problem hiding this comment.
Duplicate import Foundation. This is what's failing the SwiftLint CI job (duplicate_imports). Drop this line to go green.
| let components = timePart.split(separator: ":").compactMap { Int($0) } | ||
| guard components.count == timePart.split(separator: ":").count else { return nil } |
There was a problem hiding this comment.
timePart.split(separator: ":") is computed twice just to validate all segments parsed. Split once into a local and compare counts:
let rawComponents = timePart.split(separator: ":")
let components = rawComponents.compactMap { Int($0) }
guard components.count == rawComponents.count else { return nil }
✅ AI Code Review CompletedReview finished. Check the PR for inline comments. 📋 View Logs | 🤖 Model: |
![opgg-ai-devops[bot]](https://avatars.githubusercontent.com/in/2680015?s=60&v=4){kind=small}
There was a problem hiding this comment.
Feedback round fully addressed — approving
Went back through every point raised in the prior review round against the actual code in d0fa565, not just the commit message. All three blocking issues are genuinely fixed, not just softened:
- Codex
limits+upstream_limits: no longer concatenated. The decoder now dedupes by(limitWindow, modelFilter, limitType)and prefersupstream_limitswholesale when present (falls back tolimitsonly when upstream is empty) — exactly the "prefer upstream windows" fix Codex's own review asked for.partitionSelfServiceLimits/buildSelfServicePayloadconsume the already-cleanresponse.limits, so no double-primary/secondary risk. Covered by a new decode test with duplicate 5h upstream entries. - Gemini
"default"project fallback: fully reverted.fetchQuotaForAccountnow hard-throws again on an empty project id (no more silent"default"POST), and the "project is required" comment is restored.AntigravityProvider.swiftnets to zero diff across both commits — confirmed it's back to the originalnil-on-missing-project behavior. - SwiftLint
duplicate_imports: the duplicateimport FoundationinZaiCodingPlanProvider.swiftis gone — verified directly against the file.
Non-blocking items were also picked up: the Gemini OAuth fallback is now guarded by shouldRetryWithGeminiCLIClient so it only retries when the client actually differs (with a unit test), and the elapsed-time parser no longer splits timePart twice. Nice touch adding table-driven tests for parsePSElapsedSeconds and isTransientNetworkError — exactly what was asked for as "cheap insurance."
The OpenCodeZenProvider process-timeout rework (NSLock + didResume guard, terminate() → delayed SIGKILL) still holds up — traced the timeout path and the terminationHandler-after-timeout race is a clean no-op via the guard.
One tiny architectural note, not blocking: SelfServiceUsageResponse, SelfServiceLimit, and sourceSummary in CodexProvider had to drop from private to internal visibility purely so the new tests (a separate module) can reach them. That's a reasonable tradeoff given Swift's @testable import only exposes internal, not private — no action needed, just flagging the slightly wider surface for awareness.
Build/lint verification: this is a macOS/Xcode-only project (no xcodebuild/swiftlint available in this Linux review environment), so I verified the fixes by direct code/diff inspection rather than running the actual build or SwiftLint. Worth confirming CI is green once it runs on this fork.
No new correctness, security, or design issues found in this round. Approving.
d0fa565 to
1b6c271
Compare
✅ AI Code Review CompletedReview finished. Check the PR for inline comments. 📋 View Logs | 🤖 Model: |
There was a problem hiding this comment.
Provider fetch stability — clean fixes, one fragility concern
Went through both the new commits on top of d0fa565 and the full 10-file diff. The prior review's blocking concerns are genuinely addressed — Gemini "default" project fallback is fully reverted with a hard guard throw, Codex limits + upstream_limits is properly deduped by a composite key, and the duplicate import is gone. Nice.
The Gemini CLI OAuth fallback (shouldRetryWithGeminiCLIClient) is precise — only retries when the stored client differs from geminiClientId, exactly what was asked for. The Codex upstream-limits dedup logic picks the right source. And the stale-cleanup termination-status guard in OpenCodeZen is cheap defensive hygiene that prevents garbage-parsing edge cases.
That said, the new isTransientNetworkError in Z.AI's retry path has a fragility issue that should get cleaned up — string-matching on error messages for HTTP status codes is a ticking time bomb when the format inevitably drifts. The two independent code paths (ProviderError switch + NSURLError switch) also have no integration; a ProviderError wrapping an NSURLError silently bypasses the TLS checks. Both are unlikely to bite today but will bite eventually.
What's in this round
| File | What changed |
|---|---|
| CodexProvider | upstream_limits decoding + dedup, visibility changes for tests, OpenAI config API key source |
| GeminiCLIProvider | shouldRetryWithGeminiCLIClient guard, restored hard-throw on missing project ID |
| OpenCodeZenProvider | Termination status guard + debug log on stale process cleanup |
| ZaiCodingPlanProvider | Retry-with-backoff for transient errors, isTransientNetworkError classifier |
| TokenManager | getOpenAIProviderAPIKeyWithSource(), geminiClientId made static, config API key appended to getOpenAIAccounts() |
| Tests | Table-driven tests for parseETimeSeconds, isTransientNetworkError, Codex upstream dedup, Gemini OAuth guard, config source label |
PR description drift
The PR description still mentions "Antigravity fallbacks for missing project IDs" — but the diff has zero Antigravity changes. (The prior review round confirmed AntigravityProvider.swift nets to zero diff.) If Antigravity work is already on the base branch, update the description to say so. If it was planned and omitted, remove the claim. Same minor mismatch with "adding an internal opencode stats timeout/terminate/kill path" — the visible diff is only the stale-cleanup termination-status guard + debug log.
Shared-code duplication (pre-existing, surfaced by visibility changes)
mergeSourceLabels is copy-pasted identically across CodexProvider, ClaudeProvider, GeminiCLIProvider, and TokenManager. sourceSummary is duplicated across CodexProvider, ClaudeProvider, and GeminiCLIProvider. This PR made CodexProvider's copy internal for testing — it's now the de-facto "canonical" public copy while the others stay locked-away duplicates. A future display-format change would require updating 3+ sites. Worth extracting into a shared utility (not blocking, just calling it out while the diff is fresh).
Smoke tests
macOS/Xcode project on a Linux runner — no xcodebuild or swiftlint available locally. CI shows SwiftLint ✅ green, GitHub Actions Lint ✅ green, Build is still in progress. No runtime smoke tests possible from this environment.
Inline findings
3 inline comments covering the actionable items — check 'em.
✅ AI Code Review CompletedReview finished. Check the PR for inline comments. 📋 View Logs | 🤖 Model: |
There was a problem hiding this comment.
Third round — both prior concerns cleanly addressed
Quick delta review against the three files changed since the last round. The author nailed both remaining concerns from the prior review:
Codex dedup first-wins semantics ✅
The deduplicate function now has a clear comment explaining that the selected source (upstream or local fallback) preserves its JSON array order, and the first entry in that order remains the primary value for a duplicate window. Exactly what was asked for — no ambiguity left for future readers.
Z.AI isTransientNetworkError ProviderError/NSURLError gap ✅
The fix genuinely bridges the gap instead of papering over it:
isTransientURLError()walksNSUnderlyingErrorKeychain (up to 8 levels deep) so wrappedNSURLErrors from any layer get caught- Called before the string-matching fallback, so proper error codes take priority
- Added
NSURLErrorNotConnectedToInternetto the transient set (good catch) - Test coverage for wrapped errors (
wrappedTimeout) and the new codes
The string-matching on "HTTP 5" / "TLS" is still there as defense-in-depth, but it's now the secondary path. The coupling is within the same file (the error message is constructed in fetchDataOnce 40 lines above), so drift is unlikely.
Retry jitter ✅
retryDelayNanoseconds(for:jitter:) uses UInt64.random(in: 0...250_000_000) as the default jitter — fresh random on every call, no sync bursts. Tests verify with explicit jitter values.
CI
SwiftLint ✅ | GitHub Actions Lint ✅ | Build ✅ | Build & Test pending
No new issues, no design concerns, no regressions. Both prior blocking threads are genuinely resolved. Approving.
📝 macOS/Xcode project on Linux runner — no local build/lint/smoke possible. Verified by diff inspection. CI is green so far.
1 participant
Summary
opencode.jsoncso custom Codex-compatible endpoints likecodex.2631.eucan be queried correctly, and decodeupstream_limitsusage windows.opencode statstimeout/terminate/kill path and fixing stale process cleanup for macOSps etimeoutput.Verification
make setupgit diff --checkxcodebuild -project CopilotMonitor/CopilotMonitor.xcodeproj -scheme CopilotMonitor -configuration Release -destination 'generic/platform=macOS' -derivedDataPath build/DerivedData-pr-provider-fetch-stability CODE_SIGNING_ALLOWED=NO buildx86_64 arm64, version/build2.11.1/2.11.1.opencodebar-cli provider zai_coding_plan --jsonopencodebar-cli provider chatgpt --jsonopencodebar-cli provider gemini --jsonopencodebar-cli provider antigravity --jsonopencodebar-cli provider opencode_zen --json