feat: add codex priority billing source setting#960
Conversation
📝 WalkthroughWalkthrough本 PR 在系统设置中新增 Changes
代码评审工作量估算🎯 4 (复杂) | ⏱️ ~45 分钟 可能相关的 PR
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
github-actions
Bot
added
size/XL
🧪 测试结果
总体结果: ✅ 所有测试通过 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new system setting that allows administrators to choose whether Codex Priority billing should be based on the requested or actual service tier. This change ensures flexibility in billing practices and maintains compatibility with existing systems while providing a smooth transition for users. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new system setting codex_priority_billing_source to control billing for Codex Priority requests. The changes are comprehensive, touching the database schema, API, UI, and core proxy logic. The implementation includes robust backward compatibility for older database schemas by gracefully handling missing columns during reads and writes, which is an excellent addition. The billing logic has been refactored into the ProxySession class, improving code organization and encapsulation. The new feature is also well-covered by new integration and unit tests. Overall, this is a high-quality contribution. I have one minor suggestion to improve logging for consistency.
| const codexPriorityBillingSource = | ||
| systemSettings.codexPriorityBillingSource === "actual" ? "actual" : "requested"; | ||
|
|
||
| if (billingModelSource !== systemSettings.billingModelSource) { | ||
| logger.warn( | ||
| `[ProxySession] Invalid billingModelSource: ${String(systemSettings.billingModelSource)}, fallback to "redirected"` | ||
| ); | ||
| } |
There was a problem hiding this comment.
For consistency and better debugging, it's good practice to add a warning log for an invalid codexPriorityBillingSource value, similar to how it's done for billingModelSource. This will help identify if an unexpected value is ever present in the database. I've also made the validation slightly more robust to explicitly check for valid enum values.
const codexPriorityBillingSource =
systemSettings.codexPriorityBillingSource === "actual" ||
systemSettings.codexPriorityBillingSource === "requested"
? systemSettings.codexPriorityBillingSource
: "requested";
if (billingModelSource !== systemSettings.billingModelSource) {
logger.warn(
`[ProxySession] Invalid billingModelSource: ${String(systemSettings.billingModelSource)}, fallback to "redirected"`
);
}
if (codexPriorityBillingSource !== systemSettings.codexPriorityBillingSource) {
logger.warn(
`[ProxySession] Invalid codexPriorityBillingSource: ${String(systemSettings.codexPriorityBillingSource)}, fallback to "requested"`
);
}There was a problem hiding this comment.
已修复，见 a00c88e：补了 codexPriorityBillingSource 的显式白名单校验和 fallback warning。
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -0,0 +1 @@ | |||
| ALTER TABLE "system_settings" ADD COLUMN "codex_priority_billing_source" varchar(20) DEFAULT 'requested' NOT NULL; No newline at end of file | |||
There was a problem hiding this comment.
The file ends without a trailing newline (\ No newline at end of file in the diff). While Drizzle and most PostgreSQL clients handle this fine at runtime, POSIX convention and many linters expect text files to end with a newline. Consider adding one to keep consistent with the rest of the migration files.
| ALTER TABLE "system_settings" ADD COLUMN "codex_priority_billing_source" varchar(20) DEFAULT 'requested' NOT NULL; | |
| ALTER TABLE "system_settings" ADD COLUMN "codex_priority_billing_source" varchar(20) DEFAULT 'requested' NOT NULL; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: drizzle/0085_busy_ken_ellis.sql
Line: 1
Comment:
**Missing trailing newline**
The file ends without a trailing newline (`\ No newline at end of file` in the diff). While Drizzle and most PostgreSQL clients handle this fine at runtime, POSIX convention and many linters expect text files to end with a newline. Consider adding one to keep consistent with the rest of the migration files.
```suggestion
ALTER TABLE "system_settings" ADD COLUMN "codex_priority_billing_source" varchar(20) DEFAULT 'requested' NOT NULL;
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![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: b70aa558c2
ℹ️ 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".
| : "redirected", | ||
| codexPriorityBillingSource: | ||
| hasPersistedCachedSettings && cachedSettings.codexPriorityBillingSource === "actual" | ||
| ? "actual" | ||
| : "requested", |
There was a problem hiding this comment.
If getSystemSettings() throws before the in-memory cache has been warmed (for example right after a restart during a transient DB outage), this block silently switches billing to redirected/requested. updateRequestCostFromUsage() and resolveCodexPriorityBillingDecision() now consume these values, so requests from installations configured for billingModelSource="original" or codexPriorityBillingSource="actual" will be charged with the wrong pricing instead of being left unbilled as before.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
已修复，见 a00c88e：无可用缓存时不再按 hardcoded defaults 计费，而是跳过价格解析；同时补了 direct read 失败与 cached fallback 的回归测试。
| billingModelSource: validated.billingModelSource, | ||
| codexPriorityBillingSource: validated.codexPriorityBillingSource, |
There was a problem hiding this comment.
Now that /api/admin/system-config can update billingModelSource and codexPriorityBillingSource, this route also needs to clear the 60s getCachedSystemSettings() cache the same way saveSystemSettings() does. Otherwise API-driven changes can leave proxy code paths that read cached settings using the previous billing behavior for up to a minute.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
已修复，见 a00c88e：/api/admin/system-config 现在会在更新后主动 invalidateSystemSettingsCache。
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/repository/system-config.ts (1)
230-257: 三级降级查询策略合理，但建议添加注释说明。当前实现了三级降级查询：
- 完整字段集（含
codexPriorityBillingSource）- 移除
codexPriorityBillingSource的字段集- 最小字段集（仅 7 个核心字段）
这种设计能够兼容多个数据库版本，但最小字段集仅包含少量字段，依赖
toSystemSettings转换器来填充缺失字段的默认值。建议在最小字段集处添加注释，说明其使用场景和依赖关系。💡 建议添加注释
logger.warn("system_settings 表存在多个缺失列，继续使用最小字段集读取。", { error: fallbackError, }); + // 最小字段集仅包含核心标识和时间戳字段 + // 其他字段将由 toSystemSettings 转换器填充默认值 const minimalSelection = { id: systemSettings.id, siteTitle: systemSettings.siteTitle,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/repository/system-config.ts` around lines 230 - 257, Add a brief inline comment above the minimalSelection definition in system-config.ts describing that this block is the third/final fallback of the three-tier query strategy (after the full selection and selectionWithoutCodexPriorityBillingSource), that minimalSelection includes only seven core fields to maintain compatibility with older DB schemas, and that missing fields are expected to be populated later by the toSystemSettings converter; reference symbols: minimalSelection, selectionWithoutCodexPriorityBillingSource, toSystemSettings, isUndefinedColumnError, systemSettings, and db so maintainers can find the fallback logic and understand its dependency on default-filling in the conversion layer.tests/integration/billing-model-source.test.ts (1)
456-521: 把 Session / 限流两条计费路径也一起断言这些新增 Codex 场景里，有些用例只校验了
updateMessageRequestCost，有些最多再校验SessionManager.updateSessionUsage，但没有把RateLimitService.trackCost一起守住。当前改动同一份计费决策会同时驱动 DB、Session 和限流三条路径；如果其中一条还在按旧的 requested/actual 逻辑走，这组测试仍然可能全部通过。建议像前面的runScenario一样，把三处结果收口成统一断言。Also applies to: 523-575, 645-700, 702-757, 759-814, 816-868, 870-928
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/billing-model-source.test.ts` around lines 456 - 521, The test only asserts DB and session billing paths but omits the rate-limit billing path; add a mock implementation for RateLimitService.trackUserDailyCost (or reuse the existing mock) to capture the tracked cost (like you do for updateMessageRequestCost and SessionManager.updateSessionUsage), then after calling ProxyResponseHandler.dispatch and drainAsyncTasks assert the captured rate-limit cost equals the same expected value (e.g. "32"); update the current test and the other listed test blocks to include this third assertion and reference the mocked RateLimitService.trackUserDailyCost alongside updateMessageRequestCost and SessionManager.updateSessionUsage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/v1/_lib/proxy/session.ts`:
- Around line 811-858: The current loadBillingSettings flow calls
getSystemSettings and on error falls back to getCachedSystemSettings, but
getCachedSystemSettings can itself call getSystemSettings on cache miss, causing
a second DB read; change loadBillingSettings to avoid any potential second DB
call after the initial failure by using a cache-only getter or an option that
disables refresh. Update loadBillingSettings (and billingSettingsPromise logic)
to, on the first catch from getSystemSettings, import and call a cache-only API
(e.g., a new getCachedSystemSettingsNoRefresh / getCachedSystemSettingsOnlyCache
exported from the system-settings-cache module) and if that returns no persisted
cache (hasPersistedCachedSettings false) immediately return the safe defaults
(billingModelSource: "redirected", codexPriorityBillingSource: "requested")
without attempting any further system DB reads. Ensure references to
getSystemSettings and getCachedSystemSettings in the code are updated to use the
new cache-only call so a DB retry is not triggered on the hot proxy path.
In `@src/repository/_shared/transformers.ts`:
- Line 200: Normalize codexPriorityBillingSource by whitelisting allowed values:
when building the settings object (the place using
dbSettings?.codexPriorityBillingSource, e.g., the transform/mapper that sets
codexPriorityBillingSource), only accept "requested" or "actual" and otherwise
fallback to "requested"; implement this by checking the dbSettings value (e.g.,
const v = dbSettings?.codexPriorityBillingSource) and setting
codexPriorityBillingSource = v === "actual" ? "actual" : "requested" so
arbitrary strings are not passed through.
---
Nitpick comments:
In `@src/repository/system-config.ts`:
- Around line 230-257: Add a brief inline comment above the minimalSelection
definition in system-config.ts describing that this block is the third/final
fallback of the three-tier query strategy (after the full selection and
selectionWithoutCodexPriorityBillingSource), that minimalSelection includes only
seven core fields to maintain compatibility with older DB schemas, and that
missing fields are expected to be populated later by the toSystemSettings
converter; reference symbols: minimalSelection,
selectionWithoutCodexPriorityBillingSource, toSystemSettings,
isUndefinedColumnError, systemSettings, and db so maintainers can find the
fallback logic and understand its dependency on default-filling in the
conversion layer.
In `@tests/integration/billing-model-source.test.ts`:
- Around line 456-521: The test only asserts DB and session billing paths but
omits the rate-limit billing path; add a mock implementation for
RateLimitService.trackUserDailyCost (or reuse the existing mock) to capture the
tracked cost (like you do for updateMessageRequestCost and
SessionManager.updateSessionUsage), then after calling
ProxyResponseHandler.dispatch and drainAsyncTasks assert the captured rate-limit
cost equals the same expected value (e.g. "32"); update the current test and the
other listed test blocks to include this third assertion and reference the
mocked RateLimitService.trackUserDailyCost alongside updateMessageRequestCost
and SessionManager.updateSessionUsage.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 467d634e-f865-4b0f-a4c1-d26bd960cf48
📒 Files selected for processing (29)
drizzle/0085_busy_ken_ellis.sqldrizzle/meta/0085_snapshot.jsondrizzle/meta/_journal.jsonmessages/en/settings/config.jsonmessages/ja/settings/config.jsonmessages/ru/settings/config.jsonmessages/zh-CN/settings/config.jsonmessages/zh-TW/settings/config.jsonsrc/actions/system-config.tssrc/app/[locale]/settings/config/_components/system-settings-form.tsxsrc/app/[locale]/settings/config/page.tsxsrc/app/api/admin/system-config/route.tssrc/app/v1/_lib/proxy/response-handler.tssrc/app/v1/_lib/proxy/session.tssrc/drizzle/schema.tssrc/lib/config/system-settings-cache.tssrc/lib/utils/special-settings.tssrc/lib/validation/schemas.tssrc/repository/_shared/transformers.test.tssrc/repository/_shared/transformers.tssrc/repository/system-config.tssrc/types/special-settings.tssrc/types/system-config.tstests/integration/billing-model-source.test.tstests/unit/actions/system-config-save.test.tstests/unit/lib/config/system-settings-cache.test.tstests/unit/lib/utils/special-settings.test.tstests/unit/proxy/session.test.tstests/unit/repository/system-config-update-missing-columns.test.ts
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
Reviewed the diff for billing-source selection semantics, settings persistence/migration compatibility, admin UI wiring, and the added tests.
No reportable issues met the >=80 confidence threshold after validation.
PR Size: XL
- Lines changed: 5124
- Files changed: 29
- Split suggestions:
- PR 1: DB migration +
src/drizzle/schema.ts+ repository compatibility - PR 2: Proxy billing decision logic (
src/app/v1/_lib/proxy/*) - PR 3: Settings UI + i18n message updates (
src/app/[locale]/settings/*,messages/*) - PR 4: Tests only (
tests/*)
- PR 1: DB migration +
Review Coverage
- Logic and correctness - No issues found
- Security (OWASP Top 10) - No issues found
- Error handling - No issues found
- Type safety - No issues found
- Documentation accuracy - No issues found
- Test coverage - No missing critical tests identified
- Code clarity - No issues found
Automated review by Codex AI
| @@ -0,0 +1 @@ | |||
| ALTER TABLE "system_settings" ADD COLUMN "codex_priority_billing_source" varchar(20) DEFAULT 'requested' NOT NULL; No newline at end of file | |||
There was a problem hiding this comment.
[Low] [STANDARD-VIOLATION] SQL migration file missing trailing newline
Why this is a problem: The file ends without a trailing newline character. While PostgreSQL and most clients handle this correctly at runtime, POSIX convention and the project's other migration files follow the convention of ending text files with a newline. The Drizzle tooling also expects this format.
Suggested fix:
ALTER TABLE "system_settings" ADD COLUMN "codex_priority_billing_source" varchar(20) DEFAULT 'requested' NOT NULL;(Note the trailing newline after the semicolon)
There was a problem hiding this comment.
Code Review Summary
This PR adds a codexPriorityBillingSource system setting to control whether Codex Priority billing uses the requested or actual service tier. The implementation is solid with proper error handling, backward compatibility for databases missing the new column, and comprehensive test coverage.
PR Size: XL
- Lines changed: 5,124 (4,962 additions + 162 deletions)
- Files changed: 29
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 1 |
Low Priority Issues (Author's Discretion)
- SQL migration file missing trailing newline (
drizzle/0085_busy_ken_ellis.sql)- The file ends without a trailing newline character, deviating from POSIX convention and the pattern of other migration files in the project.
- No runtime impact; cosmetic only.
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - Well implemented with graceful fallbacks
- Type safety - Proper TypeScript types and Zod validation
- Documentation accuracy - Types have appropriate comments
- Test coverage - Comprehensive tests for both modes and edge cases
- Code clarity - Good
Implementation Highlights
- Backward compatibility handled via graceful column-missing detection (42703 error code)
- Proper caching of billing settings per request with Promise-based concurrency safety
- Correct fallback behavior when
actualmode is set but response omits service_tier hasPriorityServiceTierSpecialSettingcorrectly handles legacy audit records
Automated review by Claude AI
There was a problem hiding this comment.
- Identified the current open PR as
#960. - Computed PR size as
XL(5124 lines changed across 29 files) and applied labelsize/XL. - Completed a diff-scoped review across the 6 required perspectives; no validated findings met the >=80 confidence reporting threshold.
- Posted the required summary review via
gh pr review(no inline comments were posted).
🧪 测试结果
总体结果: ✅ 所有测试通过 |
| if ( | ||
| codexServiceTierResult.billingSourcePreference == null && | ||
| codexServiceTierResult.resolvedFrom == null && | ||
| codexServiceTierResult.actualServiceTier != null | ||
| ) { | ||
| return codexServiceTierResult.actualServiceTier === "priority"; | ||
| } | ||
| return codexServiceTierResult.effectivePriority; |
There was a problem hiding this comment.
hasPriorityServiceTierSpecialSetting
The backward-compatibility guard (lines 212–218) is logically redundant. For old records (before this PR), effectivePriority was always computed as actualServiceTier === "priority", so falling through to return codexServiceTierResult.effectivePriority on line 219 would produce the exact same result. The extra branch adds complexity without changing behavior. Consider simplifying to just return effectivePriority unconditionally:
| if ( | |
| codexServiceTierResult.billingSourcePreference == null && | |
| codexServiceTierResult.resolvedFrom == null && | |
| codexServiceTierResult.actualServiceTier != null | |
| ) { | |
| return codexServiceTierResult.actualServiceTier === "priority"; | |
| } | |
| return codexServiceTierResult.effectivePriority; | |
| return codexServiceTierResult.effectivePriority; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/utils/special-settings.ts
Line: 212-219
Comment:
**Redundant backward-compat branch in `hasPriorityServiceTierSpecialSetting`**
The backward-compatibility guard (lines 212–218) is logically redundant. For old records (before this PR), `effectivePriority` was always computed as `actualServiceTier === "priority"`, so falling through to `return codexServiceTierResult.effectivePriority` on line 219 would produce the exact same result. The extra branch adds complexity without changing behavior. Consider simplifying to just return `effectivePriority` unconditionally:
```suggestion
return codexServiceTierResult.effectivePriority;
```
How can I resolve this? If you propose a fix, please make it concise.| try { | ||
| billingSourcePreference = await session.getCodexPriorityBillingSource(); | ||
| } catch (error) { | ||
| logger.warn( | ||
| "[ResponseHandler] Failed to load codex priority billing source, fallback to requested", | ||
| { | ||
| error: error instanceof Error ? error.message : String(error), | ||
| } | ||
| ); | ||
| } |
There was a problem hiding this comment.
getCodexPriorityBillingSource()
session.getCodexPriorityBillingSource() never throws — loadBillingSettings() catches all errors internally and returns a default value. The try/catch wrapping it is dead code, since exceptions are swallowed inside the method. Consider removing the try/catch to reduce noise, or adding a comment explaining why it's kept for future safety:
| try { | |
| billingSourcePreference = await session.getCodexPriorityBillingSource(); | |
| } catch (error) { | |
| logger.warn( | |
| "[ResponseHandler] Failed to load codex priority billing source, fallback to requested", | |
| { | |
| error: error instanceof Error ? error.message : String(error), | |
| } | |
| ); | |
| } | |
| billingSourcePreference = await session.getCodexPriorityBillingSource(); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/response-handler.ts
Line: 234-243
Comment:
**Dead catch block around `getCodexPriorityBillingSource()`**
`session.getCodexPriorityBillingSource()` never throws — `loadBillingSettings()` catches all errors internally and returns a default value. The `try/catch` wrapping it is dead code, since exceptions are swallowed inside the method. Consider removing the try/catch to reduce noise, or adding a comment explaining why it's kept for future safety:
```suggestion
billingSourcePreference = await session.getCodexPriorityBillingSource();
```
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/api/admin/system-config/route.ts (1)
59-69:⚠️ Potential issue | 🟡 Minor更新 JSDoc 注释以准确反映 API 支持的字段
POST 端点的 JSDoc 注释（第 35-43 行）列出的字段不完整。注释仅列出 7 个字段，但实现实际上处理 9 个字段，缺少
billingModelSource和codexPriorityBillingSource的说明。请更新注释以匹配实现：* Body: { * siteTitle: string; * allowGlobalUsageView: boolean; * currencyDisplay?: string; * billingModelSource?: string; * codexPriorityBillingSource?: string; * enableAutoCleanup?: boolean; * cleanupRetentionDays?: number; * cleanupSchedule?: string; * cleanupBatchSize?: number; * }另外，如果 API 为设计而故意限制
UpdateSystemSettingsSchema中的某些字段（如timezone、enableClientVersionCheck等），建议在注释中说明原因，以避免将来的混淆。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/admin/system-config/route.ts` around lines 59 - 69, Update the JSDoc for the POST endpoint in route.ts so it accurately documents all fields accepted by updateSystemSettings (referenced function updateSystemSettings and the body shape used when calling it), adding billingModelSource and codexPriorityBillingSource and the optional markers for currencyDisplay, enableAutoCleanup, cleanupRetentionDays, cleanupSchedule, and cleanupBatchSize to match the implementation; also add a short note in the comment explaining any intentionally omitted schema fields from UpdateSystemSettingsSchema (e.g., timezone or enableClientVersionCheck) so readers know those exclusions are by design.
🧹 Nitpick comments (1)
src/app/v1/_lib/proxy/session.ts (1)
873-880: 建议添加代码注释说明billingModelSource不同场景下的默认值差异当设置完全不可用时，此处默认为
"redirected"，而src/repository/system-config.ts中的createFallbackSettings()默认为"original"。这个差异是有意设计的：
"original"：用于初始化和表不存在的场景"redirected"：用于运行时请求处理中所有读取失败的场景（更保守的计费策略）然而代码缺少注释解释这个意图，容易引起误解。建议在两处都添加注释说明这个不同，帮助维护者理解这不是无意的不一致。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/session.ts` around lines 873 - 880, 在 ProxySession 的错误分支（即设置读取失败时设置 billingModelSource 为 "redirected" 的逻辑）和 createFallbackSettings 函数中添加注释，明确说明两处默认值不同是有意为之：createFallbackSettings 使用 "original" 用于初始化或表缺失场景，而 ProxySession 的 "redirected" 是运行时读取失败时采用的更保守计费策略；在注释中引用相关符号（billingModelSource、codexPriorityBillingSource、ProxySession 和 createFallbackSettings）并简短说明每个默认值适用的场景和理由，以避免误解。
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/app/api/admin/system-config/route.ts`:
- Around line 59-69: Update the JSDoc for the POST endpoint in route.ts so it
accurately documents all fields accepted by updateSystemSettings (referenced
function updateSystemSettings and the body shape used when calling it), adding
billingModelSource and codexPriorityBillingSource and the optional markers for
currencyDisplay, enableAutoCleanup, cleanupRetentionDays, cleanupSchedule, and
cleanupBatchSize to match the implementation; also add a short note in the
comment explaining any intentionally omitted schema fields from
UpdateSystemSettingsSchema (e.g., timezone or enableClientVersionCheck) so
readers know those exclusions are by design.
---
Nitpick comments:
In `@src/app/v1/_lib/proxy/session.ts`:
- Around line 873-880: 在 ProxySession 的错误分支（即设置读取失败时设置 billingModelSource 为
"redirected" 的逻辑）和 createFallbackSettings
函数中添加注释，明确说明两处默认值不同是有意为之：createFallbackSettings 使用 "original" 用于初始化或表缺失场景，而
ProxySession 的 "redirected"
是运行时读取失败时采用的更保守计费策略；在注释中引用相关符号（billingModelSource、codexPriorityBillingSource、ProxySession
和 createFallbackSettings）并简短说明每个默认值适用的场景和理由，以避免误解。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 67d29fea-9242-443a-8be2-d3d18788ef98
📒 Files selected for processing (9)
drizzle/0085_busy_ken_ellis.sqlsrc/app/api/admin/system-config/route.tssrc/app/v1/_lib/proxy/session.tssrc/lib/config/index.tssrc/lib/config/system-settings-cache.tssrc/repository/_shared/transformers.tssrc/repository/system-config.tstests/integration/billing-model-source.test.tstests/unit/proxy/session.test.ts
✅ Files skipped from review due to trivial changes (1)
- drizzle/0085_busy_ken_ellis.sql
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/config/system-settings-cache.ts
- tests/unit/proxy/session.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a00c88e146
ℹ️ 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".
| requestedServiceTier, | ||
| actualServiceTier, | ||
| billingSourcePreference, | ||
| resolvedFrom, | ||
| effectivePriority: effectiveTier === "priority", |
There was a problem hiding this comment.
When loadBillingSettings() falls back to source: "default" (for example right after a restart while the settings query is failing), getResolvedPricingByBillingSource() now skips pricing entirely in session.ts:745-747, but resolveCodexPriorityBillingDecision() still returns a concrete billingSourcePreference/effectivePriority here. That causes codex_service_tier_result to be written with effectivePriority: true for requests like service_tier: "priority", and the log detail UIs then show a Fast badge even though the request was not billed at all. This makes outage-time billing/audit data misleading.
Useful? React with 👍 / 👎.
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
1 participant
Summary
Problem
Some third-party Codex proxies do not return the
service_tierfield in responses, causing incorrect billing for Fast Priority requests. Previously, CCH always used the response's service_tier for billing decisions, which meant:Related Issues:
Solution
Adds a new system setting
codexPriorityBillingSourcewith two modes:requested(default): Bill based on the service_tier sent in the request - ensures priority pricing when user asks for priorityactual: Bill based on the service_tier returned in the response, falling back to request value if response omits itChanges
Core Changes
drizzle/0085_busy_ken_ellis.sql): Addscodex_priority_billing_sourcecolumn tosystem_settingstablesrc/app/v1/_lib/proxy/session.ts): NewgetCodexPriorityBillingSource()method with caching and fallback logicsrc/app/v1/_lib/proxy/response-handler.ts): Refactored billing decision logic toresolveCodexPriorityBillingDecision()which respects the new settingsrc/types/special-settings.ts,src/lib/utils/special-settings.ts): ExtendedCodexServiceTierResultSpecialSettingto trackbillingSourcePreferenceandresolvedFromfor audit transparencySupporting Changes
src/types/system-config.ts): AddedCodexPriorityBillingSourcetypesrc/lib/validation/schemas.ts): Zod schema for the new settingsrc/repository/system-config.ts): Database compatibility layer with graceful degradation for missing columnssrc/app/[locale]/settings/config/): New dropdown in system settings formBackward Compatibility
requestedas the implicit defaultgetSystemSettings()attempts full selection, falls back to selection without new column, then to minimal fieldsupdateSystemSettings()gracefully handles missing column by excluding it from updatesTesting
Automated Tests
tests/unit/proxy/session.test.tstests/unit/actions/system-config-save.test.ts)tests/unit/lib/utils/special-settings.test.ts)tests/unit/repository/system-config-update-missing-columns.test.ts)tests/integration/billing-model-source.test.ts) - includes new test cases for "actual" modeTest Coverage Highlights
requestedmode: Uses priority pricing when request hasservice_tier: "priority", ignores actual response tieractualmode: Uses priority pricing when response reportsservice_tier: "priority", falls back to request value if response omits itactualmode: Does not use priority pricing when request asked for priority but response explicitly reports non-priorityVerification
bun run typecheckbun run buildbun run lintbun run testbunx vitest run tests/integration/billing-model-source.test.ts tests/unit/lib/utils/special-settings.test.ts tests/unit/actions/system-config-save.test.ts tests/unit/lib/config/system-settings-cache.test.ts tests/unit/repository/system-config-update-missing-columns.test.ts tests/unit/proxy/session.test.tsNotes
Description enhanced by Claude AI
Greptile Summary
This PR introduces a new system setting
codexPriorityBillingSourcethat lets operators choose whether Codex Priority billing decisions are based on the service tier sent in the request (requested, default) or the service tier returned in the response (actual). This solves a real compatibility problem with third-party Codex proxies that omitservice_tierfrom responses.Key changes:
drizzle/0085_busy_ken_ellis.sql): Addscodex_priority_billing_source varchar(20) DEFAULT 'requested' NOT NULL— safe to apply on live databases.response-handler.ts): NewresolveCodexPriorityBillingDecision()replaces the old directactualServiceTier === "priority"check; called consistently across all three finalization paths (streaming, non-streaming, deferred).session.ts):getCodexPriorityBillingSource()reuses the existingbillingSettingsPromisecoalescing pattern, loading both billing settings in a single DB read per request.system-config.ts,transformers.ts): Three-tier read fallback and two-tier write fallback gracefully handle databases where the new column hasn't been migrated yet;toSystemSettingsdefaults to"requested"for missing/unknown values.special-settings.ts):CodexServiceTierResultSpecialSettinggains optionalbillingSourcePreferenceandresolvedFromfields that record exactly which source was used for each request, with backward-compatible display logic for old stored records.Confidence Score: 5/5
Important Files Changed
resolveCodexPriorityBillingDecision()function correctly handles both billing modes and wireseffectivePriorityinto three call sites (streaming, non-streaming, deferred finalization). Minor: deadtry/catcharoundgetCodexPriorityBillingSource().getCodexPriorityBillingSource()correctly reuses thebillingSettingsPromisecoalescing pattern for thread-safe single-load-per-request semantics; fallback chain (live → cache → default "requested") is consistent.codexPriorityBillingSourcefrom both SET and RETURNING, and the column's absence is transparently resolved to the default value bytoSystemSettings.billingSourcePreference/resolvedFromfields inbuildSettingKeyandhasPriorityServiceTierSpecialSettingare correct; backward-compat branch inhasPriorityServiceTierSpecialSettingis redundant (old records'effectivePriorityalready encodes the same value) but harmless.codexPriorityBillingSource: "requested"to both theDEFAULT_SETTINGSpick and the hard-coded emergency fallback object;invalidateSystemSettingsCache()is called from the save action, ensuring fresh values propagate.billingSourcePreference?andresolvedFrom?onCodexServiceTierResultSpecialSettinguse correct optional-nullable typing for backward compatibility with old serialized records.ALTER TABLE ADD COLUMNwithDEFAULT 'requested' NOT NULL; safely applicable to live databases without locking concerns on small settings tables.toSystemSettingscorrectly validatescodexPriorityBillingSourceagainst the allowed enum values and defaults to"requested"for any unexpected/missing value, matching all other fallback paths.codexPriorityBillingSourceZod enum schema is correctly optional and uses a clear error message; wired intoUpdateSystemSettingsSchema.Sequence Diagram
sequenceDiagram participant Client participant ResponseHandler participant Session participant DB as SystemSettings DB participant Cache as InMemoryCache Client->>ResponseHandler: Codex response received ResponseHandler->>ResponseHandler: parseServiceTierFromResponseText()<br/>(actualServiceTier) ResponseHandler->>ResponseHandler: resolveCodexPriorityBillingDecision(session, actualServiceTier) ResponseHandler->>Session: getCodexPriorityBillingSource() Session->>Session: loadBillingSettings() [promise-coalesced] alt Live DB read succeeds Session->>DB: getSystemSettings() DB-->>Session: codexPriorityBillingSource else DB fails, cache available Session->>Cache: getCachedSystemSettingsOnlyCache() Cache-->>Session: cachedSettings.codexPriorityBillingSource else All fails Session-->>Session: default "requested" end Session-->>ResponseHandler: "requested" | "actual" alt billingSourcePreference = "actual" alt actualServiceTier != null ResponseHandler->>ResponseHandler: effectiveTier = actualServiceTier<br/>resolvedFrom = "actual" else actualServiceTier null, requestedServiceTier set ResponseHandler->>ResponseHandler: effectiveTier = requestedServiceTier<br/>resolvedFrom = "requested" end else billingSourcePreference = "requested" ResponseHandler->>ResponseHandler: effectiveTier = requestedServiceTier<br/>resolvedFrom = "requested" end ResponseHandler->>ResponseHandler: effectivePriority = (effectiveTier === "priority") ResponseHandler->>Session: addSpecialSetting(codex_service_tier_result) ResponseHandler->>ResponseHandler: calculateRequestCost(priorityServiceTierApplied)Prompt To Fix All With AI
Last reviewed commit: "fix: address codex p..."