fix: align non-chat endpoint fallback with raw policy#1091
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough引入面向非会话（raw）端点的跨提供商回退开关，并贯穿数据库 schema、代理会话/forwarder、计费/账本过滤、前端设置 UI 与大量测试，调整相关运行时策略与观测行为。 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
enhancement
![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: 3146acfa5a
ℹ️ 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".
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Summary
This PR adds allowNonConversationEndpointProviderFallback system setting with 5-language i18n support, enabling cross-provider fallback for /v1/messages/count_tokens and /v1/responses/compact endpoints. The implementation correctly maintains raw-safe session context and non-billing behavior for these endpoints.
PR Size: XL
- Lines changed: 7,367 (7,220 additions, 147 deletions)
- Files changed: 55
Split Suggestion: This PR could be split into:
- Core proxy/endpoint policy changes (endpoint-policy.ts, guard-pipeline.ts, forwarder.ts)
- Session management changes (session.ts, session-guard.ts)
- System settings & i18n changes (types, schema, validation, i18n files)
- SQL migrations and repository changes
- UI components and dashboard changes
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 1 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 1 | 0 |
| Types | 0 | 0 | 2 | 0 |
| SQL | 0 | 0 | 1 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
High Priority Issues (Should Fix)
[TYPE-ASSERTION] Type Safety Workaround in proxy-handler.ts
Location: src/app/v1/_lib/proxy-handler.ts:24-30
The code uses a type assertion to access setRawCrossProviderFallbackEnabled:
(
session as ProxySession & {
setRawCrossProviderFallbackEnabled?: ((enabled: boolean) => void) | undefined;
}
).setRawCrossProviderFallbackEnabled?.(
systemSettings.allowNonConversationEndpointProviderFallback ?? true
);Issue: This type assertion bypasses TypeScript's type checking. The method should be part of the proper ProxySession type definition.
Suggested Fix: Add setRawCrossProviderFallbackEnabled to the ProxySession class type definition and remove the type assertion:
// In session.ts, the method is already defined
// Just ensure it's included in the type exports
// In proxy-handler.ts, use directly:
session.setRawCrossProviderFallbackEnabled(
systemSettings.allowNonConversationEndpointProviderFallback ?? true
);[TYPE-ASSERTION] Type Safety Workaround in forwarder.ts
Location: src/app/v1/_lib/proxy/forwarder.ts:471-495
The isSessionRawCrossProviderFallbackEnabled function uses unknown type assertion:
const rawSession = session as unknown as {
requestUrl?: URL;
isRawCrossProviderFallbackEnabled?: (() => boolean) | undefined;
getEndpointPolicy?: (() => ReturnType<typeof resolveEndpointPolicy>) | undefined;
endpointPolicy?: ReturnType<typeof resolveEndpointPolicy>;
};Issue: This is another type assertion bypass. The function should properly use the ProxySession type.
Suggested Fix: Use proper method calls on the typed ProxySession:
function isSessionRawCrossProviderFallbackEnabled(session: ProxySession): boolean {
if (typeof (session as ProxySession & { isRawCrossProviderFallbackEnabled?: () => boolean }).isRawCrossProviderFallbackEnabled === "function") {
return (session as ProxySession & { isRawCrossProviderFallbackEnabled: () => boolean }).isRawCrossProviderFallbackEnabled();
}
return session.getEndpointPolicy().allowRawCrossProviderFallback;
}[SQL-TRANSACTION] Missing Transaction in Migration
Location: drizzle/0096_equal_selene.sql
The migration performs multiple operations:
- ALTER TABLE to add column
- DELETE FROM usage_ledger
- CREATE OR REPLACE FUNCTION
Issue: If the migration fails partway through (e.g., between the ALTER and DELETE), the database could be in an inconsistent state.
Suggested Fix: Wrap the migration in a transaction:
BEGIN;
ALTER TABLE "system_settings" ADD COLUMN "allow_non_conversation_endpoint_provider_fallback" boolean DEFAULT true NOT NULL;
DELETE FROM "usage_ledger"
WHERE "endpoint" IS NOT NULL
AND LOWER(REGEXP_REPLACE("endpoint", '/+$', ''))
IN ('/v1/messages/count_tokens', '/v1/responses/compact');
-- ... rest of migration
COMMIT;[EDGE-CASE] Empty Endpoint String Handling
Location: src/repository/_shared/usage-log-filters.ts:20-28
The normalizeUsageLogEndpoint function returns an empty string for empty input, which could cause issues if used in SQL comparisons.
Suggested Fix: Consider returning null for empty input or adding explicit handling:
function normalizeUsageLogEndpoint(endpoint: string): string | null {
const trimmed = endpoint.trim().toLowerCase();
if (\!trimmed) {
return null;
}
// ...
}Review Coverage
- Logic and correctness - No critical bugs found
- Security (OWASP Top 10) - No security issues found
- Error handling - Proper error handling in new code
- Type safety - Two type assertion workarounds noted
- Documentation accuracy - Comments match code behavior
- Test coverage - Adequate test coverage for new feature
- Code clarity - Generally clear implementation
Notes
- The
NON_BILLING_ENDPOINTSconstant correctly includes both target endpoints - The SQL function properly handles non-billing endpoints by deleting ledger entries
- i18n translations are complete for all 5 supported languages
- The guard pipeline correctly uses
RAW_SAFE_SESSION_PIPELINEfor the target endpoints
Automated review by Claude AI
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements a cross-provider fallback mechanism for non-conversation endpoints, specifically /v1/messages/count_tokens and /v1/responses/compact. It introduces a new system setting, allowNonConversationEndpointProviderFallback, to manage this feature. Key changes include updates to the proxy forwarder to support fallbacks for raw endpoints, adjustments to the guard pipeline to incorporate session context while bypassing chat-specific guards, and comprehensive modifications to billing and logging to ensure these endpoints remain non-billable and hidden from default views. Review feedback suggests refining the implementation by removing unnecessary asynchronous markers, simplifying helper function calls in forwarder.ts, and ensuring that side effects like Codex context tiering are correctly guarded against non-billing requests.
Contributor
There was a problem hiding this comment.
Code Review Summary
This PR adds allowNonConversationEndpointProviderFallback system setting with 5-language i18n support, enabling cross-provider fallback for /v1/messages/count_tokens and /v1/responses/compact endpoints. The implementation correctly maintains raw-safe session context and non-billing behavior for these endpoints.
PR Size: XL
- Lines changed: 7,367 (7,220 additions, 147 deletions)
- Files changed: 55
Split Suggestion: This PR could be split into:
- Core proxy/endpoint policy changes (endpoint-policy.ts, guard-pipeline.ts, forwarder.ts)
- Session management changes (session.ts, session-guard.ts)
- System settings & i18n changes (types, schema, validation, i18n files)
- SQL migrations and repository changes
- UI components and dashboard changes
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 1 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 1 | 0 |
| Types | 0 | 0 | 2 | 0 |
| SQL | 0 | 0 | 1 | 0 |
Medium Priority Issues (Should Fix)
[TYPE-ASSERTION] Type Safety Workaround in proxy-handler.ts
Location: src/app/v1/_lib/proxy-handler.ts:24-30
The code uses a type assertion to access setRawCrossProviderFallbackEnabled. This bypasses TypeScript's type checking. The method should be part of the proper ProxySession type definition.
Suggested fix: Add setRawCrossProviderFallbackEnabled to the ProxySession class type definition and remove the type assertion.
[TYPE-ASSERTION] Type Safety Workaround in forwarder.ts
Location: src/app/v1/_lib/proxy/forwarder.ts:471-495
The isSessionRawCrossProviderFallbackEnabled function uses unknown type assertion. This is another type assertion bypass that should be properly typed.
Suggested fix: Use proper method calls on the typed ProxySession by ensuring isRawCrossProviderFallbackEnabled is part of the exported type definition.
[SQL-TRANSACTION] Missing Transaction in Migration
Location: drizzle/0096_equal_selene.sql
The migration performs multiple independent operations (ALTER, DELETE, CREATE OR REPLACE FUNCTION) without a transaction wrapper. If it fails partway through, the database could be in an inconsistent state.
Suggested fix: Wrap the migration in BEGIN/COMMIT to ensure atomicity.
[EDGE-CASE] Empty Endpoint String Handling
Location: src/repository/_shared/usage-log-filters.ts:19-28
The normalizeUsageLogEndpoint function returns an empty string for empty input, which could cause issues in SQL comparisons.
Suggested fix: Consider returning null for empty input instead of empty string.
Review Coverage
- Logic and correctness - No critical bugs found
- Security (OWASP Top 10) - No security issues found
- Error handling - Proper error handling in new code
- Type safety - Two type assertion workarounds noted
- Documentation accuracy - Comments match code behavior
- Test coverage - Adequate test coverage for new feature
- Code clarity - Generally clear implementation
Notes
- The NON_BILLING_ENDPOINTS constant correctly includes both target endpoints
- The SQL function properly handles non-billing endpoints by deleting ledger entries
- i18n translations are complete for all 5 supported languages
- The guard pipeline correctly uses RAW_SAFE_SESSION_PIPELINE for the target endpoints
Automated review by Claude AI
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/unit/proxy/endpoint-policy-parity.test.ts (1)
239-245:⚠️ Potential issue | 🟡 Minor测试名已不符合新的默认策略。
默认端点现在并不是“所有 allow flags 都为 true”，
allowRawCrossProviderFallback应保持 false。建议把测试名改成新语义，避免误导。建议更新测试名
- test("default policy has all allow flags set to true", () => { + test("default policy allows normal chat flow but keeps raw cross-provider fallback disabled", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/proxy/endpoint-policy-parity.test.ts` around lines 239 - 245, The test name is misleading because the default policy no longer has all allow flags true (specifically allowRawCrossProviderFallback remains false); update the test title string in the test that iterates over DEFAULT_ENDPOINTS and calls resolveEndpointPolicy(pathname) to reflect the new semantics (e.g., "default policy has expected allow flags" or "default policy has allow flags set (raw cross-provider fallback false)"), leaving the assertions on policy.allowRetry, policy.allowProviderSwitch, policy.allowRawCrossProviderFallback, and policy.allowCircuitBreakerAccounting unchanged.tests/unit/proxy/proxy-forwarder-hedge-first-byte.test.ts (1)
1723-1787:⚠️ Potential issue | 🟡 Minor同步更新测试意图并精确锁定 winner。
测试名仍描述旧行为，而且
winnerEntry先匹配request_success再断言hedge_winner。建议直接按 provider2 +hedge_winner查找，避免链路里出现其他成功项时产生脆弱失败。建议收紧测试断言
- test("endpoint resolution failure should not inflate launchedProviderCount, winner gets request_success not hedge_winner", async () => { + test("endpoint resolution fallback counts the initial attempt, so the later provider is hedge_winner", async () => { @@ - const winnerEntry = chain.find( - (entry) => entry.reason === "request_success" || entry.reason === "hedge_winner" - ); + const winnerEntry = chain.find( + (entry) => entry.id === provider2.id && entry.reason === "hedge_winner" + ); expect(winnerEntry).toBeDefined(); - expect(winnerEntry!.reason).toBe("hedge_winner");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/proxy/proxy-forwarder-hedge-first-byte.test.ts` around lines 1723 - 1787, Update the test to assert the specific provider2 hedge-winner entry instead of loosely matching any "request_success" or "hedge_winner": change the chain lookup that assigns winnerEntry to find an entry where entry.provider?.id === 2 && entry.reason === "hedge_winner" (using session.getProviderChain()), and update the test name string to reflect that provider2 should be the hedge_winner; keep the existing assertions that the response contains "provider":"p2" and session.provider?.id === 2.src/app/v1/_lib/proxy/guard-pipeline.ts (1)
189-196:⚠️ Potential issue | 🟡 Minor确认
fromRequestType仅在测试代码中使用，生产环境无影响，但方法设计存在不一致之处。此改动在生产代码中的实际影响已被限制：
proxy-handler.ts的生产路径通过fromSession()进行管道构建（第 81 行），该方法会根据allowRawCrossProviderFallback标志在RAW_SAFE_SESSION_PIPELINE和RAW_PASSTHROUGH_PIPELINE间选择。
fromRequestType()仅在测试代码中被调用（见guard-pipeline-warmup.test.ts），不存在遗留的生产调用点。因此原有对count_tokens创建 session 绑定的担忧实际上不会在生产环境中发生。然而，方法设计不一致性仍存在：
fromRequestType(RequestType.COUNT_TOKENS)硬编码返回RAW_SAFE_SESSION_PIPELINE（始终包含 "session" 和 "messageContext" 步骤）fromEndpointPolicy()则根据标志条件地选择管道此不一致可能在未来维护中造成混淆。若测试意图是总是验证完整的会话流程，建议在方法或注释中明确说明该设计选择。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/guard-pipeline.ts` around lines 189 - 196, fromRequestType currently hardcodes RequestType.COUNT_TOKENS to RAW_SAFE_SESSION_PIPELINE which is inconsistent with fromSession/fromEndpointPolicy that respect allowRawCrossProviderFallback; update fromRequestType(RequestType.COUNT_TOKENS) to mirror the same conditional selection between RAW_SAFE_SESSION_PIPELINE and RAW_PASSTHROUGH_PIPELINE (using the same flag/logic as fromSession/fromEndpointPolicy) or, if the hardcoded behavior is intentional for tests, add a clear comment above fromRequestType explaining it is test-only and why RAW_SAFE_SESSION_PIPELINE is required (reference symbols: fromRequestType, RequestType.COUNT_TOKENS, RAW_SAFE_SESSION_PIPELINE, RAW_PASSTHROUGH_PIPELINE, fromSession, fromEndpointPolicy, allowRawCrossProviderFallback, guard-pipeline-warmup.test.ts).
🧹 Nitpick comments (15)
src/lib/ledger-backfill/trigger.sql (1)
172-176: 非对话端点的早退逻辑正确，但建议补充注释说明语义。该分支在非对话端点写入时会删除已存在的
usage_ledger行并直接返回，覆盖了「历史 UPDATE 将旧对话端点改写为非对话端点」这种罕见但可能触发的场景。归一化规则（lowercase + 去尾斜杠）与service.ts、ledger-conditions.ts中保持一致。建议在该分支上方添加简短注释，说明为何要
DELETE而不是仅跳过 UPSERT（即：保证之前可能已写入的 ledger 行被清理），便于后续维护者理解。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/ledger-backfill/trigger.sql` around lines 172 - 176, 在触发器中针对非对话端点（使用 LOWER(REGEXP_REPLACE(COALESCE(NEW.endpoint, ''), '/+$', '')) 与 '/v1/messages/count_tokens'、'/v1/responses/compact' 比对）直接 DELETE FROM usage_ledger WHERE request_id = NEW.id 并 RETURN NEW 的分支需要补充注释，说明之所以 DELETE 而不是仅跳过 UPSERT 是为了清理可能在历史写入/旧对话端点写入时已存在的 ledger 行，避免遗留无效记录；请在该分支上方添加一行简短注释，明确引用 NEW、usage_ledger 和归一化规则以便后续维护者理解语义和原因。src/app/v1/_lib/proxy-handler.ts (1)
24-40: 改为直接调用 session 已公开的方法。
setRawCrossProviderFallbackEnabled已在ProxySession类（src/app/v1/_lib/proxy/session.ts:353）中公开定义为实例方法。第 24-30 和 36-40 行不应使用类型交叉和可选调用，直接调用即可，这样可以避免隐藏集成缺失问题（如果方法被删除或重命名，类型检查会立即捕获错误）。建议改为直接调用
- ( - session as ProxySession & { - setRawCrossProviderFallbackEnabled?: ((enabled: boolean) => void) | undefined; - } - ).setRawCrossProviderFallbackEnabled?.( + session.setRawCrossProviderFallbackEnabled( systemSettings.allowNonConversationEndpointProviderFallback ?? true );同样改进第 36-40 行。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy-handler.ts` around lines 24 - 40, The code is using a type intersection and optional chaining to call setRawCrossProviderFallbackEnabled on session; since ProxySession already publicly defines setRawCrossProviderFallbackEnabled (see ProxySession), remove the cast and optional call and invoke session.setRawCrossProviderFallbackEnabled(...) directly in both places (the try block and the catch block), keeping the same boolean arguments; also keep the existing logger.warn and session.setHighConcurrencyModeEnabled(false) behavior unchanged.src/app/v1/_lib/proxy/session.ts (1)
357-365:?? resolveEndpointPolicy(...)兜底分支不可达，可简化。
endpointPolicy为readonly且在构造函数里已经通过resolveSessionEndpointPolicy(init.requestUrl)（第 206 行）赋值，运行时永远不会是null/undefined。第 358-360 行的??兜底在类型上也没有意义，保留会让读者误以为该字段可能缺失。Proposed simplification
isRawCrossProviderFallbackEnabled(): boolean { - const endpointPolicy = - this.endpointPolicy ?? - resolveEndpointPolicy((this.requestUrl as URL | undefined)?.pathname ?? "/"); return ( - endpointPolicy.allowRawCrossProviderFallback && + this.endpointPolicy.allowRawCrossProviderFallback && (this.rawCrossProviderFallbackEnabled ?? false) ); }🤖 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 357 - 365, The isRawCrossProviderFallbackEnabled method contains an unnecessary fallback expression "this.endpointPolicy ?? resolveEndpointPolicy(...)" because endpointPolicy is readonly and already set in the constructor via resolveSessionEndpointPolicy, so remove the fallback and read endpointPolicy directly; update isRawCrossProviderFallbackEnabled to reference this.endpointPolicy only and keep the boolean expression using this.endpointPolicy.allowRawCrossProviderFallback and this.rawCrossProviderFallbackEnabled to simplify the code and avoid implying endpointPolicy can be undefined.src/repository/_shared/ledger-conditions.ts (1)
5-20: SQL 归一化与 JS 归一化存在轻微差异。
isNonBillingEndpoint（performance-formatter.ts）先trim()再toLowerCase()+ 去尾斜杠；这里的 SQL 版本仅做LOWER + REGEXP_REPLACE('/+$', '')，不处理前后空白。虽然usageLedger.endpoint通常来自 URL pathname 不会含空白，但建议保持两处一致性以免未来某条写入路径没有 trim 时出现跨层判定错位。Proposed fix
const NON_BILLING_LEDGER_ENDPOINT_CONDITION = sql`( ${usageLedger.endpoint} IS NULL - OR LOWER(REGEXP_REPLACE(${usageLedger.endpoint}, '/+$', '')) NOT IN ( + OR LOWER(REGEXP_REPLACE(BTRIM(${usageLedger.endpoint}), '/+$', '')) NOT IN ( ${sql.join( NON_BILLING_ENDPOINTS.map((endpoint) => sql`${endpoint}`), sql`, ` )} ) )`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/repository/_shared/ledger-conditions.ts` around lines 5 - 20, The SQL endpoint normalization in NON_BILLING_LEDGER_ENDPOINT_CONDITION differs from the JS isNonBillingEndpoint: it doesn't trim whitespace first, so align them by applying a trim step to ${usageLedger.endpoint} before lowercasing and removing trailing slashes; update the expression used in NON_BILLING_LEDGER_ENDPOINT_CONDITION (which is also referenced by LEDGER_BILLING_CONDITION) to perform whitespace trimming then REGEXP_REPLACE('/+$', '') and LOWER, and keep the comparison against the joined NON_BILLING_ENDPOINTS unchanged.tests/unit/proxy/proxy-forwarder-endpoint-audit.test.ts (1)
171-220:createSession仍通过Object.create+Object.assign绕过构造函数，且手动挂getEndpointPolicy——易与ProxySession内部字段漂移。当前写法把
endpointPolicy作为普通属性而非 private readonly 字段挂载（原类中是private readonly），并手动重写getEndpointPolicy，但isRawCrossProviderFallbackEnabled等新方法会直接访问this.endpointPolicy（字段访问而非 getter），目前两者恰好一致所以通过。若未来ProxySession把 endpoint policy 的访问改成懒加载或私有化再加 getter，这套反射式测试会悄无声息地偏离真实行为。建议后续为ProxySession增加一个测试工厂（例如ProxySession.forTest(...)）统一构造，避免继续扩大反射式构造的面。非本 PR 必须修复。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/proxy/proxy-forwarder-endpoint-audit.test.ts` around lines 171 - 220, The test helper createSession currently fabricates a ProxySession using Object.create + Object.assign and manually sets endpointPolicy and getEndpointPolicy, which can drift from the real ProxySession behavior (see getEndpointPolicy, endpointPolicy, isRawCrossProviderFallbackEnabled); replace this by adding a test factory on the real class (e.g. static ProxySession.forTest(options) that constructs a real ProxySession instance with sensible defaults and accepts overrides) and update createSession to call ProxySession.forTest(...) instead of using Object.create, so private/readonly fields and any lazy/getter logic in ProxySession remain accurate in tests.src/app/v1/_lib/proxy/response-handler.ts (1)
3604-3617: 计费归零路径内部冗余，但通过“双重防线”保证了正确性注意到这里把
normalizedUsage而不是billableNormalizedUsage再次喂给updateRequestCostFromUsage（3611）和trackCostToRedis（3625），它们内部分别在 3412、3722 行对 non-billing endpoint 做了短路，所以结果仍然正确。双重防线在这里是好事，但读起来容易让人以为有遗漏。如果想让阅读更顺畅，可把
billableNormalizedUsage的变量名提示语义，或在调用两个下游前集中注释一次“非计费端点的成本/限额写入依赖下游内部短路”。非强制。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/response-handler.ts` around lines 3604 - 3617, The code uses normalizedUsage (not billableNormalizedUsage) when calling updateRequestCostFromUsage and trackCostToRedis which is redundant and confusing because those callee functions short-circuit non-billing endpoints; change the calls to pass billableNormalizedUsage instead of normalizedUsage (references: billableNormalizedUsage, normalizedUsage, updateRequestCostFromUsage, trackCostToRedis, isNonBillingUsageEndpoint, maybeSetCodexContext1m) so the intent is clear, or if you prefer to keep normalizedUsage for a deliberate “double‑check” safety, add a single clarifying comment above the two calls stating “non-billing endpoints are handled by internal short-circuit in updateRequestCostFromUsage/trackCostToRedis” to avoid reader confusion.src/app/v1/_lib/proxy/forwarder.ts (4)
958-995:endpointPolicy在send中被声明了两次（L960 外层循环前、L992 外层循环体内），含义相同。
ProxyForwarder.getEndpointPolicy(session)在同一请求生命周期里返回同一个策略（基于session.requestUrl.pathname），外层已计算过，内层可以直接复用。移除内层声明避免读者误以为两者可能不同，也能略微省去一次原型方法解析。建议
- const endpointPolicy = ProxyForwarder.getEndpointPolicy(session); const shouldAccountCircuitBreaker = endpointPolicy.allowCircuitBreakerAccounting; const shouldEnforceStrictEndpointPool = !isMcpRequest && isStrictEndpointPoolPolicy(endpointPolicy) && providerVendorId > 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/forwarder.ts` around lines 958 - 995, The duplicate declaration of endpointPolicy inside send masks the outer value and causes an unnecessary second call to ProxyForwarder.getEndpointPolicy(session); remove the inner declaration at the start of the outer loop body and reuse the outer endpointPolicy variable instead (keep the outer const endpointPolicy = ProxyForwarder.getEndpointPolicy(session); and delete the inner const endpointPolicy = ProxyForwarder.getEndpointPolicy(session); so subsequent uses like shouldAccountCircuitBreaker and isStrictEndpointPoolPolicy refer to the same endpointPolicy).
1929-1942: 此处的!endpointPolicy.allowRetry && !rawCrossProviderFallbackEnabled与外层shouldSkipRawRetryAndProviderSwitch语义完全一致（L961-962）。其它几个分支（L1667、L1783、L1850）都已经直接使用
shouldSkipRawRetryAndProviderSwitch；这里保留原始写法不一致，且日志消息重复表达同一语义。建议统一：建议
- if (!endpointPolicy.allowRetry && !rawCrossProviderFallbackEnabled) { + if (shouldSkipRawRetryAndProviderSwitch) { logger.debug( "ProxyForwarder: raw passthrough endpoint error, skipping circuit breaker and provider switch",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/forwarder.ts` around lines 1929 - 1942, The condition here duplicates the same semantics as the existing boolean shouldSkipRawRetryAndProviderSwitch; update the if block in ProxyForwarder to use shouldSkipRawRetryAndProviderSwitch instead of manually checking !endpointPolicy.allowRetry && !rawCrossProviderFallbackEnabled, and simplify the logger.debug message to reference shouldSkipRawRetryAndProviderSwitch (remove the redundant "skipping circuit breaker and provider switch" phrasing) while preserving the same structured fields (providerId, providerName, statusCode, error, policyKind), then throw lastError as before.
534-649:isSessionRawCrossProviderFallbackEnabled(session) ? { …无 upstream … } : { …含 upstream … }的三元结构在本文件内重复了 5 次。分别出现在：
buildClientErrorChainEntry(L544-560)buildRetryFailedChainEntry(L601-617)resource_not_found分支 (L1763-1778)vendor_type_all_timeout分支 (L1900-1915)retry_failed（PROVIDER_ERROR）分支 (L1969-1984)每份副本都必须同步维护"raw 模式下剥离
upstreamBody/upstreamParsed"的策略，未来只要任何一处漏改，就会在 raw 回退路径上泄漏上游响应体到决策链。建议抽取一个小 helper，把策略集中到一处：建议重构
function buildChainProviderDetails( session: ProxySession, provider: Provider, statusCode: number | undefined, statusText: string, upstream: ProxyError["upstreamError"] | undefined ) { const base = { id: provider.id, name: provider.name, statusCode, statusText }; if (isSessionRawCrossProviderFallbackEnabled(session)) { return base; } return { ...base, upstreamBody: upstream?.body, upstreamParsed: upstream?.parsed, }; }调用点可改为
provider: buildChainProviderDetails(session, currentProvider, statusCode, proxyError.message, proxyError.upstreamError)等，显著降低维护成本并减少漏改风险。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/forwarder.ts` around lines 534 - 649, The repeated ternary that strips upstreamBody/upstreamParsed for raw cross-provider fallback should be extracted into a single helper (e.g., buildChainProviderDetails(session, provider, statusCode, statusText, upstream)) and used wherever provider-details are built (replace the inline ternary in buildClientErrorChainEntry and buildRetryFailedChainEntry and the other branches noted: resource_not_found, vendor_type_all_timeout, retry_failed (PROVIDER_ERROR)). Implement the helper to return { id, name, statusCode, statusText } when isSessionRawCrossProviderFallbackEnabled(session) is true, otherwise include upstreamBody and upstreamParsed from upstream; then call that helper at each location and remove the duplicated ternary logic.
3377-3383:isRawCrossProviderFallbackEnabled没有真正的异步工作，可以简化且去掉async。当前实现等价于直接返回
isSessionRawCrossProviderFallbackEnabled(session)，但被写成async并通过!x ? false : true绕了一圈，调用侧的await（L959）也会额外产生一次微任务切换。建议简化
- private static async isRawCrossProviderFallbackEnabled(session: ProxySession): Promise<boolean> { - if (!isSessionRawCrossProviderFallbackEnabled(session)) { - return false; - } - - return true; - } + private static isRawCrossProviderFallbackEnabled(session: ProxySession): boolean { + return isSessionRawCrossProviderFallbackEnabled(session); + }对应调用点（L958-959）去掉
await即可。后续如果确实需要查询getCachedSystemSettings()再改回 async 也很直接。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/forwarder.ts` around lines 3377 - 3383, The method isRawCrossProviderFallbackEnabled currently adds unnecessary async/await overhead; change its implementation to non-async and simply return isSessionRawCrossProviderFallbackEnabled(session) (i.e., remove async and the redundant conditional), and update callers that do "await isRawCrossProviderFallbackEnabled(...)" to call it synchronously (remove the await). Refer to the symbols isRawCrossProviderFallbackEnabled and isSessionRawCrossProviderFallbackEnabled and remove the extra microtask by adjusting both the function and its call sites.src/repository/_shared/usage-log-filters.ts (1)
17-17: 建议对DEFAULT_HIDDEN_USAGE_LOG_ENDPOINTS做一次规范化，确保与buildNormalizedEndpointSql对齐。
buildDefaultHiddenUsageLogEndpointCondition在 SQL 侧对列做了LOWER(REGEXP_REPLACE(..., '/+$', ''))规范化，但右侧比较值直接来自NON_BILLING_ENDPOINTS。如果未来NON_BILLING_ENDPOINTS里出现任何大小写或尾部斜杠差异，NOT IN会漏掉记录（即应被隐藏的端点被意外暴露）。在构造时统一规范化一次可以消除该隐性耦合：建议修改
-export const DEFAULT_HIDDEN_USAGE_LOG_ENDPOINTS = [...NON_BILLING_ENDPOINTS]; +export const DEFAULT_HIDDEN_USAGE_LOG_ENDPOINTS = NON_BILLING_ENDPOINTS.map((endpoint) => + endpoint.trim().toLowerCase().replace(/\/+$/, "") || endpoint +);备注：如果
NON_BILLING_ENDPOINTS已有明确约束（全小写、无尾斜杠），可以加一个单元测试守住该约束，或者直接Object.freeze并注释说明。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/repository/_shared/usage-log-filters.ts` at line 17, DEFAULT_HIDDEN_USAGE_LOG_ENDPOINTS must be normalized to match the SQL-side normalization performed by buildNormalizedEndpointSql/buildDefaultHiddenUsageLogEndpointCondition; update DEFAULT_HIDDEN_USAGE_LOG_ENDPOINTS (constructed from NON_BILLING_ENDPOINTS) to map each entry to a normalized form (apply LOWER and strip trailing slashes, e.g., value.toLowerCase().replace(/\/+$/, '')) so the NOT IN comparison is correct, and consider freezing the array or adding a unit test to enforce that NON_BILLING_ENDPOINTS entries remain normalized if you choose not to normalize at construction time; update references to DEFAULT_HIDDEN_USAGE_LOG_ENDPOINTS accordingly.src/app/v1/_lib/proxy/guard-pipeline.ts (2)
161-172: 缓存session.getEndpointPolicy()的返回值，避免重复调用。当
isRawCrossProviderFallbackEnabled方法不存在时，session.getEndpointPolicy()会被调用两次（L167、L170），且需要将同一个策略对象同时传给fromEndpointPolicy。虽然通常开销可忽略，但会话上的 getter 实现可能存在副作用或内部计算，先取一次更稳妥。建议重构
static fromSession( session: Pick<ProxySession, "getEndpointPolicy"> & { isRawCrossProviderFallbackEnabled?: (() => boolean) | undefined; } ): GuardPipeline { - return GuardPipelineBuilder.fromEndpointPolicy( - session.getEndpointPolicy(), - typeof session.isRawCrossProviderFallbackEnabled === "function" - ? session.isRawCrossProviderFallbackEnabled() - : session.getEndpointPolicy().allowRawCrossProviderFallback - ); + const policy = session.getEndpointPolicy(); + const rawEnabled = + typeof session.isRawCrossProviderFallbackEnabled === "function" + ? session.isRawCrossProviderFallbackEnabled() + : policy.allowRawCrossProviderFallback; + return GuardPipelineBuilder.fromEndpointPolicy(policy, rawEnabled); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/guard-pipeline.ts` around lines 161 - 172, In GuardPipeline.fromSession, avoid calling session.getEndpointPolicy() twice; cache its result in a local variable (e.g., endpointPolicy) and use that same object when calling GuardPipelineBuilder.fromEndpointPolicy and when reading allowRawCrossProviderFallback as the fallback for isRawCrossProviderFallbackEnabled; this ensures the same policy instance is passed and prevents repeated getter side effects when evaluating session.getEndpointPolicy() and session.isRawCrossProviderFallbackEnabled().
223-227:COUNT_TOKENS_PIPELINE作为RAW_SAFE_SESSION_PIPELINE的别名导出，建议在注释中标明用途与弃用倾向。保留兼容命名是合理的，但没有任何注释说明"新代码应直接使用
RAW_SAFE_SESSION_PIPELINE"。加一行 JSDoc 可避免未来有人基于命名（COUNT_TOKENS_PIPELINE）误以为它仅服务于 count_tokens 端点而去扩展它：建议
+/** + * `@deprecated` 保留用于兼容；新代码请直接使用 `RAW_SAFE_SESSION_PIPELINE`。 + * 该别名的语义是"带 session/messageContext 的 raw passthrough 流水线"， + * 不再与具体端点（count_tokens）耦合。 + */ export const COUNT_TOKENS_PIPELINE: GuardConfig = RAW_SAFE_SESSION_PIPELINE;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/guard-pipeline.ts` around lines 223 - 227, COUNT_TOKENS_PIPELINE is exported as an alias of RAW_SAFE_SESSION_PIPELINE without any doc explaining intent; add a JSDoc comment above COUNT_TOKENS_PIPELINE stating that it is a compatibility alias for RAW_SAFE_SESSION_PIPELINE, mark it as deprecated for new code and instruct callers to use RAW_SAFE_SESSION_PIPELINE directly (mention both identifiers RAW_SAFE_SESSION_PIPELINE and COUNT_TOKENS_PIPELINE so reviewers can find the change).tests/unit/proxy/non-chat-endpoint-fallback.test.ts (2)
161-226: 测试会话通过Object.create(ProxySession.prototype)手工拼装，依赖ProxySession当前的内部字段布局。当
ProxySession后续新增必需的私有状态（例如在构造器里初始化的集合/映射）时，这些测试会静默漂移——方法在原型上可以调用，但内部状态缺失。可以考虑后续抽取一个受测类专用的createFakeProxySession(overrides)工厂，把字段初始化集中维护，降低这类维护成本。当前实现可接受。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/proxy/non-chat-endpoint-fallback.test.ts` around lines 161 - 226, Tests build a ProxySession by Object.create(ProxySession.prototype) which relies on the current internal field layout and will break silently if ProxySession later adds required private state; replace or supplement createRawSession with a dedicated factory (e.g., createFakeProxySession(overrides)) that centrally initializes all internal fields and any collections/maps/sets that ProxySession constructor normally creates, merge caller overrides, and ensure methods like setProvider, addProviderToChain, getProviderChain, isHeaderModified, etc. operate against those initialized defaults so future private-state additions won't silently break tests.
289-406:disabled setting四个用例的 session 组装完全同构，可以抽取共享 setup 降噪。
RESPONSES_COMPACT+codexprovider +responseoriginalFormat +gpt-5message 的组合在 4 个test中逐字重复。抽成buildDisabledRawCompactSession()辅助可以让每个用例只保留"它在测什么"的差异（错误分类、抛出的错误类型），提升可读性。属于可选优化。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/proxy/non-chat-endpoint-fallback.test.ts` around lines 289 - 406, The four tests duplicate identical session setup (V1_ENDPOINT_PATHS.RESPONSES_COMPACT, createRawSession, createProvider(...,{providerType:"codex"}), session.originalFormat="response", session.setRawCrossProviderFallbackEnabled(false), session.request.message { model: "gpt-5", ... }, session.setProvider(providerA}); refactor by extracting a helper (e.g., buildDisabledRawCompactSession()) that performs this common assembly and returns the session (and provider if needed), then replace the repeated blocks in the tests with calls to that helper and keep each test only showing its differing parts (mocked categorizeErrorAsync/doForward behavior and expected assertions).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@drizzle/0096_equal_selene.sql`:
- Line 47: v_is_success currently only checks NEW.error_message which can mark
4xx/5xx responses as successful if error_message is empty; update the
v_is_success assignment to also validate NEW.status_code (e.g., require
NEW.status_code BETWEEN 200 AND 299 or NEW.status_code < 400) in addition to
(NEW.error_message IS NULL OR NEW.error_message = ''), so the trigger/assignment
that sets v_is_success rejects non-2xx status codes; modify the expression that
assigns v_is_success (referencing v_is_success, NEW.error_message, and
NEW.status_code) accordingly.
In `@src/app/v1/_lib/proxy/response-handler.ts`:
- Line 3606: maybeSetCodexContext1m is being called for non-billing endpoints
(/v1/messages/count_tokens and /v1/responses/compact) which writes
context_1m_applied to the DB; while current logic (updateRequestCostFromUsage
early return, calculateRequestCost comments, and tests) prevents billing impact,
make intent explicit: either remove/skip calling maybeSetCodexContext1m for
those non-billing paths or add a guard in maybeSetCodexContext1m (or the caller
in response-handler.ts) to no-op when handling non-billing endpoints, and ensure
any persisted flag stays purely informational (reference functions
maybeSetCodexContext1m, updateRequestCostFromUsage, calculateRequestCost and the
context_1m_applied field/ledger-backfill flow).
In `@src/lib/config/system-settings-cache.ts`:
- Line 61: Change the fail-open default so cold-cache/db-read failures don't
re-enable cross-vendor raw fallback: set
allowNonConversationEndpointProviderFallback to false in the DEFAULT_SETTINGS
object (and any duplicate DEFAULT_SETTINGS or literal defaults at the other
occurrence around lines 140-141) so that getCachedSystemSettings() returning
DEFAULT_SETTINGS on DB errors will be fail-closed; update the same flag wherever
the default settings are defined to keep behavior consistent.
In `@src/lib/session-manager.ts`:
- Around line 646-655: The SessionManager code currently only rejects a provider
binding when a bound key exists but mismatches; update the check in the method
containing the shown block (the SessionManager code that reads
`redis.get('session:${sessionId}:key')`) to treat a missing boundKeyId as a
validation failure as well — i.e., if `boundKeyId` is falsy or differs from
`keyId.toString()` then log the warning and return null so we never reuse a
provider when the session:key is absent; alternatively ensure any codepath that
writes `session:${sessionId}:provider` also atomically writes
`session:${sessionId}:key` with the same TTL, but the immediate fix is to change
the conditional around `boundKeyId` to deny when it's null/undefined.
In `@src/repository/_shared/usage-log-filters.ts`:
- Around line 156-160: filters.endpoint currently uses a truthy check which
treats pure-whitespace strings as true and causes
buildUsageLogEndpointMatchCondition (which returns null for empty trimmed input)
to push null into conditions; update the check to mirror
buildDefaultHiddenUsageLogEndpointCondition by testing trimmed content (e.g. use
filters.endpoint?.trim() or equivalent) before calling
buildUsageLogEndpointMatchCondition with messageRequest.endpoint so only
non-empty endpoints produce a SQL condition and null is never pushed into
conditions.
In `@src/repository/system-config.ts`:
- Around line 211-212: The current fallback/projection logic unconditionally
falls back to a smaller legacy/minimal projection that still references
systemSettings.allowNonConversationEndpointProviderFallback, which causes other
modern fields to be lost when only that new column is missing; add an
intermediate projection and corresponding read/returning/update downgrade that
only strips the allowNonConversationEndpointProviderFallback field (keep all
other modern systemSettings fields) and try that projection before falling back
to the broader legacy/minimal projection. Update the code paths that build
projections/returning clauses and the update logic that mentions
allowNonConversationEndpointProviderFallback so they first attempt the
"only-remove-this-new-column" projection, and apply the same change to the other
occurrences where projections are defined (the other blocks referencing
allowNonConversationEndpointProviderFallback).
In `@src/repository/usage-logs.ts`:
- Around line 1583-1589: The totalTokens calculation is double-counting cache
creation by adding totalCacheCreation5mTokens and totalCacheCreation1hTokens in
addition to totalCacheCreationTokens; update the logic in the totalTokens
assignment (the variable named totalTokens that reads summaryResult.* fields) to
match other branches by only summing
input+output+totalCacheCreationTokens+totalCacheReadTokens (i.e., remove or
replace addition of totalCacheCreation5mTokens and totalCacheCreation1hTokens),
or alternatively compute cacheCreation = totalCacheCreationTokens ||
(totalCacheCreation5mTokens + totalCacheCreation1hTokens) and use that single
cacheCreation value in the totalTokens sum so all endpoints use the same
cache-creation accounting.
In `@tests/unit/repository/usage-logs-min-retry-count-filter.test.ts`:
- Around line 45-49: The test obtains `condition` from
`buildUsageLogConditions({ minRetryCount: 1 })` and then calls
`sqlToString(condition)`, but under strict null checks `condition` has type `SQL
| undefined`; after the `expect(condition).toBeDefined()` you must narrow the
type—either use a non-null assertion when passing to `sqlToString` (e.g.
`sqlToString(condition!)`) or add an explicit runtime guard that throws or fails
the test if `condition` is undefined (e.g. `if (!condition) throw new
Error("expected condition")`) before calling `sqlToString`; apply this change
where `condition` and `sqlToString` are used in the test.
---
Outside diff comments:
In `@src/app/v1/_lib/proxy/guard-pipeline.ts`:
- Around line 189-196: fromRequestType currently hardcodes
RequestType.COUNT_TOKENS to RAW_SAFE_SESSION_PIPELINE which is inconsistent with
fromSession/fromEndpointPolicy that respect allowRawCrossProviderFallback;
update fromRequestType(RequestType.COUNT_TOKENS) to mirror the same conditional
selection between RAW_SAFE_SESSION_PIPELINE and RAW_PASSTHROUGH_PIPELINE (using
the same flag/logic as fromSession/fromEndpointPolicy) or, if the hardcoded
behavior is intentional for tests, add a clear comment above fromRequestType
explaining it is test-only and why RAW_SAFE_SESSION_PIPELINE is required
(reference symbols: fromRequestType, RequestType.COUNT_TOKENS,
RAW_SAFE_SESSION_PIPELINE, RAW_PASSTHROUGH_PIPELINE, fromSession,
fromEndpointPolicy, allowRawCrossProviderFallback,
guard-pipeline-warmup.test.ts).
In `@tests/unit/proxy/endpoint-policy-parity.test.ts`:
- Around line 239-245: The test name is misleading because the default policy no
longer has all allow flags true (specifically allowRawCrossProviderFallback
remains false); update the test title string in the test that iterates over
DEFAULT_ENDPOINTS and calls resolveEndpointPolicy(pathname) to reflect the new
semantics (e.g., "default policy has expected allow flags" or "default policy
has allow flags set (raw cross-provider fallback false)"), leaving the
assertions on policy.allowRetry, policy.allowProviderSwitch,
policy.allowRawCrossProviderFallback, and policy.allowCircuitBreakerAccounting
unchanged.
In `@tests/unit/proxy/proxy-forwarder-hedge-first-byte.test.ts`:
- Around line 1723-1787: Update the test to assert the specific provider2
hedge-winner entry instead of loosely matching any "request_success" or
"hedge_winner": change the chain lookup that assigns winnerEntry to find an
entry where entry.provider?.id === 2 && entry.reason === "hedge_winner" (using
session.getProviderChain()), and update the test name string to reflect that
provider2 should be the hedge_winner; keep the existing assertions that the
response contains "provider":"p2" and session.provider?.id === 2.
---
Nitpick comments:
In `@src/app/v1/_lib/proxy-handler.ts`:
- Around line 24-40: The code is using a type intersection and optional chaining
to call setRawCrossProviderFallbackEnabled on session; since ProxySession
already publicly defines setRawCrossProviderFallbackEnabled (see ProxySession),
remove the cast and optional call and invoke
session.setRawCrossProviderFallbackEnabled(...) directly in both places (the try
block and the catch block), keeping the same boolean arguments; also keep the
existing logger.warn and session.setHighConcurrencyModeEnabled(false) behavior
unchanged.
In `@src/app/v1/_lib/proxy/forwarder.ts`:
- Around line 958-995: The duplicate declaration of endpointPolicy inside send
masks the outer value and causes an unnecessary second call to
ProxyForwarder.getEndpointPolicy(session); remove the inner declaration at the
start of the outer loop body and reuse the outer endpointPolicy variable instead
(keep the outer const endpointPolicy =
ProxyForwarder.getEndpointPolicy(session); and delete the inner const
endpointPolicy = ProxyForwarder.getEndpointPolicy(session); so subsequent uses
like shouldAccountCircuitBreaker and isStrictEndpointPoolPolicy refer to the
same endpointPolicy).
- Around line 1929-1942: The condition here duplicates the same semantics as the
existing boolean shouldSkipRawRetryAndProviderSwitch; update the if block in
ProxyForwarder to use shouldSkipRawRetryAndProviderSwitch instead of manually
checking !endpointPolicy.allowRetry && !rawCrossProviderFallbackEnabled, and
simplify the logger.debug message to reference
shouldSkipRawRetryAndProviderSwitch (remove the redundant "skipping circuit
breaker and provider switch" phrasing) while preserving the same structured
fields (providerId, providerName, statusCode, error, policyKind), then throw
lastError as before.
- Around line 534-649: The repeated ternary that strips
upstreamBody/upstreamParsed for raw cross-provider fallback should be extracted
into a single helper (e.g., buildChainProviderDetails(session, provider,
statusCode, statusText, upstream)) and used wherever provider-details are built
(replace the inline ternary in buildClientErrorChainEntry and
buildRetryFailedChainEntry and the other branches noted: resource_not_found,
vendor_type_all_timeout, retry_failed (PROVIDER_ERROR)). Implement the helper to
return { id, name, statusCode, statusText } when
isSessionRawCrossProviderFallbackEnabled(session) is true, otherwise include
upstreamBody and upstreamParsed from upstream; then call that helper at each
location and remove the duplicated ternary logic.
- Around line 3377-3383: The method isRawCrossProviderFallbackEnabled currently
adds unnecessary async/await overhead; change its implementation to non-async
and simply return isSessionRawCrossProviderFallbackEnabled(session) (i.e.,
remove async and the redundant conditional), and update callers that do "await
isRawCrossProviderFallbackEnabled(...)" to call it synchronously (remove the
await). Refer to the symbols isRawCrossProviderFallbackEnabled and
isSessionRawCrossProviderFallbackEnabled and remove the extra microtask by
adjusting both the function and its call sites.
In `@src/app/v1/_lib/proxy/guard-pipeline.ts`:
- Around line 161-172: In GuardPipeline.fromSession, avoid calling
session.getEndpointPolicy() twice; cache its result in a local variable (e.g.,
endpointPolicy) and use that same object when calling
GuardPipelineBuilder.fromEndpointPolicy and when reading
allowRawCrossProviderFallback as the fallback for
isRawCrossProviderFallbackEnabled; this ensures the same policy instance is
passed and prevents repeated getter side effects when evaluating
session.getEndpointPolicy() and session.isRawCrossProviderFallbackEnabled().
- Around line 223-227: COUNT_TOKENS_PIPELINE is exported as an alias of
RAW_SAFE_SESSION_PIPELINE without any doc explaining intent; add a JSDoc comment
above COUNT_TOKENS_PIPELINE stating that it is a compatibility alias for
RAW_SAFE_SESSION_PIPELINE, mark it as deprecated for new code and instruct
callers to use RAW_SAFE_SESSION_PIPELINE directly (mention both identifiers
RAW_SAFE_SESSION_PIPELINE and COUNT_TOKENS_PIPELINE so reviewers can find the
change).
In `@src/app/v1/_lib/proxy/response-handler.ts`:
- Around line 3604-3617: The code uses normalizedUsage (not
billableNormalizedUsage) when calling updateRequestCostFromUsage and
trackCostToRedis which is redundant and confusing because those callee functions
short-circuit non-billing endpoints; change the calls to pass
billableNormalizedUsage instead of normalizedUsage (references:
billableNormalizedUsage, normalizedUsage, updateRequestCostFromUsage,
trackCostToRedis, isNonBillingUsageEndpoint, maybeSetCodexContext1m) so the
intent is clear, or if you prefer to keep normalizedUsage for a deliberate
“double‑check” safety, add a single clarifying comment above the two calls
stating “non-billing endpoints are handled by internal short-circuit in
updateRequestCostFromUsage/trackCostToRedis” to avoid reader confusion.
In `@src/app/v1/_lib/proxy/session.ts`:
- Around line 357-365: The isRawCrossProviderFallbackEnabled method contains an
unnecessary fallback expression "this.endpointPolicy ??
resolveEndpointPolicy(...)" because endpointPolicy is readonly and already set
in the constructor via resolveSessionEndpointPolicy, so remove the fallback and
read endpointPolicy directly; update isRawCrossProviderFallbackEnabled to
reference this.endpointPolicy only and keep the boolean expression using
this.endpointPolicy.allowRawCrossProviderFallback and
this.rawCrossProviderFallbackEnabled to simplify the code and avoid implying
endpointPolicy can be undefined.
In `@src/lib/ledger-backfill/trigger.sql`:
- Around line 172-176: 在触发器中针对非对话端点（使用
LOWER(REGEXP_REPLACE(COALESCE(NEW.endpoint, ''), '/+$', '')) 与
'/v1/messages/count_tokens'、'/v1/responses/compact' 比对）直接 DELETE FROM
usage_ledger WHERE request_id = NEW.id 并 RETURN NEW 的分支需要补充注释，说明之所以 DELETE
而不是仅跳过 UPSERT 是为了清理可能在历史写入/旧对话端点写入时已存在的 ledger 行，避免遗留无效记录；请在该分支上方添加一行简短注释，明确引用
NEW、usage_ledger 和归一化规则以便后续维护者理解语义和原因。
In `@src/repository/_shared/ledger-conditions.ts`:
- Around line 5-20: The SQL endpoint normalization in
NON_BILLING_LEDGER_ENDPOINT_CONDITION differs from the JS isNonBillingEndpoint:
it doesn't trim whitespace first, so align them by applying a trim step to
${usageLedger.endpoint} before lowercasing and removing trailing slashes; update
the expression used in NON_BILLING_LEDGER_ENDPOINT_CONDITION (which is also
referenced by LEDGER_BILLING_CONDITION) to perform whitespace trimming then
REGEXP_REPLACE('/+$', '') and LOWER, and keep the comparison against the joined
NON_BILLING_ENDPOINTS unchanged.
In `@src/repository/_shared/usage-log-filters.ts`:
- Line 17: DEFAULT_HIDDEN_USAGE_LOG_ENDPOINTS must be normalized to match the
SQL-side normalization performed by
buildNormalizedEndpointSql/buildDefaultHiddenUsageLogEndpointCondition; update
DEFAULT_HIDDEN_USAGE_LOG_ENDPOINTS (constructed from NON_BILLING_ENDPOINTS) to
map each entry to a normalized form (apply LOWER and strip trailing slashes,
e.g., value.toLowerCase().replace(/\/+$/, '')) so the NOT IN comparison is
correct, and consider freezing the array or adding a unit test to enforce that
NON_BILLING_ENDPOINTS entries remain normalized if you choose not to normalize
at construction time; update references to DEFAULT_HIDDEN_USAGE_LOG_ENDPOINTS
accordingly.
In `@tests/unit/proxy/non-chat-endpoint-fallback.test.ts`:
- Around line 161-226: Tests build a ProxySession by
Object.create(ProxySession.prototype) which relies on the current internal field
layout and will break silently if ProxySession later adds required private
state; replace or supplement createRawSession with a dedicated factory (e.g.,
createFakeProxySession(overrides)) that centrally initializes all internal
fields and any collections/maps/sets that ProxySession constructor normally
creates, merge caller overrides, and ensure methods like setProvider,
addProviderToChain, getProviderChain, isHeaderModified, etc. operate against
those initialized defaults so future private-state additions won't silently
break tests.
- Around line 289-406: The four tests duplicate identical session setup
(V1_ENDPOINT_PATHS.RESPONSES_COMPACT, createRawSession,
createProvider(...,{providerType:"codex"}), session.originalFormat="response",
session.setRawCrossProviderFallbackEnabled(false), session.request.message {
model: "gpt-5", ... }, session.setProvider(providerA}); refactor by extracting a
helper (e.g., buildDisabledRawCompactSession()) that performs this common
assembly and returns the session (and provider if needed), then replace the
repeated blocks in the tests with calls to that helper and keep each test only
showing its differing parts (mocked categorizeErrorAsync/doForward behavior and
expected assertions).
In `@tests/unit/proxy/proxy-forwarder-endpoint-audit.test.ts`:
- Around line 171-220: The test helper createSession currently fabricates a
ProxySession using Object.create + Object.assign and manually sets
endpointPolicy and getEndpointPolicy, which can drift from the real ProxySession
behavior (see getEndpointPolicy, endpointPolicy,
isRawCrossProviderFallbackEnabled); replace this by adding a test factory on the
real class (e.g. static ProxySession.forTest(options) that constructs a real
ProxySession instance with sensible defaults and accepts overrides) and update
createSession to call ProxySession.forTest(...) instead of using Object.create,
so private/readonly fields and any lazy/getter logic in ProxySession remain
accurate in tests.
🪄 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: 8ae90dda-e6f1-4fa9-8437-cc2b72a4adb9
📒 Files selected for processing (55)
drizzle/0096_equal_selene.sqldrizzle/meta/0096_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/my-usage.tssrc/actions/system-config.tssrc/app/[locale]/dashboard/logs/_components/usage-logs-table.tsxsrc/app/[locale]/dashboard/logs/_components/virtualized-logs-table.tsxsrc/app/[locale]/settings/config/_components/system-settings-form.tsxsrc/app/[locale]/settings/config/page.tsxsrc/app/v1/_lib/proxy-handler.tssrc/app/v1/_lib/proxy/endpoint-policy.tssrc/app/v1/_lib/proxy/forwarder.tssrc/app/v1/_lib/proxy/guard-pipeline.tssrc/app/v1/_lib/proxy/provider-selector.tssrc/app/v1/_lib/proxy/response-handler.tssrc/app/v1/_lib/proxy/session-guard.tssrc/app/v1/_lib/proxy/session.tssrc/drizzle/schema.tssrc/lib/config/system-settings-cache.tssrc/lib/ledger-backfill/service.tssrc/lib/ledger-backfill/trigger.sqlsrc/lib/session-manager.tssrc/lib/utils/performance-formatter.tssrc/lib/validation/schemas.tssrc/repository/_shared/ledger-conditions.tssrc/repository/_shared/transformers.tssrc/repository/_shared/usage-log-filters.tssrc/repository/system-config.tssrc/repository/usage-logs.tssrc/types/system-config.tstests/configs/integration.config.tstests/integration/non-chat-endpoint-fallback-observability.test.tstests/unit/actions/system-config-non-chat-retry-setting.test.tstests/unit/actions/usage-logs-hidden-non-chat-endpoints.test.tstests/unit/proxy/endpoint-policy-parity.test.tstests/unit/proxy/endpoint-policy.test.tstests/unit/proxy/guard-pipeline-warmup.test.tstests/unit/proxy/non-chat-endpoint-fallback.test.tstests/unit/proxy/non-chat-endpoint-non-billing.test.tstests/unit/proxy/non-chat-endpoint-policy.test.tstests/unit/proxy/non-chat-endpoint-session-context.test.tstests/unit/proxy/provider-selector-model-mismatch-binding.test.tstests/unit/proxy/proxy-forwarder-endpoint-audit.test.tstests/unit/proxy/proxy-forwarder-hedge-first-byte.test.tstests/unit/proxy/proxy-forwarder-retry-limit.test.tstests/unit/proxy/response-handler-lease-decrement.test.tstests/unit/proxy/session-guard-warmup-intercept.test.tstests/unit/repository/key.test.tstests/unit/repository/usage-logs-min-retry-count-filter.test.tstests/unit/settings/system-settings-form-non-chat-fallback.test.tsx
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
ding113
force-pushed
the
fix/non-chat-endpoint-fallback-20260423T113809Z
branch
from
April 23, 2026 19:46
3b5621e to
9f8b5f4
Compare
3 tasks
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f8b5f433a
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/ledger-backfill/service.ts (1)
42-107:⚠️ Potential issue | 🟠 Major回填里的
is_success口径还没和新触发器对齐。Line 74 这里仍然只看
error_message，但 0098 里的新fn_upsert_usage_ledger()已经改成同时要求status_code IS NULL OR < 400。这样一旦执行 backfill，usage_ledger.is_success会对同类记录出现“实时写入=false、回填=true”的分裂结果，统计会不稳定。这里也要同步成和触发器一致的判定。建议修复
- (mr.error_message IS NULL OR mr.error_message = '') AS is_success, + ( + (mr.error_message IS NULL OR mr.error_message = '') + AND (mr.status_code IS NULL OR mr.status_code < 400) + ) AS is_success,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/ledger-backfill/service.ts` around lines 42 - 107, The backfill SELECT sets is_success using only mr.error_message but must match the new trigger logic in fn_upsert_usage_ledger; update the is_success expression in the batch SELECT (the column alias is_success in the query against message_request mr used to populate usage_ledger) to require both no error_message and a successy status_code (i.e., status_code IS NULL OR status_code < 400) so the backfill and fn_upsert_usage_ledger produce identical is_success values and avoid split results between real-time writes and backfill.
♻️ Duplicate comments (1)
src/app/v1/_lib/proxy/response-handler.ts (1)
2277-2291:⚠️ Potential issue | 🟠 Major流式路径里仍然会把非计费端点写成
context1mApplied=true。Line 2290 还在
billableUsageForCost计算之前调用maybeSetCodexContext1m。这样一来，/v1/messages/count_tokens或/v1/responses/compact一旦走到 SSE 结算路径，仍然可能把当前 session 标成 1M context，后续真实计费请求就会继承这个脏状态。建议修改
if (usageForCost) { usageForCost = normalizeUsageWithSwap( usageForCost, session, provider.swapCacheTtlBilling ); } - - maybeSetCodexContext1m(session, provider, usageForCost?.input_tokens); + const billableUsageForCost = + usageForCost && !isNonBillingUsageEndpoint(session) ? usageForCost : null; + + if (billableUsageForCost) { + maybeSetCodexContext1m(session, provider, billableUsageForCost.input_tokens); + } // Codex: Extract prompt_cache_key from SSE events and update session binding @@ - const billableUsageForCost = - usageForCost && !isNonBillingUsageEndpoint(session) ? usageForCost : null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/response-handler.ts` around lines 2277 - 2291, The stream/SSE path sets context1mApplied too early—maybeSetCodexContext1m is called before computing billable usage, causing non-billing endpoints (isNonBillingUsageEndpoint) to be marked as 1M and pollute later billing; fix by moving the maybeSetCodexContext1m(...) call to after billable usage is computed (after usageForCost / billableUsageForCost normalization) and only invoke it when the request is confirmed billable (i.e., ensure isNonBillingUsageEndpoint(session) is false and priorityServiceTierApplied logic remains correct); adjust calls around normalizeUsageWithSwap and ensureCodexServiceTierResultSpecialSetting to ensure context1mApplied is only set for true billable paths.
🧹 Nitpick comments (8)
src/app/v1/_lib/proxy/session-guard.ts (2)
78-82: 冗余回读：allowRawSessionContext可直接使用局部变量第 82 行
session.isRawCrossProviderFallbackEnabled()只是回读刚在 81 行 setter 写入的同一个值，没有任何旁路/异步观察者会在 setter 和 getter 之间改写它。直接复用rawFallbackEnabled可以减少一次属性访问、让阅读者少做一次心算，也能消除“setter 与 getter 语义是否真正对称”这类潜在疑问。♻️ 建议化简
const rawFallbackEnabled = (systemSettings.allowNonConversationEndpointProviderFallback ?? true) && session.getEndpointPolicy().allowRawCrossProviderFallback; session.setRawCrossProviderFallbackEnabled(rawFallbackEnabled); - const allowRawSessionContext = session.isRawCrossProviderFallbackEnabled(); + const allowRawSessionContext = rawFallbackEnabled;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/session-guard.ts` around lines 78 - 82, The code redundantly reads back the value just set; replace the call to session.isRawCrossProviderFallbackEnabled() and the local allowRawSessionContext with reuse of the computed rawFallbackEnabled: after computing rawFallbackEnabled and calling session.setRawCrossProviderFallbackEnabled(rawFallbackEnabled), assign allowRawSessionContext = rawFallbackEnabled (or remove allowRawSessionContext and use rawFallbackEnabled directly) so you avoid the extra getter call to session.isRawCrossProviderFallbackEnabled().
78-82: 跨 provider fallback 与 session 上下文注入的耦合值得在注释中点明现在
allowRawSessionContext === rawFallbackEnabled：只要允许 raw 跨供应商 fallback，就同时禁用 Codex session id 补全与 Claude metadata.user_id 注入。这个耦合在功能上合理（raw 透传不应被中间件改写请求体），但两个语义其实是独立的：
- "可跨供应商回退" = 决策链能在失败时切到别的 provider；
- "请求体保持原样（raw）" = 不能对用户负载做任何注入/补全。
一旦未来想要"允许 fallback 但仍做 user_id 注入"或反之，当前的命名会让读者误以为两者必然一致。建议：
- 在此块上方加一段注释，解释"两者当前语义绑定"的原因（即 count_tokens/compact 这两个端点的 raw 透传契约）；
- 或者把
allowRawSessionContext改名为更贴近行为的skipSessionContextMutation，明确它控制的是请求体改写而不是 fallback 决策。Also applies to: 103-103, 147-153
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/session-guard.ts` around lines 78 - 82, The current code binds the "allow raw cross-provider fallback" decision (rawFallbackEnabled) to session context mutation by deriving allowRawSessionContext from session.isRawCrossProviderFallbackEnabled(), which conflates fallback decision with whether request bodies may be mutated; either add a clear comment above this block explaining why these two behaviors are intentionally coupled (reference the count_tokens/compact endpoints' raw-pass-through contract) or rename allowRawSessionContext to a behavior-focused name such as skipSessionContextMutation and update all uses (session.setRawCrossProviderFallbackEnabled, session.isRawCrossProviderFallbackEnabled, and any logic that reads allowRawSessionContext) so it explicitly controls request-body mutation while leaving the fallback decision variable (rawFallbackEnabled) separate; apply the same change/comment at the other similar locations that mirror this pattern (the other occurrences around the same blocks).tests/unit/proxy/response-handler-lease-decrement.test.ts (1)
421-447: LGTM，建议补一条/v1/messages/count_tokens对称用例此用例验证了
/v1/responses/compact/变体（含尾斜杠，能覆盖归一化路径）下不触发计费相关的 Redis 追踪 / lease decrement，同时仍落库 token 统计，覆盖点合理。为防止未来回归只保护其中一个端点，建议用
it.each把/v1/messages/count_tokens也纳入同一套断言（尤其大小写、尾斜杠这类归一化差异），确保两个NON_BILLING_ENDPOINTS对称生效。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/proxy/response-handler-lease-decrement.test.ts` around lines 421 - 447, Add a symmetric test case covering the /v1/messages/count_tokens endpoint variants by converting the existing test into a parameterized it.each that iterates over the non-billing paths (including variants with/without trailing slash and different casing) and runs the same assertions: call ProxyResponseHandler.dispatch with the session.pathname set to each path, ensure RateLimitService.trackCost, trackUserDailyCost, and decrementLeaseBudget are not called, and verify updateMessageRequestDetails is called with the expected token fields; reference ProxyResponseHandler.dispatch, RateLimitService.trackCost/trackUserDailyCost/decrementLeaseBudget, and updateMessageRequestDetails to locate where to add the parameterized test.src/app/v1/_lib/proxy-handler.ts (1)
28-33: 日志级别建议与可控硅：加载失败是否应为 error系统设置加载失败会直接导致高并发、raw fallback 这两个开关被降级到
false，用户看到的是“功能悄然变差”。建议改为logger.error以便告警系统能捕获；同时把settingsError放到结构化字段里，避免被 logger 序列化策略丢失。此外消息里"fallback highConcurrency=false and rawCrossProviderFallback=false"属内部日志，保留英文无问题。♻️ 建议调整
- logger.warn( - "[ProxyHandler] Failed to load proxy system settings, fallback highConcurrency=false and rawCrossProviderFallback=false", - { - error: settingsError, - } - ); + logger.error( + "[ProxyHandler] Failed to load proxy system settings; degrading highConcurrency=false, rawCrossProviderFallback=false", + { err: settingsError } + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy-handler.ts` around lines 28 - 33, Change the log level from warn to error in the ProxyHandler settings load failure so alerts can trigger; in the logger call that currently references settingsError (inside src/app/v1/_lib/proxy-handler.ts), move the error into a dedicated structured field (e.g., { error: settingsError } or { settingsError }) as the second argument so the logger preserves the exception details rather than embedding it in the message, and keep the existing English message text about fallback highConcurrency/rawCrossProviderFallback unchanged.tests/unit/actions/usage-logs-hidden-non-chat-endpoints.test.ts (1)
51-64: 通过源码字符串扫描函数边界相对脆弱这里通过
indexOf("export async function getUsedEndpoints")与下一个函数名来切片源码，一旦src/repository/usage-logs.ts中这些函数被改名、拆分或调整顺序，测试会静默变空或跨段误匹配（例如空getUsedEndpointsSegment会让后续not.toContain(...)恒真）。建议至少加上对切片非空的断言，或用正则捕获函数体以避免假绿。♻️ 可选加固
+ expect(getUsedEndpointsSegment.length).toBeGreaterThan(0); + expect(getDistinctEndpointsSegment.length).toBeGreaterThan(0); expect(getUsedEndpointsSegment).not.toContain("buildDefaultHiddenUsageLogEndpointCondition"); expect(getDistinctEndpointsSegment).not.toContain( "buildDefaultHiddenUsageLogEndpointCondition" );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/actions/usage-logs-hidden-non-chat-endpoints.test.ts` around lines 51 - 64, The test uses brittle string slicing via indexOf("export async function getUsedEndpoints") and indexOf("export async function getDistinctEndpointsForKey") which can produce empty or wrong segments if functions are renamed/reordered; update the test to either (A) assert that getUsedEndpointsSegment and getDistinctEndpointsSegment are non-empty before asserting not.toContain("buildDefaultHiddenUsageLogEndpointCondition") or (B) replace the slicing with a regex that reliably captures the full function bodies (e.g. match /export async function getUsedEndpoints[\s\S]*?{[\s\S]*?^}/m or similar) so you directly search inside the captured bodies for buildDefaultHiddenUsageLogEndpointCondition; reference getUsedEndpointsSegment, getDistinctEndpointsSegment, getUsedEndpoints, getDistinctEndpointsForKey, and buildDefaultHiddenUsageLogEndpointCondition when making the change.src/app/v1/_lib/proxy/forwarder.ts (2)
1746-1761: provider 错误详情的“有无 upstreamBody/upstreamParsed”分支在多处重复Line 526-540 / 584-598（两个 builder 内部）以及 Line 1747-1761、1886-1900、1956-1970（三处内联）都使用同一结构
rawCrossProviderFallbackEnabled ? minimal : withUpstream构造errorDetails.provider。五处重复容易在未来调整字段时漏改，进而导致src/actions/my-usage.ts的 scrubbing 语义与 forwarder 出口脱节。建议抽取一个小工具函数统一生成 provider 字段（例如
buildProviderErrorDetails(provider, statusCode, statusText, rawUpstream, rawCrossProviderFallbackEnabled)），在 builder 和内联分支里共用。Also applies to: 1885-1900, 1955-1970
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/forwarder.ts` around lines 1746 - 1761, The provider error detail construction is duplicated across multiple builders and inline branches; add a shared helper (e.g. buildProviderErrorDetails(provider, statusCode, statusText, upstreamError, rawCrossProviderFallbackEnabled)) that returns either the minimal shape or the shape with upstreamBody/upstreamParsed based on rawCrossProviderFallbackEnabled, then replace the repeated ternary constructions that assign errorDetails.provider in the builders and inline branches with calls to this helper (use currentProvider, 404, proxyError.message and proxyError.upstreamError as arguments where those values are used now).
939-977:endpointPolicy在外层和 while 循环内重复解析，且shouldSkipRawRetryAndProviderSwitch未被后续代码复用Line 942 已在函数作用域解析了
endpointPolicy并据此计算shouldSkipRawRetryAndProviderSwitch，但 Line 974 又在 while 循环内用const endpointPolicy = ...重新解析同一条策略（遮蔽外层变量），并在 Line 1914 再次以内联方式重写了!endpointPolicy.allowRetry && !rawCrossProviderFallbackEnabled（与 Line 943-944 定义完全一致）。外层shouldSkipRawRetryAndProviderSwitch在 Line 1649/1766/1834 被复用，但 Line 1914 未使用，容易在后续策略演进时让两处条件出现漂移。建议删除内层
endpointPolicy的重复声明，并让 Line 1914 直接复用shouldSkipRawRetryAndProviderSwitch，保持单一真相源。♻️ 建议的重构
- const endpointPolicy = ProxyForwarder.getEndpointPolicy(session); - const shouldAccountCircuitBreaker = endpointPolicy.allowCircuitBreakerAccounting; + const shouldAccountCircuitBreaker = endpointPolicy.allowCircuitBreakerAccounting; const shouldEnforceStrictEndpointPool = !isMcpRequest && isStrictEndpointPoolPolicy(endpointPolicy) && providerVendorId > 0;并在 Line 1914 处：
- if (!endpointPolicy.allowRetry && !rawCrossProviderFallbackEnabled) { + if (shouldSkipRawRetryAndProviderSwitch) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/forwarder.ts` around lines 939 - 977, The code redeclares endpointPolicy inside the while loop and duplicates the retry-skip logic, causing shadowing and potential drift; remove the inner declaration of endpointPolicy (the call to ProxyForwarder.getEndpointPolicy(session) inside the loop) so the loop reuses the outer endpointPolicy, and replace the inlined condition there with the previously computed shouldSkipRawRetryAndProviderSwitch variable to ensure a single source of truth for retry/provider-switch decisions.tests/unit/proxy/non-chat-endpoint-fallback.test.ts (1)
161-226:createRawSession通过原型克隆构造会话，需留意未来ProxySession若改用私有字段会破坏测试目前通过
Object.create(ProxySession.prototype)+Object.assign来伪造会话实例，仅依赖setRawCrossProviderFallbackEnabled/isRawCrossProviderFallbackEnabled/getEndpointPolicy读取/写入普通属性。如果后续ProxySession重构为#private字段或在构造器内做必要初始化，这个 fixture 会在运行原型方法时抛出“Cannot read private field”。可以在createRawSession顶部加一条 JSDoc 注释提示这一耦合点，或未来考虑提供ProxySession.forTest(...)静态工厂统一测试构造。非阻塞项，当前用法可行。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/proxy/non-chat-endpoint-fallback.test.ts` around lines 161 - 226, The test helper createRawSession constructs a fake ProxySession via Object.create(ProxySession.prototype) and Object.assign, which will break if ProxySession later adopts private (#) fields or requires ctor initialization; to fix, add a JSDoc note at the top of createRawSession calling out this coupling to ProxySession's internal implementation and the reliance on prototype methods setRawCrossProviderFallbackEnabled/isRawCrossProviderFallbackEnabled/getEndpointPolicy, and consider replacing this pattern in future with a dedicated test factory like ProxySession.forTest(...) that performs proper construction and exposes only the needed hooks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@drizzle/0098_equal_selene.sql`:
- Around line 50-99: The INSERT ... ON CONFLICT DO UPDATE for the usage_ledger
trigger is missing the actual_response_model column; update the VALUES list to
include NEW.actual_response_model (or v_... if computed) and add
actual_response_model = EXCLUDED.actual_response_model in the DO UPDATE SET
clause so both insertion and conflict-update paths write the
actual_response_model field for usage_ledger (look for the INSERT INTO
usage_ledger (...) VALUES (...) and the DO UPDATE SET ... blocks referencing NEW
and EXCLUDED).
In `@src/drizzle/schema.ts`:
- Around line 794-801: The migration (drizzle/0098_equal_selene.sql) includes
irreversible destructive deletes targeting usage_ledger (the DELETE FROM
"usage_ledger" ... IN ('/v1/messages/count_tokens','/v1/responses/compact')) and
the same DELETE is present in the trigger function, so update the PR migration
notes to explicitly mark this as a data-destructive change: state that
historical usage_ledger rows for the two endpoints will be deleted, reference
the affected field/flag allowNonConversationEndpointProviderFallback and the
usage_ledger table, require a pre-deployment backup, list expected impact window
and scope (which endpoints and approximate volume), and add a runbook step to
restore or snapshot before applying in production.
In `@src/lib/session-manager.ts`:
- Around line 638-657: getSessionProvider enforces a fail-closed check against
the Redis key "session:{sessionId}:key" but the code paths that create/renew
client-provided sessions and write provider bindings do not persist that key,
causing boundKeyId to be null and breaking provider reuse; update the session
creation/renewal and all provider-binding/TTL-refresh code paths that write
"session:{sessionId}:provider" (the same places that currently call redis.set or
refresh TTL) to also set "session:{sessionId}:key" with the same TTL and value
(use keyId as string) and prefer atomic/transactional writes (MULTI/EXEC or
pipeline) so the provider and key are written/updated together. Ensure
getSessionProvider still reads the same key format and compare stringified
keyId.
In `@tests/integration/non-chat-endpoint-fallback-observability.test.ts`:
- Around line 123-140: The test uses
originalAllowNonConversationEndpointProviderFallback as a sentinel but currently
initializes it to null; change the sentinel to undefined (i.e., let
originalAllowNonConversationEndpointProviderFallback: boolean | undefined =
undefined) and update the afterAll check to restore settings only when
originalAllowNonConversationEndpointProviderFallback !== undefined; ensure the
assignments that capture the original value (in beforeAll via
getSystemSettings()) and the restore call (using updateSystemSettings()) keep
the same symbol names and that cleanupTestRows() calls remain unchanged.
---
Outside diff comments:
In `@src/lib/ledger-backfill/service.ts`:
- Around line 42-107: The backfill SELECT sets is_success using only
mr.error_message but must match the new trigger logic in fn_upsert_usage_ledger;
update the is_success expression in the batch SELECT (the column alias
is_success in the query against message_request mr used to populate
usage_ledger) to require both no error_message and a successy status_code (i.e.,
status_code IS NULL OR status_code < 400) so the backfill and
fn_upsert_usage_ledger produce identical is_success values and avoid split
results between real-time writes and backfill.
---
Duplicate comments:
In `@src/app/v1/_lib/proxy/response-handler.ts`:
- Around line 2277-2291: The stream/SSE path sets context1mApplied too
early—maybeSetCodexContext1m is called before computing billable usage, causing
non-billing endpoints (isNonBillingUsageEndpoint) to be marked as 1M and pollute
later billing; fix by moving the maybeSetCodexContext1m(...) call to after
billable usage is computed (after usageForCost / billableUsageForCost
normalization) and only invoke it when the request is confirmed billable (i.e.,
ensure isNonBillingUsageEndpoint(session) is false and
priorityServiceTierApplied logic remains correct); adjust calls around
normalizeUsageWithSwap and ensureCodexServiceTierResultSpecialSetting to ensure
context1mApplied is only set for true billable paths.
---
Nitpick comments:
In `@src/app/v1/_lib/proxy-handler.ts`:
- Around line 28-33: Change the log level from warn to error in the ProxyHandler
settings load failure so alerts can trigger; in the logger call that currently
references settingsError (inside src/app/v1/_lib/proxy-handler.ts), move the
error into a dedicated structured field (e.g., { error: settingsError } or {
settingsError }) as the second argument so the logger preserves the exception
details rather than embedding it in the message, and keep the existing English
message text about fallback highConcurrency/rawCrossProviderFallback unchanged.
In `@src/app/v1/_lib/proxy/forwarder.ts`:
- Around line 1746-1761: The provider error detail construction is duplicated
across multiple builders and inline branches; add a shared helper (e.g.
buildProviderErrorDetails(provider, statusCode, statusText, upstreamError,
rawCrossProviderFallbackEnabled)) that returns either the minimal shape or the
shape with upstreamBody/upstreamParsed based on rawCrossProviderFallbackEnabled,
then replace the repeated ternary constructions that assign
errorDetails.provider in the builders and inline branches with calls to this
helper (use currentProvider, 404, proxyError.message and
proxyError.upstreamError as arguments where those values are used now).
- Around line 939-977: The code redeclares endpointPolicy inside the while loop
and duplicates the retry-skip logic, causing shadowing and potential drift;
remove the inner declaration of endpointPolicy (the call to
ProxyForwarder.getEndpointPolicy(session) inside the loop) so the loop reuses
the outer endpointPolicy, and replace the inlined condition there with the
previously computed shouldSkipRawRetryAndProviderSwitch variable to ensure a
single source of truth for retry/provider-switch decisions.
In `@src/app/v1/_lib/proxy/session-guard.ts`:
- Around line 78-82: The code redundantly reads back the value just set; replace
the call to session.isRawCrossProviderFallbackEnabled() and the local
allowRawSessionContext with reuse of the computed rawFallbackEnabled: after
computing rawFallbackEnabled and calling
session.setRawCrossProviderFallbackEnabled(rawFallbackEnabled), assign
allowRawSessionContext = rawFallbackEnabled (or remove allowRawSessionContext
and use rawFallbackEnabled directly) so you avoid the extra getter call to
session.isRawCrossProviderFallbackEnabled().
- Around line 78-82: The current code binds the "allow raw cross-provider
fallback" decision (rawFallbackEnabled) to session context mutation by deriving
allowRawSessionContext from session.isRawCrossProviderFallbackEnabled(), which
conflates fallback decision with whether request bodies may be mutated; either
add a clear comment above this block explaining why these two behaviors are
intentionally coupled (reference the count_tokens/compact endpoints'
raw-pass-through contract) or rename allowRawSessionContext to a
behavior-focused name such as skipSessionContextMutation and update all uses
(session.setRawCrossProviderFallbackEnabled,
session.isRawCrossProviderFallbackEnabled, and any logic that reads
allowRawSessionContext) so it explicitly controls request-body mutation while
leaving the fallback decision variable (rawFallbackEnabled) separate; apply the
same change/comment at the other similar locations that mirror this pattern (the
other occurrences around the same blocks).
In `@tests/unit/actions/usage-logs-hidden-non-chat-endpoints.test.ts`:
- Around line 51-64: The test uses brittle string slicing via indexOf("export
async function getUsedEndpoints") and indexOf("export async function
getDistinctEndpointsForKey") which can produce empty or wrong segments if
functions are renamed/reordered; update the test to either (A) assert that
getUsedEndpointsSegment and getDistinctEndpointsSegment are non-empty before
asserting not.toContain("buildDefaultHiddenUsageLogEndpointCondition") or (B)
replace the slicing with a regex that reliably captures the full function bodies
(e.g. match /export async function getUsedEndpoints[\s\S]*?{[\s\S]*?^}/m or
similar) so you directly search inside the captured bodies for
buildDefaultHiddenUsageLogEndpointCondition; reference getUsedEndpointsSegment,
getDistinctEndpointsSegment, getUsedEndpoints, getDistinctEndpointsForKey, and
buildDefaultHiddenUsageLogEndpointCondition when making the change.
In `@tests/unit/proxy/non-chat-endpoint-fallback.test.ts`:
- Around line 161-226: The test helper createRawSession constructs a fake
ProxySession via Object.create(ProxySession.prototype) and Object.assign, which
will break if ProxySession later adopts private (#) fields or requires ctor
initialization; to fix, add a JSDoc note at the top of createRawSession calling
out this coupling to ProxySession's internal implementation and the reliance on
prototype methods
setRawCrossProviderFallbackEnabled/isRawCrossProviderFallbackEnabled/getEndpointPolicy,
and consider replacing this pattern in future with a dedicated test factory like
ProxySession.forTest(...) that performs proper construction and exposes only the
needed hooks.
In `@tests/unit/proxy/response-handler-lease-decrement.test.ts`:
- Around line 421-447: Add a symmetric test case covering the
/v1/messages/count_tokens endpoint variants by converting the existing test into
a parameterized it.each that iterates over the non-billing paths (including
variants with/without trailing slash and different casing) and runs the same
assertions: call ProxyResponseHandler.dispatch with the session.pathname set to
each path, ensure RateLimitService.trackCost, trackUserDailyCost, and
decrementLeaseBudget are not called, and verify updateMessageRequestDetails is
called with the expected token fields; reference ProxyResponseHandler.dispatch,
RateLimitService.trackCost/trackUserDailyCost/decrementLeaseBudget, and
updateMessageRequestDetails to locate where to add the parameterized test.
🪄 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: 18a37025-2436-4d0b-a393-9733130c993f
📒 Files selected for processing (59)
drizzle/0098_equal_selene.sqldrizzle/meta/0098_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/my-usage.tssrc/actions/system-config.tssrc/app/[locale]/dashboard/logs/_components/usage-logs-table.tsxsrc/app/[locale]/dashboard/logs/_components/virtualized-logs-table.tsxsrc/app/[locale]/settings/config/_components/system-settings-form.tsxsrc/app/[locale]/settings/config/page.tsxsrc/app/v1/_lib/proxy-handler.tssrc/app/v1/_lib/proxy/endpoint-policy.tssrc/app/v1/_lib/proxy/forwarder.tssrc/app/v1/_lib/proxy/guard-pipeline.tssrc/app/v1/_lib/proxy/provider-selector.tssrc/app/v1/_lib/proxy/response-handler.tssrc/app/v1/_lib/proxy/session-guard.tssrc/app/v1/_lib/proxy/session.tssrc/drizzle/schema.tssrc/lib/config/system-settings-cache.tssrc/lib/ledger-backfill/service.tssrc/lib/ledger-backfill/trigger.sqlsrc/lib/session-manager.tssrc/lib/utils/performance-formatter.tssrc/lib/validation/schemas.tssrc/repository/_shared/ledger-conditions.tssrc/repository/_shared/transformers.tssrc/repository/_shared/usage-log-filters.tssrc/repository/system-config.tssrc/repository/usage-logs.tssrc/types/message.tssrc/types/system-config.tstests/configs/integration.config.tstests/integration/non-chat-endpoint-fallback-observability.test.tstests/unit/actions/my-usage-readonly-provider-chain.test.tstests/unit/actions/system-config-non-chat-retry-setting.test.tstests/unit/actions/usage-logs-hidden-non-chat-endpoints.test.tstests/unit/proxy/endpoint-policy-parity.test.tstests/unit/proxy/endpoint-policy.test.tstests/unit/proxy/guard-pipeline-warmup.test.tstests/unit/proxy/hedge-error-pipeline.test.tstests/unit/proxy/non-chat-endpoint-fallback.test.tstests/unit/proxy/non-chat-endpoint-non-billing.test.tstests/unit/proxy/non-chat-endpoint-policy.test.tstests/unit/proxy/non-chat-endpoint-session-context.test.tstests/unit/proxy/provider-selector-model-mismatch-binding.test.tstests/unit/proxy/proxy-forwarder-endpoint-audit.test.tstests/unit/proxy/proxy-forwarder-hedge-first-byte.test.tstests/unit/proxy/proxy-forwarder-retry-limit.test.tstests/unit/proxy/proxy-handler-session-id-error.test.tstests/unit/proxy/response-handler-lease-decrement.test.tstests/unit/proxy/session-guard-warmup-intercept.test.tstests/unit/repository/key.test.tstests/unit/repository/usage-logs-min-retry-count-filter.test.tstests/unit/settings/system-settings-form-non-chat-fallback.test.tsx
✅ Files skipped from review due to trivial changes (10)
- src/app/[locale]/settings/config/page.tsx
- src/types/message.ts
- tests/unit/proxy/endpoint-policy.test.ts
- messages/zh-CN/settings/config.json
- tests/unit/proxy/session-guard-warmup-intercept.test.ts
- src/app/[locale]/dashboard/logs/_components/usage-logs-table.tsx
- tests/configs/integration.config.ts
- tests/unit/proxy/non-chat-endpoint-policy.test.ts
- messages/zh-TW/settings/config.json
- src/repository/usage-logs.ts
🚧 Files skipped from review as they are similar to previous changes (22)
- src/repository/_shared/transformers.ts
- src/lib/validation/schemas.ts
- messages/ja/settings/config.json
- tests/unit/repository/key.test.ts
- messages/en/settings/config.json
- tests/unit/proxy/endpoint-policy-parity.test.ts
- tests/unit/repository/usage-logs-min-retry-count-filter.test.ts
- src/lib/ledger-backfill/trigger.sql
- src/app/[locale]/settings/config/_components/system-settings-form.tsx
- src/lib/config/system-settings-cache.ts
- tests/unit/proxy/provider-selector-model-mismatch-binding.test.ts
- src/lib/utils/performance-formatter.ts
- src/actions/my-usage.ts
- tests/unit/actions/system-config-non-chat-retry-setting.test.ts
- tests/unit/proxy/proxy-forwarder-hedge-first-byte.test.ts
- drizzle/meta/_journal.json
- tests/unit/proxy/guard-pipeline-warmup.test.ts
- tests/unit/proxy/non-chat-endpoint-non-billing.test.ts
- src/repository/system-config.ts
- tests/unit/proxy/proxy-forwarder-retry-limit.test.ts
- messages/ru/settings/config.json
- tests/unit/proxy/proxy-forwarder-endpoint-audit.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/my-usage-imported-ledger.test.ts`:
- Around line 61-79: The test can still flake around server-local midnight
because getStableRecentUtcTimestamp() only zeroes seconds/millis and
insertLedgerOnlyRow writes a UTC createdAt that may fall on a different
server-local date than the "today" used by getServerDateString() and
getMyStatsSummary(); change the test to avoid the midnight edge by either (A)
when building the test timestamp (now) subtract 5–10 minutes from
getStableRecentUtcTimestamp() so createdAt is safely away from midnight, or (B)
derive the server-local date via getServerDateString(...) and then set createdAt
to a deterministic mid-day UTC instant for that date (e.g., 12:00 server time
converted to UTC) before calling insertLedgerOnlyRow, ensuring
getMyStatsSummary({ startDate: today, endDate: today }) consistently includes
the inserted row.
🪄 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: b9938edc-7e4f-4fbf-9191-f96fed7b4ae3
📒 Files selected for processing (2)
drizzle/meta/_journal.jsontests/integration/my-usage-imported-ledger.test.ts
✅ Files skipped from review due to trivial changes (1)
- drizzle/meta/_journal.json
Contributor
🧪 测试结果
总体结果: ✅ 所有测试通过 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b38180d42b
ℹ️ 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".
Contributor
🧪 测试结果
总体结果: ✅ 所有测试通过 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfed0dfe72
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/app/v1/_lib/proxy/response-handler.ts (1)
2284-2293:⚠️ Potential issue | 🟡 Minor这个流式分支里仍会把非计费请求标成
context1mApplied。这里在做
billableUsageForCost过滤之前就调用了maybeSetCodexContext1m。如果/v1/messages/count_tokens或/v1/responses/compact以后走到这条收尾路径，session会被污染成 1M context，后续同 session 的真实计费请求就可能沿用错误状态。建议修复
- maybeSetCodexContext1m(session, provider, usageForCost?.input_tokens); - // Codex: Extract prompt_cache_key from SSE events and update session binding if (provider.providerType === "codex" && session.sessionId && provider.id) { try { @@ const billableUsageForCost = usageForCost && !isNonBillingUsageEndpoint(session) ? usageForCost : null; + + if (billableUsageForCost) { + maybeSetCodexContext1m(session, provider, billableUsageForCost.input_tokens); + }这和前面已经补过的非计费保护是同一类问题，只是这里还漏了一个流式分支。
Also applies to: 2321-2322
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/response-handler.ts` around lines 2284 - 2293, The streaming branch currently calls maybeSetCodexContext1m(session, provider, usageForCost?.input_tokens) before the billable filtering, causing non-billable requests (e.g., /v1/messages/count_tokens or /v1/responses/compact) to mark session.context1mApplied; move or guard that call so it only runs for actual billable usage: ensure you apply normalizeUsageWithSwap and then check the billable usage filter (the same logic used for billableUsageForCost) and only after that invoke maybeSetCodexContext1m with the verified billable usage (references: usageForCost, normalizeUsageWithSwap, maybeSetCodexContext1m, provider.swapCacheTtlBilling); replicate the same fix for the other streaming path noted around the second occurrence (lines ~2321-2322).
🧹 Nitpick comments (5)
src/repository/usage-logs.ts (1)
1559-1618: 非计费端点快速路径实现符合预期；5m/1h token 不再重复累加。
totalCost硬编码为0、totalTokens仅累加input+output+cacheCreation+cacheRead（第 1601-1605 行），与其他分支口径保持一致，并附了说明注释。ledger-only 模式下继续走 ledger 分支（由 migration 清理），符合设计意图。可选：此块约 60 行，与后续 ledger 分支在 userId/keyId/providerId 条件构建上存在模式重复，后续若再增加端点类型，可考虑抽出辅助函数（如
buildMessageRequestStatsQuery）统一收拢；本 PR 维持现状也可以。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/repository/usage-logs.ts` around lines 1559 - 1618, Non-billing quick path in the if (!ledgerOnly && isNonBillingEndpoint(filters.endpoint)) block works as intended (totalCost hardcoded to "0" and totalTokens computed as input+output+cacheCreation+cacheRead) but there is duplicated condition-building logic with the ledger branch; either leave as-is (approved) or refactor by extracting the shared condition/query construction into a helper (e.g., buildMessageRequestStatsQuery) that accepts filters/userId/keyId/providerId and returns the baseQuery (using messageRequest, keysTable, buildUsageLogConditions) so both branches reuse the same query-building code.src/app/v1/_lib/proxy/forwarder.ts (3)
941-979:endpointPolicy在外层与 while 内层被重复声明，形成无必要的遮蔽第 942 行已在外层声明
endpointPolicy并据此计算shouldSkipRawRetryAndProviderSwitch；第 974 行又在while循环内用const endpointPolicy再次调用ProxyForwarder.getEndpointPolicy(session)并在后续分支（shouldAccountCircuitBreaker、shouldEnforceStrictEndpointPool、1659/1776/1844/1925、以及bypassForwarderPreprocessing等）复用。由于 endpoint policy 在单次
send()内不会随 provider 切换而变化（它来源于session.getEndpointPolicy()或requestUrl.pathname），每次循环重新解析不仅是重复调用，也让endpointPolicy在方法内出现两个同名变量，阅读时需要来回确认指向的是哪一个。建议直接复用外层变量，删除循环内的重新声明：
♻️ 建议 diff
const env = getEnvConfig(); const envDefaultMaxAttempts = clampRetryAttempts(env.MAX_RETRY_ATTEMPTS_DEFAULT); const rawCrossProviderFallbackEnabled = session.isRawCrossProviderFallbackEnabled(); const endpointPolicy = ProxyForwarder.getEndpointPolicy(session); const shouldSkipRawRetryAndProviderSwitch = !endpointPolicy.allowRetry && !rawCrossProviderFallbackEnabled; @@ - const endpointPolicy = ProxyForwarder.getEndpointPolicy(session); const shouldAccountCircuitBreaker = endpointPolicy.allowCircuitBreakerAccounting;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/forwarder.ts` around lines 941 - 979, The code declares endpointPolicy twice (once before the while loop and again inside it), causing shadowing and redundant calls to ProxyForwarder.getEndpointPolicy(session); remove the inner declaration (the const endpointPolicy inside the while loop) and update uses of shouldAccountCircuitBreaker and shouldEnforceStrictEndpointPool (and any downstream uses like bypassForwarderPreprocessing) to reference the outer endpointPolicy so the loop reuses the single endpointPolicy value and avoids duplicate computations and shadowing.
1917-1930: 此处条件与shouldSkipRawRetryAndProviderSwitch完全等价，建议直接复用
!endpointPolicy.allowRetry && !rawCrossProviderFallbackEnabled与第 943–944 行定义的shouldSkipRawRetryAndProviderSwitch是同一个表达式；SYSTEM_ERROR（1652）、RESOURCE_NOT_FOUND（1769）、空响应（1837）三处已统一使用变量，这里是唯一残留的字面重复，容易在后续修改语义时漏改其中一处，导致 raw 端点的透传语义在 PROVIDER_ERROR 分支与其他分支不一致。♻️ 建议 diff
- // Raw passthrough endpoints: no circuit breaker, no provider switch, no retry - if (!endpointPolicy.allowRetry && !rawCrossProviderFallbackEnabled) { + // Raw passthrough endpoints: no circuit breaker, no provider switch, no retry + if (shouldSkipRawRetryAndProviderSwitch) { logger.debug(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/forwarder.ts` around lines 1917 - 1930, The condition near the throw in ProxyForwarder (the block that logs "ProxyForwarder: raw passthrough endpoint error, skipping circuit breaker and provider switch" and then throws lastError) duplicates the expression !endpointPolicy.allowRetry && !rawCrossProviderFallbackEnabled; replace that literal with the existing shouldSkipRawRetryAndProviderSwitch variable (defined earlier) so the PROVIDER_ERROR branch uses the same predicate as the SYSTEM_ERROR/RESOURCE_NOT_FOUND/empty-response branches and avoid divergent behavior if the logic changes later.
1745-1767:provider错误详情的条件性脱敏逻辑在多处重复当前
rawCrossProviderFallbackEnabled ? {...精简字段} : {...含 upstreamBody/upstreamParsed}的三元写法出现在 5 个位置：buildClientErrorChainEntry（526–540）、buildRetryFailedChainEntry（584–598）、以及send()内的resource_not_found（1750–1764）、vendor_type_all_timeout（1889–1903）、retry_failed（1959–1973）。结构完全一致，仅statusCode/statusText字段取值不同。一旦后续需要调整脱敏的字段集合（例如再去掉一个字段，或加入
rawBody相关控制），需要同时修改 5 处，容易漏改，也正是本 PR 注明的“clientError scrubbing regression”这类回归最容易出现的土壤。建议抽一个辅助函数把
{provider, statusCode, statusText, upstreamBody?, upstreamParsed?}的拼装收敛在一处：function buildProviderErrorDetail( provider: Provider, statusCode: number, statusText: string, upstreamError: ProxyError["upstreamError"], rawCrossProviderFallbackEnabled: boolean, ) { const base = { id: provider.id, name: provider.name, statusCode, statusText }; return rawCrossProviderFallbackEnabled ? base : { ...base, upstreamBody: upstreamError?.body, upstreamParsed: upstreamError?.parsed, }; }然后在 5 个调用点替换相应字面对象。这也有助于后续若要把
clientError/safeClientMessageCandidate等字段纳入同一脱敏策略时集中处理。Also applies to: 1885-1906, 1952-1976
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/forwarder.ts` around lines 1745 - 1767, Extract the repeated ternary that builds the provider error object into a single helper function (e.g., buildProviderErrorDetail) that accepts (provider, statusCode, statusText, upstreamError, rawCrossProviderFallbackEnabled) and returns the base {id, name, statusCode, statusText} when rawCrossProviderFallbackEnabled is true or the base plus upstreamBody/upstreamParsed when false; then replace the inline ternaries in buildClientErrorChainEntry, buildRetryFailedChainEntry and the three send() cases (resource_not_found, vendor_type_all_timeout, retry_failed) to call this helper so the scrubbing logic is centralized and consistent across those five call sites.src/app/v1/_lib/proxy/endpoint-policy.ts (1)
65-73: 两个新谓词缺少显式返回类型注解，且shouldEnforceStrictEndpointPoolPolicy当前逻辑恒为真
缺少返回类型：
isStrictEndpointPoolPolicy和shouldEnforceStrictEndpointPoolPolicy均未声明显式返回类型，与同文件其他导出的isRawPassthroughEndpointPath和isRawPassthroughEndpointPolicy不一致（都显式标注了: boolean）。建议统一补上: boolean。逻辑问题：
EndpointPoolStrictness当前只有"inherit" | "strict"两个取值，而shouldEnforceStrictEndpointPoolPolicy对两者均返回true，因此对任何合法的EndpointPolicy都恒为真。虽然该函数在forwarder.ts的实际使用中是与其他条件（!isMcpRequest && ... && providerVendorId > 0）组合的，但单看函数定义时容易产生误解。建议二选一：
- 若这是为未来新增取值（例如
"disabled"/"off"）预留的扩展点，请在函数上方加注释说明，避免后续开发者误认为冗余；- 若当前没有此计划，可考虑删除该层调用或与
isStrictEndpointPoolPolicy合并为语义清晰的单一谓词。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/endpoint-policy.ts` around lines 65 - 73, Add explicit boolean return annotations to isStrictEndpointPoolPolicy and shouldEnforceStrictEndpointPoolPolicy (declare both as ": boolean"), and fix the confusing logic: either change shouldEnforceStrictEndpointPoolPolicy to only return true for strict (i.e., return policy.endpointPoolStrictness === "strict") or, if "inherit" is intentionally treated as enforcing for future values, add a clear comment above shouldEnforceStrictEndpointPoolPolicy explaining why inherit is considered enforcing and that this is an extension point for future EndpointPoolStrictness values; update the functions isStrictEndpointPoolPolicy and shouldEnforceStrictEndpointPoolPolicy accordingly.
🤖 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/lib/session-manager.ts`:
- Around line 578-591: In refreshSessionTTL, don't overwrite the session:key
binding during TTL refresh — remove the
pipeline.setex(`session:${sessionId}:key`, ...) path that writes keyId and
instead only refresh TTL (use pipeline.expire for `session:${sessionId}:key`) so
you don't change ownership; if backfilling missing `session:${sessionId}:key` is
required, do it only in the verified binding or atomic create/update paths (not
in SessionManager.refreshSessionTTL) to avoid bypassing getSessionProvider's
fail-closed checks.
In `@src/repository/usage-logs.ts`:
- Around line 323-333: The endpoint matching logic is inconsistent: some
branches use strict eq(usageLedger.endpoint, filters.endpoint) while
findUsageLogsStats uses buildUsageLogEndpointMatchCondition (which lowercases
and normalizes trailing slashes); this causes mismatched results between list
and stats. Replace the eq(...) comparisons in the ledger fallback within
findUsageLogsBatch and in buildKeyLedgerConditions with
buildUsageLogEndpointMatchCondition(usageLedger.endpoint, filters.endpoint) and
keep using buildDefaultHiddenUsageLogEndpointCondition as-is (it returns null
when filters.endpoint is present), so all three places (findUsageLogsStats,
findUsageLogsBatch ledger branch, and buildKeyLedgerConditions) use
buildUsageLogEndpointMatchCondition for consistent normalization/matching.
---
Duplicate comments:
In `@src/app/v1/_lib/proxy/response-handler.ts`:
- Around line 2284-2293: The streaming branch currently calls
maybeSetCodexContext1m(session, provider, usageForCost?.input_tokens) before the
billable filtering, causing non-billable requests (e.g.,
/v1/messages/count_tokens or /v1/responses/compact) to mark
session.context1mApplied; move or guard that call so it only runs for actual
billable usage: ensure you apply normalizeUsageWithSwap and then check the
billable usage filter (the same logic used for billableUsageForCost) and only
after that invoke maybeSetCodexContext1m with the verified billable usage
(references: usageForCost, normalizeUsageWithSwap, maybeSetCodexContext1m,
provider.swapCacheTtlBilling); replicate the same fix for the other streaming
path noted around the second occurrence (lines ~2321-2322).
---
Nitpick comments:
In `@src/app/v1/_lib/proxy/endpoint-policy.ts`:
- Around line 65-73: Add explicit boolean return annotations to
isStrictEndpointPoolPolicy and shouldEnforceStrictEndpointPoolPolicy (declare
both as ": boolean"), and fix the confusing logic: either change
shouldEnforceStrictEndpointPoolPolicy to only return true for strict (i.e.,
return policy.endpointPoolStrictness === "strict") or, if "inherit" is
intentionally treated as enforcing for future values, add a clear comment above
shouldEnforceStrictEndpointPoolPolicy explaining why inherit is considered
enforcing and that this is an extension point for future EndpointPoolStrictness
values; update the functions isStrictEndpointPoolPolicy and
shouldEnforceStrictEndpointPoolPolicy accordingly.
In `@src/app/v1/_lib/proxy/forwarder.ts`:
- Around line 941-979: The code declares endpointPolicy twice (once before the
while loop and again inside it), causing shadowing and redundant calls to
ProxyForwarder.getEndpointPolicy(session); remove the inner declaration (the
const endpointPolicy inside the while loop) and update uses of
shouldAccountCircuitBreaker and shouldEnforceStrictEndpointPool (and any
downstream uses like bypassForwarderPreprocessing) to reference the outer
endpointPolicy so the loop reuses the single endpointPolicy value and avoids
duplicate computations and shadowing.
- Around line 1917-1930: The condition near the throw in ProxyForwarder (the
block that logs "ProxyForwarder: raw passthrough endpoint error, skipping
circuit breaker and provider switch" and then throws lastError) duplicates the
expression !endpointPolicy.allowRetry && !rawCrossProviderFallbackEnabled;
replace that literal with the existing shouldSkipRawRetryAndProviderSwitch
variable (defined earlier) so the PROVIDER_ERROR branch uses the same predicate
as the SYSTEM_ERROR/RESOURCE_NOT_FOUND/empty-response branches and avoid
divergent behavior if the logic changes later.
- Around line 1745-1767: Extract the repeated ternary that builds the provider
error object into a single helper function (e.g., buildProviderErrorDetail) that
accepts (provider, statusCode, statusText, upstreamError,
rawCrossProviderFallbackEnabled) and returns the base {id, name, statusCode,
statusText} when rawCrossProviderFallbackEnabled is true or the base plus
upstreamBody/upstreamParsed when false; then replace the inline ternaries in
buildClientErrorChainEntry, buildRetryFailedChainEntry and the three send()
cases (resource_not_found, vendor_type_all_timeout, retry_failed) to call this
helper so the scrubbing logic is centralized and consistent across those five
call sites.
In `@src/repository/usage-logs.ts`:
- Around line 1559-1618: Non-billing quick path in the if (!ledgerOnly &&
isNonBillingEndpoint(filters.endpoint)) block works as intended (totalCost
hardcoded to "0" and totalTokens computed as
input+output+cacheCreation+cacheRead) but there is duplicated condition-building
logic with the ledger branch; either leave as-is (approved) or refactor by
extracting the shared condition/query construction into a helper (e.g.,
buildMessageRequestStatsQuery) that accepts filters/userId/keyId/providerId and
returns the baseQuery (using messageRequest, keysTable, buildUsageLogConditions)
so both branches reuse the same query-building code.
🪄 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: f4aada0b-ece9-482d-8759-3f02dd45cb27
📒 Files selected for processing (11)
drizzle/0098_equal_selene.sqlsrc/app/v1/_lib/proxy/endpoint-policy.tssrc/app/v1/_lib/proxy/forwarder.tssrc/app/v1/_lib/proxy/response-handler.tssrc/lib/session-manager.tssrc/repository/usage-logs.tstests/integration/my-usage-imported-ledger.test.tstests/integration/non-chat-endpoint-fallback-observability.test.tstests/unit/proxy/endpoint-policy.test.tstests/unit/proxy/non-chat-endpoint-fallback.test.tstests/unit/proxy/proxy-forwarder-hedge-first-byte.test.ts
✅ Files skipped from review due to trivial changes (2)
- tests/unit/proxy/proxy-forwarder-hedge-first-byte.test.ts
- tests/integration/non-chat-endpoint-fallback-observability.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/unit/proxy/endpoint-policy.test.ts
- drizzle/0098_equal_selene.sql
- tests/unit/proxy/non-chat-endpoint-fallback.test.ts
Contributor
🧪 测试结果
总体结果: ✅ 所有测试通过 |
Contributor
🧪 测试结果
总体结果: ✅ 所有测试通过 |
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
This was referenced Apr 28, 2026
ding113
added a commit
that referenced
this pull request
May 17, 2026
…-only scans (#1194) * fix(db): cover endpoint in usage_ledger cost indexes to restore index-only scans #1091 added a non-billing-endpoint filter to LEDGER_BILLING_CONDITION, so every SUM(cost_usd) query now also references usage_ledger.endpoint. That column was not part of the three cost covering indexes (idx_usage_ledger_user_cost_cover, idx_usage_ledger_provider_cost_cover, idx_usage_ledger_key_cost), so the rate-limit and Quotas-page hot-path queries lost their Index Only Scan and degraded to a Bitmap Heap Scan with one heap fetch per matching row. Reproduced on Postgres 18 (1,000,000 rows, 200 users). The per-user SUM(cost_usd) query (sumUserTotalCost): - pre-#1091 condition: Index Only Scan, 46 shared buffers, Heap Fetches: 0, ~1ms - post-#1091 condition: Bitmap Heap Scan, 5027 shared buffers, 5000 heap blocks, ~15ms Add endpoint as a trailing column to the three covering indexes so the endpoint filter can be evaluated from the index. After applying the migration the post-#1091 query is back to Index Only Scan / Heap Fetches: 0 / ~40 buffers. Drizzle has no INCLUDE support, so endpoint is added as a trailing key column, matching the existing convention on idx_usage_ledger_key_created_at_desc_cover. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(db): make the 0103 cost-index migration idempotent and operator-safe Addresses PR review feedback on the index rebuild. A plain CREATE INDEX on usage_ledger holds a SHARE lock that blocks writes for the whole rebuild, and Drizzle's migrator is transactional so CREATE INDEX CONCURRENTLY cannot be inlined. Wrap each rebuild in a guarded DO block that skips when the index definition already contains `endpoint`. Operators on a large / busy database can now rebuild the three indexes ahead of time with CREATE INDEX CONCURRENTLY and the migration becomes a no-op -- the escape hatch documented on migrations 0082 / 0088, extended to the drop+recreate case. Verified on Postgres 18: 3-column to 4-column on the first apply, clean no-op on a second apply. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: ding113 <ding113@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add
allowNonConversationEndpointProviderFallbacksystem setting with settings UI and 5-language i18n. When enabled,/v1/messages/count_tokensand/v1/responses/compactreuse the session/provider decision chain to perform compatible cross-provider fallback after the current provider fails -- while preserving raw passthrough, strict endpoint-pool, and non-billing semantics. The two target endpoints remain hidden by default in usage queries.Problem
Non-conversation endpoints (
/v1/messages/count_tokens,/v1/responses/compact) used aRAW_PASSTHROUGH_PIPELINEthat skipped session and message-context guards entirely, preventing any retry or provider fallback. If the initial provider was unreachable or errored, the request failed immediately with no recovery path.Related Issues:
/v1/responses/compactendpoint now supports cross-provider fallback when a provider fails, instead of failing outright with no retrySolution
EndpointPolicycapability flag (allowRawCrossProviderFallback) that only the raw passthrough policy enables -- the default chat policy remainsfalserawCrossProviderFallbackEnabledonProxySession, driven by the newallowNonConversationEndpointProviderFallbacksystem settingRAW_SAFE_SESSION_PIPELINEguard pipeline (auth -> client -> model -> version -> probe -> session -> provider -> messageContext) that runs session/provider resolution for these endpoints when fallback is enabled, falling back to the originalRAW_PASSTHROUGH_PIPELINEwhen disabledNON_BILLING_ENDPOINTSarray covering both target paths. The ledger trigger (fn_upsert_usage_ledger) deletes ledger rows for these endpoints; the response handler skips cost calculation and Redis cost trackingclearSessionProvider) when the session-bound provider mismatches the request formatDEFAULT_HIDDEN_USAGE_LOG_ENDPOINTSChanges
Core Changes
endpoint-policy.ts- AddallowRawCrossProviderFallbackflag toEndpointPolicyinterface; enabled only forraw_passthroughkindguard-pipeline.ts- NewRAW_SAFE_SESSION_PIPELINEconfig;fromEndpointPolicy/fromSessionroute to it when raw cross-provider fallback is activesession.ts-rawCrossProviderFallbackEnabledruntime flag withisRawCrossProviderFallbackEnabled()gating;shouldReuseProvider()now considers this flagsession-guard.ts- Propagate system setting to session; skip codex completion and session mutation when raw session context is activeforwarder.ts- Implement retry / provider-switch logic for raw fallback path; scrub readonly fields; align chain entry buildersproxy-handler.ts- ReadallowNonConversationEndpointProviderFallbackfrom system settings and inject into sessionresponse-handler.ts- Skip billing (cost calc, Redis cost tracking, service tier special settings) for non-billing endpointsprovider-selector.ts- Add format-compatibility check for session-bound providers; clear stale bindings; passkeyIdtogetSessionProviderDatabase / Schema
schema.ts- NewallowNonConversationEndpointProviderFallbackboolean column onsystem_settings(defaulttrue)0098_equal_selene.sql- Migration: add column, purge existing ledger rows for target endpoints, and rebuildfn_upsert_usage_ledgerwhile preservingactual_response_modelpropagation and non-billing endpoint exclusion logicUsage / Billing
performance-formatter.ts-NON_BILLING_ENDPOINT->NON_BILLING_ENDPOINTSarray +isNonBillingEndpoint()function with trailing-slash normalizationledger-conditions.ts- Replace single-endpoint filter withNON_BILLING_LEDGER_ENDPOINT_CONDITIONderived fromNON_BILLING_ENDPOINTSusage-log-filters.ts-DEFAULT_HIDDEN_USAGE_LOG_ENDPOINTS; normalized endpoint filteringusage-logs.ts/my-usage.ts- Apply hidden-endpoint filters in queries; scrub readonly chain dataSettings UI & i18n
system-settings-form.tsx/page.tsx- New toggle for the settingmessages/{en,ja,ru,zh-CN,zh-TW}/settings/config.json- i18n strings for all 5 languagesSupporting Changes
session-manager.ts-getSessionProvideraccepts optionalkeyId; newclearSessionProviderusagesystem-settings-cache.ts/repository/system-config.ts- Surface new settingvalidation/schemas.ts/types/system-config.ts- Type definitions for the new settingledger-backfill/service.ts/trigger.sql- Updated backfill logic for new ledger functionTests (8 new test files, 12 updated)
non-chat-endpoint-policy.test.ts- Policy flag assertionsnon-chat-endpoint-session-context.test.ts- Session/guard integration for raw fallbacknon-chat-endpoint-fallback.test.ts- Forwarder retry/provider-switch behaviornon-chat-endpoint-non-billing.test.ts- Non-billing enforcementusage-logs-hidden-non-chat-endpoints.test.ts- Hidden endpoint filteringsystem-config-non-chat-retry-setting.test.ts- Setting persistencesystem-settings-form-non-chat-fallback.test.tsx- UI toggle renderingnon-chat-endpoint-fallback-observability.test.ts- Integration (observability)endpoint-policy.test.ts,endpoint-policy-parity.test.ts,guard-pipeline-warmup.test.ts,proxy-forwarder-endpoint-audit.test.ts,proxy-forwarder-retry-limit.test.ts,proxy-forwarder-hedge-first-byte.test.ts,response-handler-lease-decrement.test.ts,session-guard-warmup-intercept.test.ts,provider-selector-model-mismatch-binding.test.ts,key.test.ts,usage-logs-min-retry-count-filter.test.ts,integration.config.tsMigration Safety
drizzle/0098_equal_selene.sqlcontains an irreversibleDELETE FROM "usage_ledger"for historical/v1/messages/count_tokensand/v1/responses/compactrows.usage_ledgerif those historical rows are needed for audit or rollback analysis.SELECT count(*) FROM usage_ledger WHERE endpoint IS NOT NULL AND LOWER(REGEXP_REPLACE(endpoint, '/+$', '')) IN ('/v1/messages/count_tokens', '/v1/responses/compact');to estimate impact.Breaking Changes
NON_BILLING_ENDPOINTconstant still exported but deprecated in favor ofNON_BILLING_ENDPOINTSarray andisNonBillingEndpoint()isNonBillingEndpoint(endpoint)LEDGER_BILLING_CONDITIONreplaced withNON_BILLING_LEDGER_ENDPOINT_CONDITIONCOUNT_TOKENS_PIPELINEnow points toRAW_SAFE_SESSION_PIPELINEusage_ledgerbefore production rollout if those rows are needed for auditVerification
Screenshot
!settings config screenshot
Checklist
db:generateDescription enhanced by Claude AI
Greptile Summary
This PR introduces
allowNonConversationEndpointProviderFallback— a system setting that enables/v1/messages/count_tokensand/v1/responses/compactto reuse the session/provider resolution chain for cross-provider fallback instead of failing immediately. It ships a newRAW_SAFE_SESSION_PIPELINE, non-billing billing guards in the response handler,keyId-scoped session provider validation in the session manager, normalized endpoint filtering across all usage-log queries, a Drizzle migration that permanently deletes historical ledger rows for those two endpoints, and i18n + settings UI for the new toggle.Confidence Score: 4/5
Safe to merge after addressing the redundant endpointPolicy shadow and reviewing non-atomic key binding; no P0 issues found.
All remaining findings are P2. The duplicate
endpointPolicyin the forwarder loop body produces no behavioral difference since both declarations call the same pure method on the same session, but it is confusing. The non-atomic provider+key binding in session-manager is mitigated by fail-closed behavior ingetSessionProvider, limiting blast radius to a stale Redis entry for one TTL window. The migration's irreversible DELETE is clearly documented with a preflight query. No P0/P1 issues remain beyond prior-thread items.src/lib/session-manager.ts (first-bind branches lack pipeline atomicity); src/app/v1/_lib/proxy/forwarder.ts (duplicate endpointPolicy const).
Important Files Changed
shouldSkipRawRetryAndProviderSwitchand per-provider single-attempt cap; has a redundant innerendpointPolicydeclaration that shadows the outer one.keyIdcross-validation togetSessionProvider(fail-closed) and propagateskeyIdthrough all binding paths; provider + key writes are non-atomic in theSET NXfirst-bind branches.rawCrossProviderFallbackEnabledruntime flag with getter/setter;shouldReuseProvidernow returns true for raw fallback sessions; clean implementation.billableUsageMetrics/billableUsageForCostguards to skip cost calculation, Redis tracking, Codex 1M context, and service-tier special settings for non-billing endpoints; looks correct.RAW_SAFE_SESSION_PIPELINEroutes raw endpoints through session/provider/messageContext steps when fallback is enabled;fromRequestType(COUNT_TOKENS)always uses this pipeline regardless of the runtime flag (only called from tests).allow_non_conversation_endpoint_provider_fallbackcolumn; deletes historical non-billing ledger rows; rebuildsfn_upsert_usage_ledgerwith non-billing endpoint exclusion andactual_response_modelpropagation preserved.findUsageLogsBatch,buildKeyLedgerConditions, andfindUsageLogsStats; adds a non-ledger message-request path for non-billing endpoint stats.clearSessionProvideron stale binding; passeskeyIdtogetSessionProviderfor ownership validation.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Incoming Request /v1/messages/count_tokens /v1/responses/compact] --> B{resolveEndpointPolicy} B --> C[RAW_PASSTHROUGH_ENDPOINT_POLICY allowRawCrossProviderFallback=true] C --> D{proxy-handler: allowNonConversationEndpointProviderFallback system setting} D -- enabled --> E[session.setRawCrossProviderFallbackEnabled=true] D -- disabled --> F[session.setRawCrossProviderFallbackEnabled=false] E --> G[fromSession → RAW_SAFE_SESSION_PIPELINE auth→client→model→version→probe →session→provider→messageContext] F --> H[fromSession → RAW_PASSTHROUGH_PIPELINE auth→client→model→version→probe→provider] G --> I[ProxyForwarder maxAttemptsPerProvider=1 rawCrossProviderFallbackEnabled=true] H --> J[ProxyForwarder skipRawRetryAndProviderSwitch=true] I -- provider fails --> K[Switch to next provider no circuit breaker accounting] I -- success --> L[Response skip billing/cost/Redis tracking] J -- any error --> M[Fail immediately 503/original error] K --> IPrompt To Fix All With AI
Reviews (6): Last reviewed commit: "fix: close remaining review threads" | Re-trigger Greptile