fix: show external Codex usage without inflating OpenCode Zen#119
fix: show external Codex usage without inflating OpenCode Zen#119kargnas merged 6 commits intoopgginc:mainfrom
Conversation
…le /v1 URL injection Agent-Logs-Url: https://github.com/Daltonganger/opencode-bar/sessions/2672ff4d-23d5-4e27-9d06-5c3ec83b03b4 Co-authored-by: Daltonganger <17501732+Daltonganger@users.noreply.github.com>
✅ AI Code Review Completed
Review posted successfully — APPROVED with 6 minor inline comments. Reviewed with |
![op-gg-ai-devops[bot]](https://avatars.githubusercontent.com/in/2680015?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix: show external Codex usage without inflating OpenCode Zen
Solid contribution from an external contributor. This PR does three things well:
-
Self-service
/v1/usageendpoint for API-key Codex accounts — clean routing logic with proper URL path rewriting, flexible decoding for mixed API response shapes, and a sensible partition of base vs spark limits. TheSelfServiceLimitdecoder is defensive and handles all the weird type coercion this project's APIs love to throw. Good stuff. -
OpenCode Zen cost deduction — removing
openai/*model costs from the pay-as-you-go total when OpenCode routes through an external Codex LB prevents double-counting. The proportionalavgCostPerDayadjustment is mathematically sound. -
OpenAI API key auth flow — reading
openaiAPIKeyfrom bothauth.json(asAPIKeystruct) and~/.codex/auth.json(as rawOPENAI_API_KEYstring) expands auth coverage cleanly.
Tests are thorough — URL rewriting, self-service payload mapping, missing limits, and the OpenCode Zen exclusion logic all have focused test cases. CI is green across build, lint, and test.
A few minor things to consider below, but nothing blocking.
Reviewed with anthropic/claude-opus-4-6
The bot will automatically re-review when new commits are pushed to this branch. You can also request a review by mentioning @op-gg-ai-devops in a comment. Examples:
@op-gg-ai-devops review typos@op-gg-ai-devops focus on security@op-gg-ai-devops check the URL rewriting logic
❌ AI Code Review CancelledNewer commit detected or workflow was manually cancelled. 📋 View Logs | 🤖 Model: |
|
@Daltonganger there is a conflict! |
|
I will have a look! Thank you for the double check, Im still learning from it! |
Keep the safer OpenCode OpenAI auth decoding from main while preserving the PR's API-key and JSONC config coverage for Codex usage.
✅ AI Code Review Completed
Reviewed with |
There was a problem hiding this comment.
Review: fix: show external Codex usage without inflating OpenCode Zen (re-review)
All six inline comments from the previous round were addressed cleanly. The codexWindowMetadata helper deriving window labels from limit_window_seconds is exactly the right approach, sameUsageValue with epsilon comparison is solid, and the subscriptionId doc comment is back where it belongs. The double-/v1 URL injection fix is well-protected by the hasSuffix("/v1/usage") early-exit guard, and the SelfServiceLimit decoder handles mixed numeric types gracefully.
Two things caught my attention in this re-review:
1. formatCodexWindowLabel threshold vs. test expectation
compactWindowLabel(2419200) returns "4w" (exactly 4 weeks), which is passed into formatCodexWindowLabel. That function reaches the hours path: 4 × 24 × 7 = 672 hours → 28 days → hits if days >= 28 { return "Monthly" } and returns "Monthly". But testDecodeUsagePayloadDerivesStandardWindowLabelsFromLimitSeconds asserts sparkSecondaryWindowLabel == "28d" for the 2419200-second window. These two disagree — the test would fail if actually run. Either the threshold should be >= 30 (to reserve "Monthly" for 30-31 day periods and let 28-day windows render as "28d"), or the test expectation should be updated to "Monthly". Based on the test intent, >= 30 looks correct.
2. CI "Test" job doesn't actually run tests
test.yml has two xcodebuild build steps but zero xcodebuild test steps. The green "Test" CI badge only confirms the project compiles, not that any XCTest assertion passes. This is how the threshold mismatch above slipped through: no tests were executed. Adding xcodebuild test -scheme CopilotMonitor -destination 'platform=macOS' would catch regressions like this.
The isEmpty != false optional comparison in TokenManager is a minor style nit — see inline.
Note: Smoke tests could not be run in the review environment (no Xcode available on Linux runner). CI build results were used as the build signal.
| if hours % 24 == 0 { | ||
| let days = hours / 24 | ||
| if days == 7 { return "Weekly" } | ||
| if days >= 28 { return "Monthly" } |
There was a problem hiding this comment.
compactWindowLabel(2419200) produces "4w", and then formatCodexWindowLabel("4w") computes 4 × 24 × 7 = 672 hours → 28 days → hits this branch and returns "Monthly". But the test at line 468 of CodexProviderTests.swift asserts sparkSecondaryWindowLabel == "28d" for that same input. The test would fail at runtime. Change to >= 30 to keep 28-day (4-week) windows as "28d":
if days >= 30 { return "Monthly" }| XCTAssertEqual(payload.details.codexSecondaryWindowHours, 336) | ||
| XCTAssertEqual(payload.details.sparkPrimaryWindowLabel, "12h") | ||
| XCTAssertEqual(payload.details.sparkPrimaryWindowHours, 12) | ||
| XCTAssertEqual(payload.details.sparkSecondaryWindowLabel, "28d") |
There was a problem hiding this comment.
This test expects "28d" for limit_window_seconds: 2419200 (= 4 × 604800, exactly 4 weeks), but formatCodexWindowLabel would return "Monthly" because 2419200 → compactWindowLabel → "4w" → 672h → 28 days → days >= 28 → "Monthly". The assertion will fail when tests are actually run. See the corresponding fix in CodexProvider.swift:842.
| } | ||
|
|
||
| if let codexAuth = readCodexAuth(), | ||
| codexAuth.tokens?.accessToken?.isEmpty != false, |
There was a problem hiding this comment.
The != false comparison on an Optional<Bool> is correct but hard to read at a glance. Prefer:
(codexAuth.tokens?.accessToken ?? "").isEmptyThis returns true when accessToken is nil or empty, and false when it has a real value — the same semantics but obviously so.
|
@op-gg-ai-devops 스크린샷 보여줘 |
✅ AI Code Review Completed
Reviewed with |
There was a problem hiding this comment.
Review Summary
Codex self-service API key 인증 지원과 OpenCode Zen 이중 과금 방지 로직 전반적으로 잘 짜여있음. OpenAICredentialType enum 설계도 깔끔하고, codexWindowMetadata 헬퍼로 window label 추출하는 것도 적절한 리팩토링임.
좋은 점:
SelfServiceUsageResponse의 유연한 타입 디코딩 (parseFlexibleDateString, numeric field fallbacks) — 외부 API 스키마가 불안정할 때 이런 방어적 디코딩이 필수적임adjustStatsForDisplay의 static 설계로 테스트 용이성 확보한 점buildStandardPayload의baseWindows?.shortWindow ?? primaryWindow ?? synthetic zerofallback 체인이 resilience 측면에서 좋음- 테스트 커버리지 추가가 의미 있음 — 특히
CodexProviderTests에 8개 새 케이스
주요 지적사항:
formatCodexWindowLabel에서days >= 28을 "Monthly"로 매핑하는데, 테스트는 28일 window에 "28d"를 기대함 — CI가xcodebuild test를 실행하지 않아서 잡히지 않고 있음- Self-service limit 배열 디코딩이 all-or-nothing이라 한 원소만 깨져도 전체 limit이 사라짐
- Usage percent가 100으로 하드캡되어 overage 표시 불가 (standard path와 불일치)
CI 상태: Lint ✅ | Build ✅ | Test ✅ | CI ✅ (단, Test는 build만 하고 실제 unit test는 실행하지 않음)
Visual Changes
📸 스크린샷 캡처 실패: 이 PR은 macOS 네이티브 Swift 앱의 백엔드/데이터 레이어 변경사항(CodexProvider, OpenCodeZenProvider, TokenManager)만 포함하고 있어서 웹 UI 스크린샷 캡처가 불가능함. 변경된 파일이 모두 .swift 파일이며, UI 컴포넌트 변경은 없음.
Avoid treating numeric 28-31 day windows as monthly so Codex usage labels remain exact across February and other month lengths. Also run real XCTest coverage in CI and align merge-related TokenManager call sites with the updated models.
✅ AI Code Review Completed
Reviewed with |
There was a problem hiding this comment.
Review: fix: keep Codex day windows explicit
nice work here — the self-service API key path, flexible decoding, and OpenCode Zen cost dedup are all well thought out. the DecodedUsagePayload extraction is clean and the adjustStatsForDisplay logic is a solid approach to avoid double-counting routed OpenAI usage.
a few things worth addressing:
good stuff:
decodeFlexibleDouble/decodeFlexibleDatehandle the typical API type chaos gracefully (Double, Int, String, timestamps vs ISO8601). solid defensive coding.buildSelfServicePayloadandbuildStandardPayloadsplit is clean — keeps the two response formats from contaminating each other.sameUsageValuewith epsilon comparison is the right call for floating-point dedup.- CI workflow
set -o pipefailfix means actual build failures won't silently pass anymore. good catch. - test coverage for the new URL rewriting, self-service payload decoding, and stats adjustment is thorough.
heads up:
- the
limitsarray decoding swallows the entire array if any single entry fails — one bad limit kills all valid ones. see inline comment. - self-service path caps usage at 100% while standard path allows raw percentages through. inconsistent behavior for overage scenarios.
ISO8601DateFormatteris expensive to allocate and gets created fresh on everyparseFlexibleDateStringcall.- the
codexAuth.tokens?.accessToken?.isEmpty != falseguard in TokenManager works correctly but reads like a logic puzzle.
all CI checks are green (Build, SwiftLint, GitHub Actions Lint, Build & Test all pass). could not run local smoke tests since this is a macOS Xcode project and review runs on Linux.
This branch will be automatically reviewed again on new pushes. You can also request a re-review by mentioning @op-gg-ai-devops in a PR comment with optional directions, e.g.:
@op-gg-ai-devops review typos@op-gg-ai-devops focus on error handling@op-gg-ai-devops check the test coverage
Reviewed with anthropic/claude-opus-4-6|max
| totalTokens = try container.decodeIfPresent(Int.self, forKey: .totalTokens) | ||
| cachedInputTokens = try container.decodeIfPresent(Int.self, forKey: .cachedInputTokens) | ||
| totalCostUSD = try container.decodeIfPresent(Double.self, forKey: .totalCostUSD) | ||
| limits = (try? container.decodeIfPresent([SelfServiceLimit].self, forKey: .limits)) ?? [] |
There was a problem hiding this comment.
(try? container.decodeIfPresent([SelfServiceLimit].self, forKey: .limits)) ?? [] drops the entire array if any single element fails to decode. one malformed limit entry (e.g. unexpected type for max_value) silently wipes all valid windows.
consider a lossy per-element approach:
let rawLimits = (try? container.decodeIfPresent([AnyCodable].self, forKey: .limits)) ?? []
limits = rawLimits.compactMap { element in
try? JSONDecoder().decode(SelfServiceLimit.self, from: JSONEncoder().encode(element))
}or use a LossyDecodableArray<SelfServiceLimit> wrapper so valid entries survive alongside broken ones.
| } | ||
|
|
||
| let rawPercent = (currentValue / maxValue) * 100.0 | ||
| let usagePercent = min(max(rawPercent, 0), 100) |
There was a problem hiding this comment.
min(max(rawPercent, 0), 100) caps self-service usage at 100%, but buildStandardPayload passes raw used_percent straight through without clamping. if the API reports >100% during overage, the standard path shows it while self-service hides it.
either cap both paths consistently or let both pass raw values through — the display layer should decide how to present overages.
| } | ||
|
|
||
| if let codexAuth = readCodexAuth(), | ||
| codexAuth.tokens?.accessToken?.isEmpty != false, |
There was a problem hiding this comment.
codexAuth.tokens?.accessToken?.isEmpty != false is a triple-optional-negation chain that correctly means "proceed when there's no usable OAuth token" — but it reads like a logic puzzle. a future maintainer could easily misread or break this.
something like this would be self-documenting:
let hasOAuthToken = codexAuth.tokens?.accessToken.map { !$0.isEmpty } ?? false
guard !hasOAuthToken,| : Date(timeIntervalSince1970: timestamp) | ||
| } | ||
|
|
||
| let formatterWithFractional = ISO8601DateFormatter() |
There was a problem hiding this comment.
ISO8601DateFormatter() is expensive to allocate — two new instances get created on every parseFlexibleDateString call. for a provider that fetches periodically, this adds up.
hoist them to static properties:
private static let isoFormatterWithFrac: ISO8601DateFormatter = {
let f = ISO8601DateFormatter()
f.formatOptions = [.withInternetDateTime, .withFractionalSeconds]
return f
}()
private static let isoFormatter: ISO8601DateFormatter = {
let f = ISO8601DateFormatter()
f.formatOptions = [.withInternetDateTime]
return f
}()
Summary
/v1/usagesupport for external OpenAI API-key Codex accounts and map the returned limits into Codex quota dataopenai/*spend as pay-as-you-go when OpenCode routes OpenAI throughprovider.openai.options.baseURLTesting
make lint-swiftxcodebuild -project "CopilotMonitor.xcodeproj" -scheme "CopilotMonitor" -configuration Debug build -quiet