refactor(proxy): introduce EndpointPolicy to replace hardcoded count_tokens checks#801
refactor(proxy): introduce EndpointPolicy to replace hardcoded count_tokens checks#801
Conversation
…tokens checks Replace scattered isCountTokensRequest() conditionals with a unified EndpointPolicy system resolved once at session construction time. This generalizes the "raw passthrough" behavior to cover both count_tokens and responses/compact endpoints via a single policy object. Key changes: - Add endpoint-paths.ts (path constants + normalization with case/slash/query handling) - Add endpoint-policy.ts (EndpointPolicy interface + resolution logic) - ProxySession holds immutable EndpointPolicy resolved at construction - GuardPipeline.fromSession() reads policy instead of RequestType enum - Forwarder, ResponseHandler, RequestFilter all gate on policy flags - proxy-handler uses trackConcurrentRequests from policy
📝 WalkthroughWalkthrough将基于 RequestType 的守卫与并发计数逻辑替换为基于会话的端点策略（EndpointPolicy）。新增端点路径与策略模块，并在转发器、响应处理、请求过滤和守卫管道中以策略标志为门控条件实现有条件绕过与隔离行为。 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 分钟 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 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 |
Summary of ChangesHello @ding113, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the proxy's request handling logic by introducing a robust Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an excellent refactoring that introduces the EndpointPolicy system, replacing scattered isCountTokensRequest() checks with a more robust and maintainable policy-driven approach. The centralization of endpoint path logic in endpoint-paths.ts and the clear definition of policies in endpoint-policy.ts significantly improve code clarity and organization. The changes are well-supported by a comprehensive suite of new and updated tests. I have one suggestion to clean up some duplicated code that was touched during this refactoring.
src/app/v1/_lib/proxy/forwarder.ts
Outdated
| if ( | ||
| resolvedCacheTtl && | ||
| (provider.providerType === "claude" || provider.providerType === "claude-auth") | ||
| ) { | ||
| const applied = applyCacheTtlOverrideToMessage(session.request.message, resolvedCacheTtl); | ||
| if (applied) { | ||
| logger.info("ProxyForwarder: Applied cache TTL override to request", { | ||
| providerId: provider.id, | ||
| providerName: provider.name, | ||
| cacheTtl: resolvedCacheTtl, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
This block of code, which applies a cache TTL override for Claude providers, appears to be redundant. A similar block exists later in this file (lines 2025-2036) within the more specific claude provider logic. This duplication means applyCacheTtlOverrideToMessage is called twice for the same request, which could lead to unexpected behavior.
To consolidate the logic and prevent the redundant call, I recommend removing this block. The logic will be correctly handled by the block within the provider-specific if statement.
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/v1/_lib/proxy/response-handler.ts (1)
256-365:⚠️ Potential issue | 🟠 MajorallowCircuitBreakerAccounting 只覆盖失败路径，成功路径仍会被统计。
当前仅在失败分支跳过 recordFailure，但成功分支仍会执行 recordEndpointSuccess/recordSuccess，raw_passthrough 仍会影响熔断统计，语义不一致。建议成功路径也加同样的 policy 保护。
建议修复
- if (meta.endpointId != null) { - try { - const { recordEndpointSuccess } = await import("@/lib/endpoint-circuit-breaker"); - await recordEndpointSuccess(meta.endpointId); - } catch (endpointError) { - logger.warn("[ResponseHandler] Failed to record endpoint success (stream)", { - endpointId: meta.endpointId, - providerId: meta.providerId, - error: endpointError, - }); - } - } - - try { - const { recordSuccess } = await import("@/lib/circuit-breaker"); - await recordSuccess(meta.providerId); - } catch (cbError) { - logger.warn("[ResponseHandler] Failed to record streaming success in circuit breaker", { - providerId: meta.providerId, - error: cbError, - }); - } + if (session.getEndpointPolicy().allowCircuitBreakerAccounting) { + if (meta.endpointId != null) { + try { + const { recordEndpointSuccess } = await import("@/lib/endpoint-circuit-breaker"); + await recordEndpointSuccess(meta.endpointId); + } catch (endpointError) { + logger.warn("[ResponseHandler] Failed to record endpoint success (stream)", { + endpointId: meta.endpointId, + providerId: meta.providerId, + error: endpointError, + }); + } + } + + try { + const { recordSuccess } = await import("@/lib/circuit-breaker"); + await recordSuccess(meta.providerId); + } catch (cbError) { + logger.warn("[ResponseHandler] Failed to record streaming success in circuit breaker", { + providerId: meta.providerId, + error: cbError, + }); + } + }🤖 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 256 - 365, Success responses are still updating circuit-breaker/accounting (e.g., calls to recordEndpointSuccess, recordSuccess or any raw_passthrough-related accounting) while failures are gated by session.getEndpointPolicy().allowCircuitBreakerAccounting; modify the success-path code to check session.getEndpointPolicy().allowCircuitBreakerAccounting before performing any circuit-breaker or endpoint accounting. Specifically, in the response success handling (the code path that currently invokes recordEndpointSuccess, recordSuccess or performs raw_passthrough accounting), wrap those calls with if (session.getEndpointPolicy().allowCircuitBreakerAccounting) { ... } or skip the accounting when the policy is false so success and failure paths follow the same policy semantics.
🧹 Nitpick comments (4)
tests/unit/proxy/proxy-handler-session-id-error.test.ts (1)
179-212: 新增的 RED 测试用例结构清晰，但getEndpointPolicy重复赋值是冗余的。Line 202 重新赋值
h.session.getEndpointPolicy，但 Line 16 已经定义了动态版本() => resolveEndpointPolicy(h.session.requestUrl.pathname)，会自动跟随requestUrl的变更。Line 201 设置了新的requestUrl后，Line 16 的定义已经能正确返回对应策略，无需重复赋值。建议移除冗余赋值
h.session.requestUrl = new URL(`http://localhost${pathname}`); - h.session.getEndpointPolicy = () => resolveEndpointPolicy(h.session.requestUrl.pathname); h.session.sessionId = "s_123";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/proxy/proxy-handler-session-id-error.test.ts` around lines 179 - 212, The test redundantly reassigns h.session.getEndpointPolicy after setting h.session.requestUrl; remove the second assignment so the existing dynamic getter definition h.session.getEndpointPolicy = () => resolveEndpointPolicy(h.session.requestUrl.pathname) is used (it will pick up the new h.session.requestUrl), keeping the rest of the test setup and assertions (including handleProxyRequest, h.session.requestUrl, and resolveEndpointPolicy) unchanged.src/app/v1/_lib/proxy/endpoint-paths.ts (1)
21-26:STRICT_STANDARD_ENDPOINT_PATHS的语义与成员不太直观。该数组包含
RESPONSES_COMPACT（一个 raw passthrough 端点），同时排除了MESSAGES_COUNT_TOKENS（也是 raw passthrough）和MODELS。"Strict Standard" 的命名没有清晰传达其筛选标准。建议补充一行注释说明该集合的用途和筛选逻辑，方便后续维护者理解为什么某些端点被包含/排除。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/endpoint-paths.ts` around lines 21 - 26, STRICT_STANDARD_ENDPOINT_PATHS 的命名与实际成员（包含 RESPONSES_COMPACT，排除 MESSAGES_COUNT_TOKENS 与 MODELS 等 raw passthrough/非标准端点）容易让人疑惑；请在该常量定义上方添加一行简短注释，说明该集合的用途（比如“用于对外暴露的标准写入/读取端点集合，用于 X 场景/路由判断”）以及清晰列出筛选逻辑（为何包含 V1_ENDPOINT_PATHS.RESPONSES_COMPACT、为何排除 V1_ENDPOINT_PATHS.MESSAGES_COUNT_TOKENS 与 V1_ENDPOINT_PATHS.MODELS），以便后续维护者理解包含/排除的原则；保持注释与常量靠近并使用同一文件中的 STRICT_STANDARD_ENDPOINT_PATHS / V1_ENDPOINT_PATHS 名称以便定位。tests/unit/proxy/session.test.ts (1)
400-431:createSessionForHeaders辅助函数缺少endpointPolicy字段。虽然当前使用此函数的测试（
isHeaderModified系列）不涉及端点策略，但如果后续扩展使用场景，缺少该字段会导致getEndpointPolicy()返回undefined。建议与其他测试辅助函数保持一致。建议补充
import { resolveEndpointPolicy } from "@/app/v1/_lib/proxy/endpoint-policy"; // ... Object.assign(session, { // ... existing fields ... cachedBillingModelSource: undefined, + endpointPolicy: resolveEndpointPolicy("/v1/messages"), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/proxy/session.test.ts` around lines 400 - 431, The helper createSessionForHeaders is missing the endpointPolicy property which can make ProxySession.getEndpointPolicy() return undefined later; update the object assigned to the created ProxySession instance in createSessionForHeaders to include an endpointPolicy field (matching the shape used by other helpers/tests), e.g. add endpointPolicy: <appropriate default/policy object or null> so tests that call ProxySession.getEndpointPolicy() behave consistently with other session builders.src/app/v1/_lib/proxy/session.ts (1)
805-814: 空catch块会静默吞掉异常，建议至少记录一条 debug 日志。当前实现在
pathname读取失败时静默回退到默认策略。虽然测试已覆盖此场景，但在生产环境中完全吞掉异常会增加排查难度。建议添加日志
function resolveSessionEndpointPolicy(requestUrl: URL): EndpointPolicy { try { const pathname = requestUrl.pathname; if (typeof pathname === "string" && pathname.length > 0) { return resolveEndpointPolicy(pathname); } - } catch {} + } catch (err) { + logger.debug("[resolveSessionEndpointPolicy] Failed to read pathname, using default policy", { + error: err, + }); + } return resolveEndpointPolicy("/"); }🤖 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 805 - 814, In resolveSessionEndpointPolicy, the empty catch silently swallows exceptions; update the catch to log the error (and relevant context like requestUrl or pathname) at debug level before falling back to resolveEndpointPolicy("/"); specifically, inside the catch block of function resolveSessionEndpointPolicy(requestUrl: URL) call a debug logger (e.g., console.debug(...) or the project logger if available) with the caught error and the requestUrl.pathname, then continue returning resolveEndpointPolicy("/").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/v1/_lib/proxy/endpoint-policy.ts`:
- Line 1: Replace the relative import of normalizeEndpointPath and
V1_ENDPOINT_PATHS in endpoint-policy.ts with the project path-alias import using
the `@/` prefix; locate the import statement that references "./endpoint-paths"
and change it to import normalizeEndpointPath and V1_ENDPOINT_PATHS from
"@/app/v1/_lib/proxy/endpoint-paths" so the module uses the src path alias.
In `@src/app/v1/_lib/proxy/forwarder.ts`:
- Around line 1624-1637: The raw_passthrough error handling only honored
allowRetry/allowProviderSwitch/allowCircuitBreakerAccounting inside the
PROVIDER_ERROR branch; instead, after error classification you should early-exit
based on session.getEndpointPolicy() for all error kinds so system
errors/timeouts do not trigger retries, provider switches or circuit-breaker
accounting. Update the logic in ProxyForwarder around the raw_passthrough branch
to check session.getEndpointPolicy().allowRetry, .allowProviderSwitch and
.allowCircuitBreakerAccounting immediately after classifying proxyError (using
variables proxyError/lastError/currentProvider) and throw/return when the policy
forbids retry/switch/accounting; also gate calls to recordFailure,
recordEndpointFailure, recordSuccess and recordEndpointSuccess with those same
policy flags so accounting only happens when allowed.
In `@src/app/v1/_lib/proxy/guard-pipeline.ts`:
- Line 3: Import of EndpointPolicy should use the project path alias; update the
import in guard-pipeline.ts to reference EndpointPolicy via the '@/...' alias
instead of a relative path (replace the existing import of EndpointPolicy from
"./endpoint-policy" with an import that uses the '@/...' path to the same
module), ensuring the module specifier matches the repo's src root alias and
keeps the same exported symbol name EndpointPolicy.
---
Outside diff comments:
In `@src/app/v1/_lib/proxy/response-handler.ts`:
- Around line 256-365: Success responses are still updating
circuit-breaker/accounting (e.g., calls to recordEndpointSuccess, recordSuccess
or any raw_passthrough-related accounting) while failures are gated by
session.getEndpointPolicy().allowCircuitBreakerAccounting; modify the
success-path code to check
session.getEndpointPolicy().allowCircuitBreakerAccounting before performing any
circuit-breaker or endpoint accounting. Specifically, in the response success
handling (the code path that currently invokes recordEndpointSuccess,
recordSuccess or performs raw_passthrough accounting), wrap those calls with if
(session.getEndpointPolicy().allowCircuitBreakerAccounting) { ... } or skip the
accounting when the policy is false so success and failure paths follow the same
policy semantics.
---
Nitpick comments:
In `@src/app/v1/_lib/proxy/endpoint-paths.ts`:
- Around line 21-26: STRICT_STANDARD_ENDPOINT_PATHS 的命名与实际成员（包含
RESPONSES_COMPACT，排除 MESSAGES_COUNT_TOKENS 与 MODELS 等 raw
passthrough/非标准端点）容易让人疑惑；请在该常量定义上方添加一行简短注释，说明该集合的用途（比如“用于对外暴露的标准写入/读取端点集合，用于 X
场景/路由判断”）以及清晰列出筛选逻辑（为何包含 V1_ENDPOINT_PATHS.RESPONSES_COMPACT、为何排除
V1_ENDPOINT_PATHS.MESSAGES_COUNT_TOKENS 与
V1_ENDPOINT_PATHS.MODELS），以便后续维护者理解包含/排除的原则；保持注释与常量靠近并使用同一文件中的
STRICT_STANDARD_ENDPOINT_PATHS / V1_ENDPOINT_PATHS 名称以便定位。
In `@src/app/v1/_lib/proxy/session.ts`:
- Around line 805-814: In resolveSessionEndpointPolicy, the empty catch silently
swallows exceptions; update the catch to log the error (and relevant context
like requestUrl or pathname) at debug level before falling back to
resolveEndpointPolicy("/"); specifically, inside the catch block of function
resolveSessionEndpointPolicy(requestUrl: URL) call a debug logger (e.g.,
console.debug(...) or the project logger if available) with the caught error and
the requestUrl.pathname, then continue returning resolveEndpointPolicy("/").
In `@tests/unit/proxy/proxy-handler-session-id-error.test.ts`:
- Around line 179-212: The test redundantly reassigns
h.session.getEndpointPolicy after setting h.session.requestUrl; remove the
second assignment so the existing dynamic getter definition
h.session.getEndpointPolicy = () =>
resolveEndpointPolicy(h.session.requestUrl.pathname) is used (it will pick up
the new h.session.requestUrl), keeping the rest of the test setup and assertions
(including handleProxyRequest, h.session.requestUrl, and resolveEndpointPolicy)
unchanged.
In `@tests/unit/proxy/session.test.ts`:
- Around line 400-431: The helper createSessionForHeaders is missing the
endpointPolicy property which can make ProxySession.getEndpointPolicy() return
undefined later; update the object assigned to the created ProxySession instance
in createSessionForHeaders to include an endpointPolicy field (matching the
shape used by other helpers/tests), e.g. add endpointPolicy: <appropriate
default/policy object or null> so tests that call
ProxySession.getEndpointPolicy() behave consistently with other session
builders.
| @@ -0,0 +1,68 @@ | |||
| import { normalizeEndpointPath, V1_ENDPOINT_PATHS } from "./endpoint-paths"; | |||
There was a problem hiding this comment.
endpoint-paths 导入请改用 @/ 路径别名。
该文件位于 src 内，新增导入按规范应使用 @/ 前缀。As per coding guidelines: Use path alias @/ to reference files in ./src/ directory.
建议修复
-import { normalizeEndpointPath, V1_ENDPOINT_PATHS } from "./endpoint-paths";
+import { normalizeEndpointPath, V1_ENDPOINT_PATHS } from "@/app/v1/_lib/proxy/endpoint-paths";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { normalizeEndpointPath, V1_ENDPOINT_PATHS } from "./endpoint-paths"; | |
| import { normalizeEndpointPath, V1_ENDPOINT_PATHS } from "@/app/v1/_lib/proxy/endpoint-paths"; |
🤖 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` at line 1, Replace the relative
import of normalizeEndpointPath and V1_ENDPOINT_PATHS in endpoint-policy.ts with
the project path-alias import using the `@/` prefix; locate the import statement
that references "./endpoint-paths" and change it to import normalizeEndpointPath
and V1_ENDPOINT_PATHS from "@/app/v1/_lib/proxy/endpoint-paths" so the module
uses the src path alias.
| // Raw passthrough endpoints: no circuit breaker, no provider switch, no retry | ||
| if (!session.getEndpointPolicy().allowRetry) { | ||
| logger.debug( | ||
| "ProxyForwarder: count_tokens request error, skipping circuit breaker and provider switch", | ||
| "ProxyForwarder: raw passthrough endpoint error, skipping circuit breaker and provider switch", | ||
| { | ||
| providerId: currentProvider.id, | ||
| providerName: currentProvider.name, | ||
| statusCode, | ||
| error: proxyError.message, | ||
| policyKind: session.getEndpointPolicy().kind, | ||
| } | ||
| ); | ||
| // 直接抛出错误，不重试，不切换供应商 | ||
| // Throw immediately: no retry, no provider switch | ||
| throw lastError; |
There was a problem hiding this comment.
raw_passthrough 仅在 PROVIDER_ERROR 分支短路，系统错误仍会重试/计入熔断。
allowRetry/allowProviderSwitch/allowCircuitBreakerAccounting 目前只在 PROVIDER_ERROR 分支生效，SYSTEM_ERROR/超时仍会重试、切换供应商并记录熔断，和 policy 语义不一致。建议在错误分类后统一早退，并在 recordFailure/recordEndpointFailure/recordSuccess/recordEndpointSuccess 处统一 gate。
建议修复
- // Raw passthrough endpoints: no circuit breaker, no provider switch, no retry
- if (!session.getEndpointPolicy().allowRetry) {
+ const policy = session.getEndpointPolicy();
+ // Raw passthrough endpoints: no circuit breaker, no provider switch, no retry
+ if (!policy.allowRetry) {
logger.debug(
"ProxyForwarder: raw passthrough endpoint error, skipping circuit breaker and provider switch",
{
providerId: currentProvider.id,
providerName: currentProvider.name,
statusCode,
error: proxyError.message,
- policyKind: session.getEndpointPolicy().kind,
+ policyKind: policy.kind,
}
);
// Throw immediately: no retry, no provider switch
throw lastError;
}🤖 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 1624 - 1637, The
raw_passthrough error handling only honored
allowRetry/allowProviderSwitch/allowCircuitBreakerAccounting inside the
PROVIDER_ERROR branch; instead, after error classification you should early-exit
based on session.getEndpointPolicy() for all error kinds so system
errors/timeouts do not trigger retries, provider switches or circuit-breaker
accounting. Update the logic in ProxyForwarder around the raw_passthrough branch
to check session.getEndpointPolicy().allowRetry, .allowProviderSwitch and
.allowCircuitBreakerAccounting immediately after classifying proxyError (using
variables proxyError/lastError/currentProvider) and throw/return when the policy
forbids retry/switch/accounting; also gate calls to recordFailure,
recordEndpointFailure, recordSuccess and recordEndpointSuccess with those same
policy flags so accounting only happens when allowed.
| @@ -1,5 +1,6 @@ | |||
| import { ProxyAuthenticator } from "./auth-guard"; | |||
| import { ProxyClientGuard } from "./client-guard"; | |||
| import type { EndpointPolicy } from "./endpoint-policy"; | |||
There was a problem hiding this comment.
EndpointPolicy 导入请改用 @/ 路径别名。
该文件位于 src 内，新增导入按规范应使用 @/ 前缀。As per coding guidelines: Use path alias @/ to reference files in ./src/ directory.
建议修复
-import type { EndpointPolicy } from "./endpoint-policy";
+import type { EndpointPolicy } from "@/app/v1/_lib/proxy/endpoint-policy";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import type { EndpointPolicy } from "./endpoint-policy"; | |
| import type { EndpointPolicy } from "@/app/v1/_lib/proxy/endpoint-policy"; |
🤖 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` at line 3, Import of EndpointPolicy
should use the project path alias; update the import in guard-pipeline.ts to
reference EndpointPolicy via the '@/...' alias instead of a relative path
(replace the existing import of EndpointPolicy from "./endpoint-policy" with an
import that uses the '@/...' path to the same module), ensuring the module
specifier matches the repo's src root alias and keeps the same exported symbol
name EndpointPolicy.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
This PR introduces a well-designed EndpointPolicy system that successfully replaces scattered isCountTokensRequest() conditionals with a unified, declarative approach. The implementation is clean, the test coverage is comprehensive (341 new tests across 3 files), and the refactoring maintains backward compatibility while fixing the Codex CLI compact endpoint issues.
PR Size: XL
- Lines changed: 1259 (1062 additions, 197 deletions)
- Files changed: 22
Split Suggestion for Future: While this PR is well-scoped for a refactoring, it could theoretically be split into:
- Core policy infrastructure (endpoint-paths.ts, endpoint-policy.ts, session.ts changes)
- Guard pipeline integration (guard-pipeline.ts)
- Forwarder and response handler integration (forwarder.ts, response-handler.ts, filters)
- Test additions
However, given the interdependent nature of these changes, keeping them together is reasonable.
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 1 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 2 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Medium Priority Issues (Should Fix)
1. [ERROR-SILENT] Empty catch block in session.ts:811
- Location:
src/app/v1/_lib/proxy/session.ts:811 - Issue: The
resolveSessionEndpointPolicyfunction silently swallows errors - Fix: Add debug logging:
} catch (error) {
logger.debug("[ProxySession] Failed to resolve endpoint policy from URL, using default", {
error: error instanceof Error ? error.message : String(error),
});
}Low Priority Issues (Author's Discretion)
2. [COMMENT-INCOMPLETE] Outdated comments in proxy-handler.ts
- Location: Lines 62 and 98
- Issue: Comments say "跳过 count_tokens" but should say "跳过 raw passthrough 端点" to match the new generic policy
Review Coverage
- Logic and correctness - Clean implementation, good architectural decisions
- Security (OWASP Top 10) - No issues found
- Error handling - One silent catch block to address
- Type safety - Proper use of readonly types and Object.freeze
- Documentation accuracy - Minor comment updates suggested
- Test coverage - Comprehensive (341 new tests across 3 files)
- Code clarity - Well-structured, follows existing patterns
Highlights
- Excellent test coverage: Path normalization edge cases, policy parity tests, bypass completeness assertions
- Clean architecture: Immutable frozen policy objects, single-responsibility path normalization
- Backward compatibility:
COUNT_TOKENS_PIPELINEalias andfromRequestType()deprecated but retained - Bug fix: Properly addresses the
prompt_cache_keyinjection bug for/v1/responses/compact
Automated review by Claude AI
… in local var - Remove redundant first applyCacheTtlOverrideToMessage call (lines 1905-1917) that duplicated the post-Anthropic-overrides call (lines 2025-2036) - Cache session.getEndpointPolicy() in local variable in error handling path to avoid repeated accessor calls Addresses: gemini-code-assist and coderabbitai review comments
🧪 测试结果
总体结果: ✅ 所有测试通过 |
| // Raw passthrough endpoints: no circuit breaker, no provider switch, no retry | ||
| const endpointPolicy = session.getEndpointPolicy(); | ||
| if (!endpointPolicy.allowRetry) { | ||
| logger.debug( |
There was a problem hiding this comment.
[CRITICAL] [LOGIC-BUG] Raw passthrough EndpointPolicy can still mutate circuit-breaker state / trigger provider switching
Why this is a problem: the raw passthrough branch is currently keyed off:
const endpointPolicy = session.getEndpointPolicy();
if (!endpointPolicy.allowRetry) {
// ...
throw lastError;
}But RAW_PASSTHROUGH_ENDPOINT_POLICY also sets allowCircuitBreakerAccounting: false and allowProviderSwitch: false. Today this check only prevents this retry/switch path — it does not prevent earlier/lateral circuit-breaker writes (e.g. vendor-type circuit open) or other error categories (system/network errors) from calling recordFailure(...) / entering provider-switch logic.
That means /v1/messages/count_tokens and /v1/responses/compact can still influence global availability decisions and failover behaviour, contradicting the policy contract.
Suggested fix: enforce policy at the top of the error-handling path, before any circuit-breaker/provider-switch side effects, and gate all circuit-breaker writes behind endpointPolicy.allowCircuitBreakerAccounting (and provider switching behind endpointPolicy.allowProviderSwitch).
const endpointPolicy = session.getEndpointPolicy();
if (!endpointPolicy.allowRetry) {
logger.debug("ProxyForwarder: raw passthrough endpoint error, skipping retry/provider switch/circuit breaker", {
providerId: currentProvider.id,
statusCode,
policyKind: endpointPolicy.kind,
});
throw lastError;
}
// Example: only open vendor-type circuit when accounting is allowed
if (endpointPolicy.allowCircuitBreakerAccounting && currentProvider.providerVendorId && statusCode === 524 /* ... */) {
await recordVendorTypeAllEndpointsTimeout(currentProvider.providerVendorId, currentProvider.providerType);
}
| return resolveEndpointPolicy(pathname); | ||
| } | ||
| } catch {} | ||
|
|
There was a problem hiding this comment.
[CRITICAL] [ERROR-SWALLOWED] Empty catch {} silently suppresses endpoint-policy resolution failures
Why this is a problem: in resolveSessionEndpointPolicy, this new code swallows all exceptions and silently falls back to the default policy:
try {
const pathname = requestUrl.pathname;
if (typeof pathname === "string" && pathname.length > 0) {
return resolveEndpointPolicy(pathname);
}
} catch {}
return resolveEndpointPolicy("/");If anything ever goes wrong here (unexpected requestUrl, runtime differences, future refactors), raw passthrough endpoints will quietly revert to the default pipeline (re-enabling preprocessing/circuit-breaker logic) with no log trail.
Suggested fix: either remove the try/catch entirely (preferred — URL.pathname should be safe), or at minimum log the error before falling back.
function resolveSessionEndpointPolicy(requestUrl: URL): EndpointPolicy {
const pathname = requestUrl.pathname;
return resolveEndpointPolicy(pathname.length > 0 ? pathname : "/");
}
// OR (if you want to keep a fallback)
// } catch (error) {
// logger.warn("[ProxySession] Failed to resolve endpoint policy; defaulting to /", { error });
// }
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/app/v1/_lib/proxy/forwarder.ts`:
- Around line 1624-1639: 在 ProxyForwarder 中把 raw passthrough
的策略短路判断上移到错误分类之后、重试/切换供应商逻辑之前：在错误分类完成后（即在现有分类分支开始执行重试/provider switch 前的位置）读取
session.getEndpointPolicy() 并用 endpointPolicy.allowRetry (或命名为
allowCircuitBreakerAccounting) 决定是否早退；若不允许则立即跳过重试与 provider
switch，并且在所有计账处（recordFailure、recordSuccess、recordEndpointFailure、recordEndpointSuccess）前加上同一门控判断以避免对
raw passthrough 端点做熔断/成功/失败记账；确保原先仅在 PROVIDER_ERROR 分支的短路逻辑被移除并统一由该集中判断控制。
There was a problem hiding this comment.
Code Review Summary
This PR is an XL refactor that introduces EndpointPolicy and rewires core proxy pipeline behavior. There are two critical policy-consistency issues that can cause raw passthrough endpoints to produce unintended side effects (circuit breaker/provider switching) and make endpoint classification failures hard to diagnose.
PR Size: XL
- Lines changed: 1250
- Files changed: 22
Split suggestions (recommended for reviewability/risk reduction):
- PR1: Introduce
endpoint-paths.ts+endpoint-policy.ts+ session plumbing (getEndpointPolicy()) - PR2: Wire policy through forwarder/response-handler/guard pipeline (behavioral changes)
- PR3: Add/adjust tests for policy parity + bypass guarantees
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 1 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 1 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
src/app/v1/_lib/proxy/forwarder.ts:1626(and vendor-type circuit open atsrc/app/v1/_lib/proxy/forwarder.ts:1616): raw passthrough policy isn’t enforced before circuit-breaker/provider-switch side effects;allowCircuitBreakerAccounting/allowProviderSwitchare not consistently respected. Confidence: 98src/app/v1/_lib/proxy/session.ts:811: emptycatch {}silently swallows endpoint policy resolution errors, causing silent fallback to default policy. Confidence: 95
High Priority Issues (Should Fix)
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Codex AI
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| readonly trackConcurrentRequests: boolean; | ||
| readonly bypassRequestFilters: boolean; | ||
| readonly bypassForwarderPreprocessing: boolean; | ||
| readonly bypassSpecialSettings: boolean; |
There was a problem hiding this comment.
Unused policy field bypassSpecialSettings
bypassSpecialSettings is defined on the EndpointPolicy interface and set on both policy objects, but it is never read anywhere in the production source code. The same applies to allowProviderSwitch (line 11) and endpointPoolStrictness (line 18) — none of these fields are consumed by any component in this PR. If these are placeholders for future work, consider adding a brief comment indicating that; otherwise they add surface area to the interface without being tested in an integration context.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/endpoint-policy.ts
Line: 16:16
Comment:
**Unused policy field `bypassSpecialSettings`**
`bypassSpecialSettings` is defined on the `EndpointPolicy` interface and set on both policy objects, but it is never read anywhere in the production source code. The same applies to `allowProviderSwitch` (line 11) and `endpointPoolStrictness` (line 18) — none of these fields are consumed by any component in this PR. If these are placeholders for future work, consider adding a brief comment indicating that; otherwise they add surface area to the interface without being tested in an integration context.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| export const STANDARD_ENDPOINT_PATHS = [ | ||
| V1_ENDPOINT_PATHS.MESSAGES, | ||
| V1_ENDPOINT_PATHS.MESSAGES_COUNT_TOKENS, | ||
| V1_ENDPOINT_PATHS.RESPONSES, | ||
| V1_ENDPOINT_PATHS.RESPONSES_COMPACT, | ||
| V1_ENDPOINT_PATHS.CHAT_COMPLETIONS, | ||
| V1_ENDPOINT_PATHS.MODELS, | ||
| ] as const; | ||
|
|
||
| export const STRICT_STANDARD_ENDPOINT_PATHS = [ | ||
| V1_ENDPOINT_PATHS.MESSAGES, | ||
| V1_ENDPOINT_PATHS.RESPONSES, | ||
| V1_ENDPOINT_PATHS.RESPONSES_COMPACT, | ||
| V1_ENDPOINT_PATHS.CHAT_COMPLETIONS, | ||
| ] as const; |
There was a problem hiding this comment.
Several exports are unused across the codebase
STANDARD_ENDPOINT_PATHS, STRICT_STANDARD_ENDPOINT_PATHS, isStandardEndpointPath, isStrictStandardEndpointPath, and toV1RoutePath are all exported but never imported or used anywhere in the source or test files. If these are intended for future use, a brief comment would help; otherwise consider removing them to keep the module focused.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/endpoint-paths.ts
Line: 12:26
Comment:
**Several exports are unused across the codebase**
`STANDARD_ENDPOINT_PATHS`, `STRICT_STANDARD_ENDPOINT_PATHS`, `isStandardEndpointPath`, `isStrictStandardEndpointPath`, and `toV1RoutePath` are all exported but never imported or used anywhere in the source or test files. If these are intended for future use, a brief comment would help; otherwise consider removing them to keep the module focused.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
- Identified and reviewed PR
#801and applied thesize/XLlabel. - Left 2 inline review comments on diff lines:
src/app/v1/_lib/proxy/forwarder.ts(raw passthrough policy not consistently enforced before circuit-breaker/provider-switch side effects)src/app/v1/_lib/proxy/session.ts(emptycatch {}silently swallowing endpoint-policy resolution errors)
- Submitted the required “Code Review Summary” via
gh pr review --commentwith severity counts, confidence scores, and XL split suggestions.
github-project-automation
bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
1 participant
Summary
Introduce
EndpointPolicysystem to replace scatteredisCountTokensRequest()conditionals across the proxy pipeline. This generalizes the "raw passthrough" behavior to cover bothcount_tokensandresponses/compactendpoints via a unified policy object resolved once atProxySessionconstruction.Related Issues
prompt_cache_keyinjection bug for/v1/responses/compactendpoint by bypassing forwarder preprocessing for raw passthrough endpoints.Problem
Scattered conditional logic: The codebase had hardcoded
isCountTokensRequest()checks spread across proxy-handler, forwarder, response-handler, and filters - making it difficult to maintain consistent special-case handling.Codex CLI compact endpoint issues (Issue 中转codex cli的remote compact异常的BUG #797):
prompt_cache_keyparameter was being injected into/v1/responses/compactrequestsExtensibility: Adding new special-case endpoints required modifying multiple files with repetitive conditional checks.
Solution
Create a declarative
EndpointPolicysystem:Centralized path constants (
endpoint-paths.ts): Case-insensitive, trailing-slash tolerant, query-string safe path normalizationImmutable policy objects (
endpoint-policy.ts): Two frozen policies:DEFAULT_ENDPOINT_POLICY: Full pipeline for normal chat requestsRAW_PASSTHROUGH_ENDPOINT_POLICY: Minimal processing forcount_tokensandresponses/compactPolicy-driven behavior: All components now read from
session.getEndpointPolicy()instead of hardcoded checks:trackConcurrentRequests: Skip for passthrough endpointsbypassForwarderPreprocessing: Skip model redirects, cache TTL overrides, provider-specific injections (fixes 中转codex cli的remote compact异常的BUG #797 bug 1)bypassRequestFilters: Skip global/provider request filtersbypassResponseRectifier: Skip response fixer processingallowRetry/allowCircuitBreakerAccounting: Skip for passthrough endpointsChanges
Core Changes
src/app/v1/_lib/proxy/endpoint-paths.ts- Centralized endpoint path constants + normalization utilitiessrc/app/v1/_lib/proxy/endpoint-policy.ts-EndpointPolicyinterface withresolveEndpointPolicy()resolversrc/app/v1/_lib/proxy/session.ts- Hold immutableEndpointPolicy, expose viagetEndpointPolicy()src/app/v1/_lib/proxy/guard-pipeline.ts- NewfromSession()/fromEndpointPolicy()builders; renameCOUNT_TOKENS_PIPELINE->RAW_PASSTHROUGH_PIPELINEsrc/app/v1/_lib/proxy/forwarder.ts- Gate retry/circuit-breaker/preprocessing on policy flags (allowRetry,bypassForwarderPreprocessing)src/app/v1/_lib/proxy/response-handler.ts- Gate circuit-breaker accounting and response rectifier on policy flagssrc/app/v1/_lib/proxy/request-filter.ts/provider-request-filter.ts- Early return whenbypassRequestFiltersis setsrc/app/v1/_lib/proxy-handler.ts- UsetrackConcurrentRequestsfrom policy instead ofisCountTokensRequest()Tests (12 files)
tests/unit/proxy/endpoint-path-normalization.test.ts- Path normalization edge casestests/unit/proxy/endpoint-policy.test.ts- Policy resolution correctnesstests/unit/proxy/endpoint-policy-parity.test.ts- T11-T14: parity, bypass completeness, non-target regression, edge casesendpointPolicyin session stubsBreaking Changes
None. This is an internal refactoring that maintains backward compatibility.
Test Plan
bun run test- 497 tests pass (45 files)bun run typecheck- cleanbun run lint- cleanImplementation Notes
ProxySessionconstruction and frozen - no runtime overheadRAW_PASSTHROUGH_PIPELINEguard steps:[auth, client, model, version, probe, provider]- minimal validation onlyfromRequestType()is deprecated but retained for backward compatibility; new code should usefromSession()orfromEndpointPolicy()Description enhanced by Claude AI
Greptile Summary
Replaces scattered
isCountTokensRequest()conditionals with a centralizedEndpointPolicysystem resolved once atProxySessionconstruction. Thecount_tokensandresponses/compactendpoints now share a unifiedraw_passthroughpolicy that bypasses forwarder preprocessing (fixing theprompt_cache_keyinjection bug for/v1/responses/compact), request filters, response rectifier, circuit breaker accounting, retry logic, and concurrent request tracking. Normal chat endpoints retain the full pipeline via the frozendefaultpolicy.endpoint-paths.ts(path constants + normalization),endpoint-policy.ts(EndpointPolicyinterface + resolver)session.tsholds immutable policy;proxy-handler.ts,forwarder.ts,response-handler.ts,request-filter.ts,provider-request-filter.ts, andguard-pipeline.tsall switch from hardcoded checks to policy-driven behaviorCOUNT_TOKENS_PIPELINErenamed toRAW_PASSTHROUGH_PIPELINEwith fewer steps (removesrequestFilter/providerRequestFilter); backward-compat alias retainedendpointPolicyin session stubsEndpointPolicyfields (bypassSpecialSettings,allowProviderSwitch,endpointPoolStrictness) andendpoint-paths.tsexports are defined but not consumed yet — these appear to be forward-looking placeholdersConfidence Score: 4/5
src/app/v1/_lib/proxy/endpoint-policy.tsandsrc/app/v1/_lib/proxy/endpoint-paths.tshave unused exported symbols that could be cleaned up.Important Files Changed
Flowchart
flowchart TD A[Incoming Request] --> B[ProxySession.fromContext] B --> C[resolveSessionEndpointPolicy<br/>pathname → EndpointPolicy] C --> D{policy.guardPreset} D -->|chat| E[CHAT_PIPELINE<br/>auth → sensitive → client → model<br/>→ version → probe → session → warmup<br/>→ requestFilter → rateLimit → provider<br/>→ providerRequestFilter → messageContext] D -->|raw_passthrough| F[RAW_PASSTHROUGH_PIPELINE<br/>auth → client → model<br/>→ version → probe → provider] E --> G[trackConcurrentRequests ✓] F --> H[trackConcurrentRequests ✗] G --> I[ProxyForwarder.send] H --> I I --> J{bypassForwarderPreprocessing?} J -->|No| K[Model redirect + Provider overrides<br/>+ Cache TTL + Billing rectifier] J -->|Yes| L[Skip all preprocessing] K --> M[Upstream Request] L --> M M --> N[ProxyResponseHandler.dispatch] N --> O{bypassResponseRectifier?} O -->|No| P[ResponseFixer.process] O -->|Yes| Q[Skip ResponseFixer] P --> R{allowCircuitBreakerAccounting?} Q --> R R -->|Yes| S[Record success/failure in circuit breaker] R -->|No| T[Skip circuit breaker accounting] S --> U[Return Response] T --> ULast reviewed commit: 40b030d