Conversation
* feat: improve provider detection templates * fix: address provider detection review feedback * chore: format code (feat-provider-detect-templates-02e4d1f) * test: cover provider detection fallback retries * chore: format code (feat-provider-detect-templates-4dc1701) * fix: tighten provider detection validation --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* fix: honor disabled error rules during queued reloads * fix: propagate matched rule metadata in hedge errors * fix: avoid hot retries on error rule reload failure * fix: avoid redundant error-rule reload passes * fix: await queued error rule reload completion
…on (#1009) * fix: prevent STREAM_PROCESSING_ERROR from premature agent pool eviction The agent pool cleanup timer was destroying HTTP agents after 5min TTL with no awareness of in-flight streaming requests, causing ClientDestroyedError and ERR_HTTP2_STREAM_ERROR for long-running responses. These errors were also misclassified because isTransportError did not recognize them. Three fixes: 1. Add UND_ERR_DESTROYED, UND_ERR_CLOSED, and HTTP/2 errors to isTransportError() so they classify as STREAM_UPSTREAM_ABORTED 2. Add reference counting (activeRequests) to AgentPool to prevent eviction of agents with in-flight requests (30min hard upper bound) 3. Wire up releaseAgent lifecycle in forwarder/response-handler to decrement refcount when streams complete Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: harden agent pool refcount against cross-generation misattribution Addresses bot review feedback on #1009 covering four root causes that still let STREAM_PROCESSING_ERROR leak through under concurrency: - Track dispatcher generation ids so releaseAgent can ignore stale releases from old dispatchers after markUnhealthy / 30-min hard expire / LRU eviction rebuilds the same cacheKey - Increment activeRequests for pending-creation waiters, not just the creator, so concurrent bursts don't prematurely evict the shared agent once the first request completes - Move agent acquisition inside forwarder's fetch try block so any exception between acquire and fetch still releases the refcount - Clear proxyConfig after a successful proxy-to-direct fallback to prevent session.releaseAgent double-decrementing the shared agent - Add ERR_HTTP2_STREAM_ERROR to transport error codes so wrapped HTTP/2 errors on the cause chain are detected by isTransportError Adds regression coverage for concurrent pending-creation waiter counting (Promise.all with blocked createAgent), stale dispatcher generation release safety, and HTTP/2 cause-chain detection. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: respect case-sensitive provider model ids * fix: keep model picker ids case-sensitive
📝 WalkthroughWalkthrough此PR将多语言消息参数化为动态 Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
enhancement
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors provider rule management and API testing, increasing rule limits to 100,000 and introducing case-sensitive model matching. It enhances the proxy agent pool with reference counting to prevent premature connection eviction and updates the API test service to support automatic template probing. Review feedback identifies a missing parsers.ts file that will cause build failures and recommends using the nullish coalescing operator (??) instead of logical OR (||) when handling parsed response content to ensure empty strings are processed correctly and do not trigger structural fallbacks.
|
|
||
| import { createProxyAgentForProvider, type ProviderProxyConfig } from "@/lib/proxy-agent"; | ||
| import { getPreset, getPresetPayload } from "./presets"; | ||
| import { parseResponse } from "./parsers"; |
There was a problem hiding this comment.
The file src/lib/provider-testing/parsers/index.ts (or parsers.ts) appears to be missing from this pull request. The code adds an import for parseResponse from this path on line 8 and uses it on line 198, but the file itself is not present in the provided patches. This will lead to build failures.
| // Tier 1: HTTP Status validation | ||
| const contentType = response.headers.get("content-type") || undefined; | ||
| const parsed = parseResponse(config.providerType, responseBody, contentType); | ||
| const validationInput = parsed.content || responseBody; |
There was a problem hiding this comment.
Using the logical OR operator (||) for validationInput causes a fallback to the full responseBody if parsed.content is an empty string. This can lead to false positives in content validation if the successContains keyword matches a key or value in the raw JSON structure (e.g., matching the string "content" against the JSON key "content": ""). It is recommended to use the nullish coalescing operator (??) to ensure that an empty string is treated as valid content and doesn't trigger the fallback.
| const validationInput = parsed.content || responseBody; | |
| const validationInput = parsed.content ?? responseBody; |
| content: responseBody.slice(0, 500), // Preview for quick view | ||
| rawResponse: responseBody.slice(0, 5000), // Full response for detailed inspection | ||
| model: parsed.model, | ||
| content: parsed.content || responseBody, |
There was a problem hiding this comment.
Similar to the validation input, using || here will cause the UI to display the entire raw JSON response if the model returns an empty string. Using the nullish coalescing operator (??) ensures that an empty string is preserved as the content, while still falling back to the raw response if parsing failed to extract any content field at all.
| content: parsed.content || responseBody, | |
| content: parsed.content ?? responseBody, |
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| export const PROVIDER_RULE_LIMITS = { | ||
| // 模型白名单 / 重定向规则保存在 JSONB 中，这里统一放宽到支持大规模路由表 | ||
| MAX_ITEMS: 100_000, |
There was a problem hiding this comment.
MAX_ITEMS = 100_000 will render as the raw string "100000" in all five locales — e.g. "You can add up to 100000 allowlist rules" and "Reached the 100000-rule limit while importing upstream models". The hyphenated form in the latter is especially jarring. Consider using next-intl's {max, number} ICU format token so the locale's digit grouping is applied automatically, or format the value before passing it to t():
In the call sites, switch to locale-aware formatting:
// In allowed-model-rule-editor.tsx / model-redirect-editor.tsx
t("maxRules", { max: Intl.NumberFormat().format(MAX_ALLOWED_MODEL_RULES) })Or update the i18n messages to use ICU number format: "You can add up to {max, number} allowlist rules".
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/constants/provider.constants.ts
Line: 21
Comment:
**Unformatted large number in user-facing messages**
`MAX_ITEMS = 100_000` will render as the raw string `"100000"` in all five locales — e.g. `"You can add up to 100000 allowlist rules"` and `"Reached the 100000-rule limit while importing upstream models"`. The hyphenated form in the latter is especially jarring. Consider using `next-intl`'s `{max, number}` ICU format token so the locale's digit grouping is applied automatically, or format the value before passing it to `t()`:
In the call sites, switch to locale-aware formatting:
```ts
// In allowed-model-rule-editor.tsx / model-redirect-editor.tsx
t("maxRules", { max: Intl.NumberFormat().format(MAX_ALLOWED_MODEL_RULES) })
```
Or update the i18n messages to use ICU number format: `"You can add up to {max, number} allowlist rules"`.
How can I resolve this? If you propose a fix, please make it concise.| if (pending) { | ||
| // Wait for the pending creation and return its result | ||
| const result = await pending; | ||
| // ⚠️ 关键：等待 pending 创建的调用者也必须计入 activeRequests。 | ||
| // 创建者在 createAgentWithCache 里把 activeRequests 初始化为 1（只代表它自己）， | ||
| // 这里的每个等待者都要再 +1，否则一旦创建者完成并减到 0，cleanup/LRU 会在其它 | ||
| // 仍在飞行中的请求头上把共享 agent 驱逐掉，又会把 STREAM_PROCESSING_ERROR 带回来。 | ||
| const currentCached = this.cache.get(cacheKey); | ||
| if (currentCached && currentCached.id === result.dispatcherId) { | ||
| currentCached.activeRequests++; | ||
| currentCached.lastUsedAt = Date.now(); | ||
| } | ||
| // Count as cache hit - we're reusing the pending result, not creating a new agent | ||
| // Note: Don't decrement cacheMisses here since we never incremented it for this request | ||
| this.stats.cacheHits++; |
There was a problem hiding this comment.
activeRequests after resolving, but returns the original result
When multiple callers race and the second caller awaits the pending promise, it increments activeRequests on the cached entry only if currentCached.id === result.dispatcherId. The subtle risk: if the 30-minute hard-expire elapses between the moment await pending resolves and the moment the waiter's increment runs, the cache entry would have been replaced with a new id. The condition would be false, activeRequests would not be incremented, and the subsequent releaseAgent call would also be a no-op — leaving the new agent's activeRequests permanently at 1 until its own hard-expire fires. Consider adding a comment noting this invariant and the assumption that the pending creation window is always well under 30 minutes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/proxy-agent/agent-pool.ts
Line: 244-258
Comment:
**Pending-creation waiter increments `activeRequests` after resolving, but returns the original `result`**
When multiple callers race and the second caller awaits the `pending` promise, it increments `activeRequests` on the cached entry only if `currentCached.id === result.dispatcherId`. The subtle risk: if the 30-minute hard-expire elapses between the moment `await pending` resolves and the moment the waiter's increment runs, the cache entry would have been replaced with a new `id`. The condition would be false, `activeRequests` would not be incremented, and the subsequent `releaseAgent` call would also be a no-op — leaving the new agent's `activeRequests` permanently at `1` until its own hard-expire fires. Consider adding a comment noting this invariant and the assumption that the pending creation window is always well under 30 minutes.
How can I resolve this? If you propose a fix, please make it concise.| @@ -2900,6 +3001,7 @@ export class ProxyForwarder { | |||
| const sessionWithTimeout = session as ProxySession & { | |||
There was a problem hiding this comment.
releaseAgent attached as an ad-hoc property on ProxySession
The releaseAgent callback is grafted onto the session object via a local type cast (session as ProxySession & { releaseAgent?: () => void }). This pattern is repeated both here and in response-handler.ts. If future callers access session without the cast, the callback is invisible and could be silently skipped. Consider adding releaseAgent?: () => void to the actual ProxySession interface (or a dedicated StreamingContext type) so the contract is explicit and compiler-checked.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/forwarder.ts
Line: 3001
Comment:
**`releaseAgent` attached as an ad-hoc property on `ProxySession`**
The `releaseAgent` callback is grafted onto the session object via a local type cast (`session as ProxySession & { releaseAgent?: () => void }`). This pattern is repeated both here and in `response-handler.ts`. If future callers access `session` without the cast, the callback is invisible and could be silently skipped. Consider adding `releaseAgent?: () => void` to the actual `ProxySession` interface (or a dedicated `StreamingContext` type) so the contract is explicit and compiler-checked.
How can I resolve this? If you propose a fix, please make it concise.![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: d2e71b8254
ℹ️ 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".
| if (agentCacheKeyToRelease && agentDispatcherIdToRelease) { | ||
| const pool = getGlobalAgentPool(); | ||
| sessionWithTimeout.releaseAgent = () => { | ||
| pool.releaseAgent(agentCacheKeyToRelease, agentDispatcherIdToRelease); |
There was a problem hiding this comment.
This callback introduces in-flight ref counting, but it assumes response handling will always run a finalizer. In handleStream, requests with no messageContext return immediately (which is expected on raw passthrough routes like /v1/responses/compact), so releaseAgent is never called. In proxy/HTTP2 pooled paths that leaks activeRequests per request, blocking normal TTL cleanup and eventually causing avoidable agent eviction/churn under load.
Useful? React with 👍 / 👎.
| // Tier 1: HTTP Status validation | ||
| const contentType = response.headers.get("content-type") || undefined; | ||
| const parsed = parseResponse(config.providerType, responseBody, contentType); | ||
| const validationInput = parsed.content || responseBody; |
There was a problem hiding this comment.
validationInput currently prefers parsed.content, but parser fallback paths return only a 500-character preview when JSON parsing fails. That means long plain-text/malformed JSON responses can contain the success marker after char 500 and still be reported as content_mismatch, producing false negatives and unnecessary template retries even when the upstream response is actually valid.
Useful? React with 👍 / 👎.
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
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/forwarder.ts (1)
3001-3029:⚠️ Potential issue | 🔴 CriticalHedge 路径会泄漏 agent 的引用计数。
这里把
releaseAgent只挂到了 attempt session 上，默认等 response-handler 去调用；但 hedge 的 loser、首块读取失败分支都在sendStreamingWithHedge()内部消费响应，根本不会经过 response-handler。影子 session 获胜时，syncWinningAttemptSession()也没有把这个回调同步回主 session。结果是每次 hedge 都可能把activeRequests留高，TTL 清理和池容量判断都会失真。🤖 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 3001 - 3029, The hedge path leaks agent reference counts because releaseAgent is only attached to the attempt session (sessionWithTimeout) and relied on the response-handler, while sendStreamingWithHedge() consumes loser/failed attempt responses internally (and syncWinningAttemptSession() doesn't copy the callback back to the main session); fix by ensuring the agent release callback is registered on every session that may hold the agent: attach the same releaseAgent function to the primary session object and to any shadow/attempt sessions created by sendStreamingWithHedge(), and update syncWinningAttemptSession(session, winningAttempt) to copy releaseAgent (and responseController/clearResponseTimeout if relevant) from the winning attempt to the main session; use the existing symbols releaseAgent, sessionWithTimeout, sendStreamingWithHedge, syncWinningAttemptSession, getGlobalAgentPool, agentCacheKeyToRelease and agentDispatcherIdToRelease to locate where to add the synchronization so all code paths (hedge losers, failures, and the winning sync) call pool.releaseAgent exactly once.
🤖 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/`[locale]/settings/providers/_components/allowed-model-rule-editor.tsx:
- Line 42: The UI currently uses MAX_ALLOWED_MODEL_RULES (set from
PROVIDER_RULE_LIMITS.MAX_ITEMS) in AllowedModelRuleEditor and renders the entire
rules table (around lines ~360-503), which will freeze the browser at tens of
thousands of items; change the approach by splitting the backend storage limit
from the UI interaction limit: introduce a separate UI cap (e.g.,
MAX_ALLOWED_MODEL_RULES_UI) used by AllowedModelRuleEditor and any functions
that render or iterate the full rule set, keep PROVIDER_RULE_LIMITS.MAX_ITEMS as
the storage ceiling, and do NOT increase the UI cap until you implement
pagination or a virtualized list for the rules table (or server-side pagination)
so additions/edits/removals operate on a paged/virtualized subset rather than
scanning the entire array.
In `@src/app/`[locale]/settings/providers/_components/forms/api-test-button.tsx:
- Around line 9-12: 将 testProviderGemini 的返回结构对齐到与 testProviderUnified 相同：不要把
details 限制为“仅成功时保留”，在失败分支也要回传 details.error 和 details.rawResponse（保持与
ProviderApiTestSuccessDetails/返回结构一致）；同时计算最终 status 时除了 fallback 还要考虑
validationDetails.latencyPassed（若 latencyPassed 为 false 则应把状态降为 "yellow" 而不是保持
"green"）；检查并调整 testProviderGemini 的类型签名/返回类型以允许在失败时包含 details，确保调用方（UI 分支）能获取到原始
body、error、rawResponse 和 validationDetails 字段。
In `@src/app/v1/_lib/proxy/response-handler.ts`:
- Around line 43-57: The early-return branches in handleNonStream (the !provider
path) and handleStream (the !messageContext || !provider || !response.body
paths) skip releasing the session agent, leaving activeRequests held; before
each of those early returns call releaseSessionAgent(session) to decrement/clear
the pool reference and also clear any per-session state (same behavior as the
finally-backed path), ensuring releaseSessionAgent is invoked wherever you
return early; apply the same change at the other similar spots noted (around the
other occurrences referenced).
In `@src/lib/constants/provider.constants.ts`:
- Around line 19-23: The constant PROVIDER_RULE_LIMITS currently sets MAX_ITEMS
= 100_000 which mismatches the front-end editor (see model-redirect-editor.tsx
where a direct map at line ~232 renders the full list) and will cause UI
freezes; update the design by splitting the limit into separate backend and
frontend caps (e.g., BACKEND_MAX_ITEMS and FRONTEND_SAFE_MAX_ITEMS) and use
FRONTEND_SAFE_MAX_ITEMS where the editor rendering logic
(model-redirect-editor.tsx map) relies on full-list rendering, or alternatively
implement virtualization/virtual-scrolling in model-redirect-editor.tsx to
safely handle large lists; ensure all references to
PROVIDER_RULE_LIMITS.MAX_ITEMS that affect UI are replaced to use the new
FRONTEND_SAFE_MAX_ITEMS and keep the larger backend cap for storage/validation.
In `@src/lib/error-rule-detector.ts`:
- Around line 195-214: The failure branch currently clears isInitialized and
dbLoadedSuccessfully before calling reload(), causing ensureInitialized() to hit
the DB again on every request if the event-driven reload fails; change the logic
so you either (A) do not clear isInitialized/dbLoadedSuccessfully before calling
reload() from the event handler (leave the old usable cache/state in place until
reload succeeds), or (B) if you must clear them up-front, restore the previous
values when the catch branch runs (i.e., revert isInitialized and
dbLoadedSuccessfully to their prior values) and still set
lastReloadTime/reloadRequestedWhileLoading as needed; alternatively implement a
separate stale flag (e.g., isStale) to mark expiration without touching
isInitialized/dbLoadedSuccessfully so ensureInitialized() won’t retry DB loads
on every request.
- Around line 172-179: The current reload() treats callers that simply call
reload() as if they wanted to join the in-flight reload (returning
activeReloadPromise), which downgrades an explicit reload into a no-op and can
leave old rules in memory; change reload so its default behavior is to queue
another run when a reload is already in progress (i.e. treat
options.queueIfRunning === undefined as true), by setting the default sematics
in reload() to queue (and keep the existing reloadRequestedWhileLoading flag
behavior), and update callers that truly want to only join the current pass
(such as ensureInitialized()) to call reload({ queueIfRunning: false }) so they
retain the join-only behavior; refer to reload, activeReloadPromise,
reloadRequestedWhileLoading and ensureInitialized to locate and adjust call
sites and the method signature accordingly.
In `@src/lib/provider-testing/test-service.test.ts`:
- Around line 12-19: Replace the direct global assignment of fetch in beforeEach
with vitest's stub mechanism: instead of setting global.fetch = fetchMock in the
beforeEach block, call vi.stubGlobal("fetch", fetchMock) so vitest tracks and
can restore it via vi.unstubAllGlobals() in afterEach; keep vi.clearAllMocks()
in beforeEach and leave afterEach calling vi.unstubAllGlobals() unchanged, and
update references to fetchMock, beforeEach, afterEach, vi.clearAllMocks,
vi.stubGlobal, and vi.unstubAllGlobals accordingly.
In `@src/lib/provider-testing/test-service.ts`:
- Around line 195-205: The current code calls parseResponse(...) before
classifyHttpStatus(...), so parse errors can bubble out and turn valid 4xx/5xx
HTTP results into network errors; change this so the original HTTP result is
preserved and parsing is best-effort: first compute httpResult =
classifyHttpStatus(response.status, latencyMs, slowThresholdMs) using the raw
response/responseBody, then attempt parseResponse(config.providerType,
responseBody, contentType) inside a try/catch; on parse failure fall back to
using the raw responseBody (e.g. set parsed = { content: responseBody } or
similar) so evaluateContentValidation(httpResult.status, httpResult.subStatus,
validationInput, plan.successContains) always runs with a valid httpResult and
non-throwing parsing logic (refer to parseResponse, classifyHttpStatus,
evaluateContentValidation, responseBody, validationInput).
- Around line 65-72: The code currently checks only that
getPreset(config.preset) returns a value but does not verify the preset's
providerType; update the block that assigns presets (the code using presets,
config.preset and getPreset) to also assert that preset.providerType ===
config.providerType (or equivalent field on the returned preset) and throw a
clear error like `Preset X does not match providerType Y` if it doesn't match,
so we fail fast when a caller passes a preset for a different provider type.
In `@tests/unit/actions/providers-api-test.test.ts`:
- Around line 93-105: The beforeEach replaces global.fetch with fetchMock (in
the beforeEach block) but there is no afterEach to restore the original
global.fetch which can leak into other tests; add an afterEach that restores the
global.fetch (either call vi.restoreAllMocks() or explicitly save the original
fetch before overwriting and reassign it in afterEach), or replace the manual
assignment with vi.stubGlobal('fetch', fetchMock) in the beforeEach so the test
runner will automatically clean it up; update the beforeEach/afterEach
surrounding the global.fetch mock accordingly.
In `@tests/unit/lib/proxy-agent/agent-pool.test.ts`:
- Around line 529-532: Remove the emoji from the added test comments (replace "⭐
回归用例：" and the other occurrence at the later comment) so they comply with the
"Never use emoji" rule; edit the comment lines surrounding vi.useRealTimers()
and the similar comment around lines 599-600 to use plain-text wording (e.g.,
"Regression test: simulate concurrent requests during initial creation") while
preserving the existing explanation about using vi.useRealTimers() and spying to
block createAgent and asserting activeRequests increments; keep other logic
unchanged.
- Around line 532-595: The test creates concurrentPool under real timers but
only calls concurrentPool.shutdown() and createSpy.mockRestore() on the
successful path; move cleanup into the finally block so shutdown and spy restore
always run: ensure the finally block calls createSpy?.mockRestore() (guarding if
spy variable is null) and awaits concurrentPool?.shutdown() (guarding if pool
was created) before switching back to vi.useFakeTimers(), so no real timers or
background tasks leak to other tests; reference the variables concurrentPool,
createSpy, releaseCreate and the methods getAgent, releaseAgent, getPoolStats,
and shutdown to locate where to perform the cleanup.
---
Outside diff comments:
In `@src/app/v1/_lib/proxy/forwarder.ts`:
- Around line 3001-3029: The hedge path leaks agent reference counts because
releaseAgent is only attached to the attempt session (sessionWithTimeout) and
relied on the response-handler, while sendStreamingWithHedge() consumes
loser/failed attempt responses internally (and syncWinningAttemptSession()
doesn't copy the callback back to the main session); fix by ensuring the agent
release callback is registered on every session that may hold the agent: attach
the same releaseAgent function to the primary session object and to any
shadow/attempt sessions created by sendStreamingWithHedge(), and update
syncWinningAttemptSession(session, winningAttempt) to copy releaseAgent (and
responseController/clearResponseTimeout if relevant) from the winning attempt to
the main session; use the existing symbols releaseAgent, sessionWithTimeout,
sendStreamingWithHedge, syncWinningAttemptSession, getGlobalAgentPool,
agentCacheKeyToRelease and agentDispatcherIdToRelease to locate where to add the
synchronization so all code paths (hedge losers, failures, and the winning sync)
call pool.releaseAgent exactly once.
🪄 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: 456b2bd9-b2d8-448b-9bd4-9a1f07ab83bb
📒 Files selected for processing (52)
messages/en/settings/providers/form/allowedModelRules.jsonmessages/en/settings/providers/form/modelRedirect.jsonmessages/ja/settings/providers/form/allowedModelRules.jsonmessages/ja/settings/providers/form/modelRedirect.jsonmessages/ru/settings/providers/form/allowedModelRules.jsonmessages/ru/settings/providers/form/modelRedirect.jsonmessages/zh-CN/settings/providers/form/allowedModelRules.jsonmessages/zh-CN/settings/providers/form/modelRedirect.jsonmessages/zh-TW/settings/providers/form/allowedModelRules.jsonmessages/zh-TW/settings/providers/form/modelRedirect.jsonsrc/actions/providers.tssrc/app/[locale]/settings/providers/_components/allowed-model-rule-editor.tsxsrc/app/[locale]/settings/providers/_components/forms/api-test-button.tsxsrc/app/[locale]/settings/providers/_components/model-multi-select.tsxsrc/app/[locale]/settings/providers/_components/model-redirect-editor.tsxsrc/app/v1/_lib/proxy/errors.tssrc/app/v1/_lib/proxy/forwarder.tssrc/app/v1/_lib/proxy/response-handler.tssrc/lib/constants/provider.constants.tssrc/lib/error-rule-detector.tssrc/lib/provider-allowed-model-schema.tssrc/lib/provider-model-redirect-schema.tssrc/lib/provider-testing/data/cc_beta_cli.jsonsrc/lib/provider-testing/data/cc_haiku_basic.jsonsrc/lib/provider-testing/data/cx_codex_basic.jsonsrc/lib/provider-testing/data/cx_gpt_basic.jsonsrc/lib/provider-testing/data/gm_flash_basic.jsonsrc/lib/provider-testing/data/gm_pro_basic.jsonsrc/lib/provider-testing/data/oa_chat_basic.jsonsrc/lib/provider-testing/data/oa_chat_stream.jsonsrc/lib/provider-testing/presets.test.tssrc/lib/provider-testing/presets.tssrc/lib/provider-testing/test-service.test.tssrc/lib/provider-testing/test-service.tssrc/lib/provider-testing/types.tssrc/lib/provider-testing/utils/test-prompts.tssrc/lib/proxy-agent.tssrc/lib/proxy-agent/agent-pool.tssrc/lib/request-filter-engine.tstests/unit/actions/providers-api-test.test.tstests/unit/actions/providers-patch-contract.test.tstests/unit/lib/error-rule-detector-reload-queue.test.tstests/unit/lib/provider-allowed-model-schema.test.tstests/unit/lib/provider-model-redirect-schema.test.tstests/unit/lib/proxy-agent/agent-pool.test.tstests/unit/proxy/proxy-forwarder-hedge-first-byte.test.tstests/unit/proxy/proxy-forwarder-retry-limit.test.tstests/unit/settings/providers/allowed-model-rule-editor.test.tsxtests/unit/settings/providers/api-test-button.test.tsxtests/unit/settings/providers/model-multi-select.test.tsxtests/unit/settings/providers/model-redirect-editor.test.tsxtests/unit/transport-error-detection.test.ts
💤 Files with no reviewable changes (1)
- src/lib/request-filter-engine.ts
| pattern: "", | ||
| }; | ||
| const MAX_ALLOWED_MODEL_RULES = 100; | ||
| const MAX_ALLOWED_MODEL_RULES = PROVIDER_RULE_LIMITS.MAX_ITEMS; |
There was a problem hiding this comment.
把前端上限直接放到 100_000 会让编辑器失去可用性。
Line 42 现在跟随后端常量放开到 100_000，但下面 Lines 360-503 仍然会一次性渲染整张规则表，增删改也都依赖整表线性扫描。实际加载几万到 10 万条规则时，这个页面基本会卡死，用户很容易保存出之后再也打不开、编辑不了的配置。建议把“存储上限”和“交互上限”拆开，至少补分页或虚拟列表后再放开 UI 上限。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/app/`[locale]/settings/providers/_components/allowed-model-rule-editor.tsx
at line 42, The UI currently uses MAX_ALLOWED_MODEL_RULES (set from
PROVIDER_RULE_LIMITS.MAX_ITEMS) in AllowedModelRuleEditor and renders the entire
rules table (around lines ~360-503), which will freeze the browser at tens of
thousands of items; change the approach by splitting the backend storage limit
from the UI interaction limit: introduce a separate UI cap (e.g.,
MAX_ALLOWED_MODEL_RULES_UI) used by AllowedModelRuleEditor and any functions
that render or iterate the full rule set, keep PROVIDER_RULE_LIMITS.MAX_ITEMS as
the storage ceiling, and do NOT increase the UI cap until you implement
pagination or a virtualized list for the rules table (or server-side pagination)
so additions/edits/removals operate on a paged/virtualized subset rather than
scanning the entire array.
| type ProviderApiTestSuccessDetails, | ||
| testProviderGemini, | ||
| testProviderUnified, | ||
| } from "@/actions/providers"; |
There was a problem hiding this comment.
让 Gemini 分支返回与统一测试一致的结果结构。
Line 189 这里把 details 限制成“仅成功时才保留”，所以失败场景下服务端已经返回的 details.error 和 details.rawResponse 会被直接丢掉；同时 Line 240 只根据 fallback 决定 status，Line 257 的延迟校验失败不会把结果降成 yellow。这样会同时出现“失败时前端拿不到原始 body”和“状态是 green 但 validationDetails.latencyPassed 为 false”的矛盾结果。
参考修改
import {
getUnmaskedProviderKey,
+ type ProviderApiTestFailureDetails,
type ProviderApiTestSuccessDetails,
testProviderGemini,
testProviderUnified,
} from "@/actions/providers";
@@
- const isSuccess = response.data.success === true;
- const details = (isSuccess ? response.data.details : undefined) as
- | ProviderApiTestSuccessDetails
- | undefined;
+ const isSuccess = response.data.success === true;
+ const details = response.data.details;
+ const successDetails = isSuccess
+ ? (details as ProviderApiTestSuccessDetails | undefined)
+ : undefined;
+ const failureDetails = !isSuccess
+ ? (details as ProviderApiTestFailureDetails | undefined)
+ : undefined;
const latencyMs = response.data.details?.responseTime ?? 0;
+ const latencyPassed = isSuccess && latencyMs <= 5000;
@@
testResultData = {
success: isSuccess,
- status: isSuccess ? (usedFallback ? "yellow" : "green") : "red",
+ status: !isSuccess ? "red" : usedFallback || !latencyPassed ? "yellow" : "green",
subStatus: inferSubStatus(),
message: cleanMessage,
latencyMs,
- model: details?.model,
- content: details?.content,
+ model: successDetails?.model,
+ content: successDetails?.content,
rawResponse: details?.rawResponse,
- usage: normalizeUsage(details?.usage),
- streamInfo: details?.streamInfo
+ usage: normalizeUsage(successDetails?.usage),
+ streamInfo: successDetails?.streamInfo
? {
isStreaming: true,
- chunksReceived: details.streamInfo.chunksReceived,
+ chunksReceived: successDetails.streamInfo.chunksReceived,
}
: undefined,
+ errorMessage: failureDetails?.error,
testedAt: new Date().toISOString(),
validationDetails: {
httpPassed: isSuccess,
- latencyPassed: isSuccess && latencyMs < 5000,
+ latencyPassed,
latencyMs,
contentPassed: isSuccess,
contentTarget: "pong",Also applies to: 188-261
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/`[locale]/settings/providers/_components/forms/api-test-button.tsx
around lines 9 - 12, 将 testProviderGemini 的返回结构对齐到与 testProviderUnified 相同：不要把
details 限制为“仅成功时保留”，在失败分支也要回传 details.error 和 details.rawResponse（保持与
ProviderApiTestSuccessDetails/返回结构一致）；同时计算最终 status 时除了 fallback 还要考虑
validationDetails.latencyPassed（若 latencyPassed 为 false 则应把状态降为 "yellow" 而不是保持
"green"）；检查并调整 testProviderGemini 的类型签名/返回类型以允许在失败时包含 details，确保调用方（UI 分支）能获取到原始
body、error、rawResponse 和 validationDetails 字段。
| export const PROVIDER_RULE_LIMITS = { | ||
| // 模型白名单 / 重定向规则保存在 JSONB 中，这里统一放宽到支持大规模路由表 | ||
| MAX_ITEMS: 100_000, | ||
| MAX_TEXT_LENGTH: 4_096, | ||
| } as const; |
There was a problem hiding this comment.
MAX_ITEMS = 100_000 与当前前端编辑器渲染模型不匹配，会产生明显可用性风险。
Line 21 将规则数上限放大到 100k，但当前编辑器仍是全量列表渲染与客户端内存处理（如 model-redirect-editor.tsx Line 232 的直接 map）。在大规则集下会导致页面卡顿甚至不可操作。建议拆分“后端硬上限”和“前端安全编辑上限”，或在放宽上限前先引入虚拟化。
可选修正方向（示例）
export const PROVIDER_RULE_LIMITS = {
- // 模型白名单 / 重定向规则保存在 JSONB 中，这里统一放宽到支持大规模路由表
- MAX_ITEMS: 100_000,
+ // 存储/接口硬上限
+ MAX_ITEMS_HARD: 100_000,
+ // 前端编辑器安全上限（避免一次渲染超大列表）
+ MAX_ITEMS_UI: 2_000,
MAX_TEXT_LENGTH: 4_096,
} as const;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/constants/provider.constants.ts` around lines 19 - 23, The constant
PROVIDER_RULE_LIMITS currently sets MAX_ITEMS = 100_000 which mismatches the
front-end editor (see model-redirect-editor.tsx where a direct map at line ~232
renders the full list) and will cause UI freezes; update the design by splitting
the limit into separate backend and frontend caps (e.g., BACKEND_MAX_ITEMS and
FRONTEND_SAFE_MAX_ITEMS) and use FRONTEND_SAFE_MAX_ITEMS where the editor
rendering logic (model-redirect-editor.tsx map) relies on full-list rendering,
or alternatively implement virtualization/virtual-scrolling in
model-redirect-editor.tsx to safely handle large lists; ensure all references to
PROVIDER_RULE_LIMITS.MAX_ITEMS that affect UI are replaced to use the new
FRONTEND_SAFE_MAX_ITEMS and keep the larger backend cap for storage/validation.
| async reload(options: { queueIfRunning?: boolean } = {}): Promise<void> { | ||
| if (this.activeReloadPromise) { | ||
| if (options.queueIfRunning) { | ||
| this.reloadRequestedWhileLoading = true; | ||
| logger.info("[ErrorRuleDetector] Reload already in progress, queueing another pass"); | ||
| } | ||
| return this.activeReloadPromise; | ||
| } |
There was a problem hiding this comment.
reload() 在并发场景下会把“显式刷新”降级成“仅等待当前刷新”。
如果调用方是在写库之后再 await errorRuleDetector.reload()，而当前 in-flight pass 已经读过旧快照，这里直接返回 activeReloadPromise 不会补跑下一轮，最终仍可能把旧规则留在内存里。建议把显式 reload() 的默认语义改成排队补跑，只给 ensureInitialized() 这类普通等待者保留“仅 join 当前 reload”的行为。
一个最小改法
- async reload(options: { queueIfRunning?: boolean } = {}): Promise<void> {
+ async reload(options: { queueIfRunning?: boolean } = { queueIfRunning: true }): Promise<void> {
+ const queueIfRunning = options.queueIfRunning ?? true;
if (this.activeReloadPromise) {
- if (options.queueIfRunning) {
+ if (queueIfRunning) {
this.reloadRequestedWhileLoading = true;
logger.info("[ErrorRuleDetector] Reload already in progress, queueing another pass");
}
return this.activeReloadPromise;
}如果某些调用点确实只想复用当前 reload，再显式传 queueIfRunning: false。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/error-rule-detector.ts` around lines 172 - 179, The current reload()
treats callers that simply call reload() as if they wanted to join the in-flight
reload (returning activeReloadPromise), which downgrades an explicit reload into
a no-op and can leave old rules in memory; change reload so its default behavior
is to queue another run when a reload is already in progress (i.e. treat
options.queueIfRunning === undefined as true), by setting the default sematics
in reload() to queue (and keep the existing reloadRequestedWhileLoading flag
behavior), and update callers that truly want to only join the current pass
(such as ensureInitialized()) to call reload({ queueIfRunning: false }) so they
retain the join-only behavior; refer to reload, activeReloadPromise,
reloadRequestedWhileLoading and ensureInitialized to locate and adjust call
sites and the method signature accordingly.
| let presets: PresetConfig[]; | ||
| if (config.preset) { | ||
| const preset = getPreset(config.preset); | ||
| if (!preset) { | ||
| throw new Error(`Preset not found: ${config.preset}`); | ||
| } | ||
| presets = [preset]; | ||
| } else { |
There was a problem hiding this comment.
拒绝与 providerType 不匹配的显式 preset。
Line 67-71 现在只校验 preset 是否存在，没有校验它是否属于当前 providerType。如果调用方传入 providerType: "codex" + preset: "gm_pro_basic"，这里会把 Gemini 的 path/body 和 Codex 的 headers/parser 混在一起，最后得到非常难诊断的假错误。这里应该直接 fail fast。
参考修改
import {
getExecutionPresetCandidates,
getPreset,
getPresetPayload,
+ isPresetCompatible,
type PresetConfig,
} from "./presets";
@@
let presets: PresetConfig[];
if (config.preset) {
+ if (!isPresetCompatible(config.preset, config.providerType)) {
+ throw new Error(
+ `Preset ${config.preset} is not compatible with provider type ${config.providerType}`
+ );
+ }
const preset = getPreset(config.preset);
if (!preset) {
throw new Error(`Preset not found: ${config.preset}`);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/provider-testing/test-service.ts` around lines 65 - 72, The code
currently checks only that getPreset(config.preset) returns a value but does not
verify the preset's providerType; update the block that assigns presets (the
code using presets, config.preset and getPreset) to also assert that
preset.providerType === config.providerType (or equivalent field on the returned
preset) and throw a clear error like `Preset X does not match providerType Y` if
it doesn't match, so we fail fast when a caller passes a preset for a different
provider type.
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| getSessionMock.mockResolvedValue({ user: { id: 1, role: "admin" } }); | ||
| validateProviderUrlForConnectivityMock.mockImplementation((providerUrl: string) => ({ | ||
| valid: true, | ||
| normalizedUrl: providerUrl, | ||
| })); | ||
| createProxyAgentForProviderMock.mockReturnValue(null); | ||
| getAccessTokenMock.mockImplementation(async (apiKey: string) => apiKey); | ||
| isJsonMock.mockReturnValue(false); | ||
| getPresetsForProviderMock.mockReturnValue([]); | ||
| global.fetch = fetchMock as typeof fetch; | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's get the full test file to see the complete structure
fd -t f "providers-api-test.test.ts" --exec cat -n {} \;Repository: ding113/claude-code-hub
Length of output: 6661
在 afterEach 中恢复 global.fetch，避免测试间污染。
第 104 行直接覆写了 global.fetch，但该文件缺少对应的 afterEach 钩子来恢复原始值。vi.clearAllMocks() 只清空 mock 调用历史，不会还原全局对象的直接赋值，可能导致后续测试继承这里的 mock，引发不稳定的结果。建议添加 afterEach(() => { vi.restoreAllMocks(); }) 或改用 vi.stubGlobal('fetch', fetchMock) 自动处理清理。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/actions/providers-api-test.test.ts` around lines 93 - 105, The
beforeEach replaces global.fetch with fetchMock (in the beforeEach block) but
there is no afterEach to restore the original global.fetch which can leak into
other tests; add an afterEach that restores the global.fetch (either call
vi.restoreAllMocks() or explicitly save the original fetch before overwriting
and reassign it in afterEach), or replace the manual assignment with
vi.stubGlobal('fetch', fetchMock) in the beforeEach so the test runner will
automatically clean it up; update the beforeEach/afterEach surrounding the
global.fetch mock accordingly.
| vi.useRealTimers(); | ||
| try { | ||
| const concurrentPool = new AgentPoolImpl({ | ||
| ...defaultConfig, | ||
| agentTtlMs: 60_000, | ||
| }); | ||
|
|
||
| const params = { | ||
| endpointUrl: "https://api.anthropic.com/v1/messages", | ||
| proxyUrl: null, | ||
| enableHttp2: false, | ||
| }; | ||
|
|
||
| // 用 deferred 控制 createAgent 的完成时机，保证后续 getAgent 都落入 pendingCreations | ||
| let releaseCreate: (() => void) | null = null; | ||
| const createBlocker = new Promise<void>((resolve) => { | ||
| releaseCreate = resolve; | ||
| }); | ||
|
|
||
| // biome-ignore lint/suspicious/noExplicitAny: private 方法 spy | ||
| const createSpy = vi.spyOn(concurrentPool as any, "createAgent"); | ||
| createSpy.mockImplementationOnce(async () => { | ||
| await createBlocker; | ||
| return { | ||
| close: vi.fn().mockResolvedValue(undefined), | ||
| destroy: vi.fn().mockResolvedValue(undefined), | ||
| options: {}, | ||
| }; | ||
| }); | ||
|
|
||
| // 同时发起 3 个请求：首个进入 createAgentWithCache；后两个必须走 pendingCreations | ||
| const p1 = concurrentPool.getAgent(params); | ||
| const p2 = concurrentPool.getAgent(params); | ||
| const p3 = concurrentPool.getAgent(params); | ||
|
|
||
| // 稍微等一个 microtask，确保 p2/p3 已经进入 await pending 分支 | ||
| await Promise.resolve(); | ||
| await Promise.resolve(); | ||
|
|
||
| // 放行创建 | ||
| releaseCreate?.(); | ||
|
|
||
| const [r1, r2, r3] = await Promise.all([p1, p2, p3]); | ||
|
|
||
| // 三个调用应拿到相同的 cacheKey 与 dispatcherId | ||
| expect(r2.cacheKey).toBe(r1.cacheKey); | ||
| expect(r3.cacheKey).toBe(r1.cacheKey); | ||
| expect(r2.dispatcherId).toBe(r1.dispatcherId); | ||
| expect(r3.dispatcherId).toBe(r1.dispatcherId); | ||
|
|
||
| // 关键断言：所有 3 个并发 waiter 都应计入 activeRequests | ||
| expect(concurrentPool.getPoolStats().activeRequests).toBe(3); | ||
|
|
||
| // 释放 3 次后归零，且仍能再多释一次（no-op） | ||
| concurrentPool.releaseAgent(r1.cacheKey, r1.dispatcherId); | ||
| concurrentPool.releaseAgent(r2.cacheKey, r2.dispatcherId); | ||
| concurrentPool.releaseAgent(r3.cacheKey, r3.dispatcherId); | ||
| expect(concurrentPool.getPoolStats().activeRequests).toBe(0); | ||
|
|
||
| createSpy.mockRestore(); | ||
| await concurrentPool.shutdown(); | ||
| } finally { | ||
| vi.useFakeTimers(); | ||
| } |
There was a problem hiding this comment.
把额外 pool 的清理移到 finally。
这个用例在 vi.useRealTimers() 下新建了 concurrentPool，但 shutdown() 只会在 try 正常走到底时执行。前面任一断言失败都会跳过清理，把真实计时器/后台任务留给后续用例。
建议修复
vi.useRealTimers();
+ let concurrentPool: AgentPoolImpl | null = null;
+ let createSpy: { mockRestore: () => void } | null = null;
try {
- const concurrentPool = new AgentPoolImpl({
+ concurrentPool = new AgentPoolImpl({
...defaultConfig,
agentTtlMs: 60_000,
});
@@
- const createSpy = vi.spyOn(concurrentPool as any, "createAgent");
+ createSpy = vi.spyOn(concurrentPool as any, "createAgent");
@@
- createSpy.mockRestore();
- await concurrentPool.shutdown();
} finally {
+ createSpy?.mockRestore();
+ await concurrentPool?.shutdown();
vi.useFakeTimers();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/lib/proxy-agent/agent-pool.test.ts` around lines 532 - 595, The
test creates concurrentPool under real timers but only calls
concurrentPool.shutdown() and createSpy.mockRestore() on the successful path;
move cleanup into the finally block so shutdown and spy restore always run:
ensure the finally block calls createSpy?.mockRestore() (guarding if spy
variable is null) and awaits concurrentPool?.shutdown() (guarding if pool was
created) before switching back to vi.useFakeTimers(), so no real timers or
background tasks leak to other tests; reference the variables concurrentPool,
createSpy, releaseCreate and the methods getAgent, releaseAgent, getPoolStats,
and shutdown to locate where to perform the cleanup.
![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
No significant issues identified in this PR. This is a well-structured XL release PR consolidating five features with comprehensive test coverage. The core changes -- agent pool reference counting with generation-token protection, ErrorRuleDetector reload queue coalescing, provider testing preset overhaul with automatic template fallback, and case-sensitive model ID handling -- are logically sound and well-tested.
PR Size: XL
- Lines changed: 4218 (3220 additions + 998 deletions)
- Files changed: 53
Split suggestion (XL): This PR bundles 5 distinct features. While each feature is coherent internally, future releases could benefit from merging features individually to simplify bisection. The current structure is acceptable for a release branch merge.
Review Coverage
- Logic and correctness - Clean. Reload queue do-while loop correctly handles DB failure circuit-breaking. Agent pool
dispatcherIdgeneration tokens prevent cross-generation ref-count corruption. Template fallback retries on 400/404/405/415/422 are appropriate. - Security (OWASP Top 10) - Clean. No injection vectors, credentials handled properly (API keys passed via headers, not logged).
rawResponsepassthrough is intentional per PR description. - Error handling - Clean. All new catch blocks have appropriate logging or documented rationale.
releaseSessionAgentidempotent design prevents double-release. DB failure during reload queue breaks the loop to avoid hot-retry storms. - Type safety - Acceptable. The
releaseAgentcallback attached via type cast onProxySessionis repeated in forwarder.ts and response-handler.ts; adding it to theProxySessioninterface would be cleaner but this is a design preference, not a defect (already noted by Greptile). - Documentation accuracy - Clean. Comments match code behavior. Chinese comments are consistent with codebase conventions.
- Test coverage - Excellent. 1000+ lines of new tests covering: reload queue race conditions (5 scenarios including microtask-level timing), agent pool reference counting (10 scenarios including concurrent waiters, cross-generation release, hard upper bound), provider testing presets and template fallback, schema validation with new limits, and UI component updates.
- Code clarity - Good. Extracted helpers (
buildMatchedRuleDetails,buildMatchedRuleLogContext,buildClientErrorChainEntry) reduce duplication in forwarder.ts. ApiTestButton simplification removes significant UI complexity.
Automated review by Claude AI
| if (s.releaseAgent) { | ||
| try { | ||
| s.releaseAgent(); | ||
| } catch { |
There was a problem hiding this comment.
[CRITICAL] [ERROR-SILENT] Swallowed exception in releaseSessionAgent hides agent release failures
Evidence: src/app/v1/_lib/proxy/response-handler.ts:52
} catch {
// ignore - agent may already be evicted
}Why this is a problem: If session.releaseAgent() throws (unexpected agent-pool state, bugs in the callback, etc.), the exception is silently dropped. That can mask refcount leaks and makes production failures much harder to diagnose.
Suggested fix:
function releaseSessionAgent(session: ProxySession): void {
const s = session as ProxySession & { releaseAgent?: () => void };
if (!s.releaseAgent) return;
try {
s.releaseAgent();
} catch (error) {
logger.debug("ProxyResponseHandler: releaseAgent failed", {
error: error instanceof Error ? error.message : String(error),
});
} finally {
s.releaseAgent = undefined;
}
}
| if (now - cached.createdAt > MAX_AGENT_LIFETIME_MS) return true; | ||
|
|
||
| // Never expire agents with in-flight requests | ||
| if (cached.activeRequests > 0) return false; |
There was a problem hiding this comment.
[HIGH] [LOGIC-BUG] LRU eviction can still destroy in-flight dispatchers despite activeRequests
Evidence: src/lib/proxy-agent/agent-pool.ts:437
// Never expire agents with in-flight requests
if (cached.activeRequests > 0) return false;Why this is a problem: enforceMaxSize() evicts solely by lastUsedAt and calls closeAgent() even when activeRequests > 0. If the pool exceeds maxTotalAgents during a long-lived stream (or any slow request), an in-flight dispatcher can be destroyed, causing stream truncation / request failures.
Suggested fix (skip in-flight entries during LRU eviction):
private async enforceMaxSize(): Promise<void> {
if (this.cache.size <= this.config.maxTotalAgents) return;
const needed = this.cache.size - this.config.maxTotalAgents;
const evictable = Array.from(this.cache.entries())
.filter(([, cached]) => cached.activeRequests === 0)
.sort(([, a], [, b]) => a.lastUsedAt - b.lastUsedAt);
for (const [key] of evictable.slice(0, needed)) {
await this.evictByKey(key);
}
// Optional: if still over limit because all agents are active, defer eviction until releases occur.
}
| let response: Response; | ||
| const fetchStartTime = Date.now(); | ||
| try { | ||
| // ⭐ 把 agent 获取 & 配置日志放进 try 块，确保获取后到 fetch 之前任何异常 |
There was a problem hiding this comment.
[MEDIUM] [STANDARD-VIOLATION] Emoji characters added in code comments
Evidence:
src/app/v1/_lib/proxy/forwarder.ts:2503// ⭐ 把 agent 获取 & 配置日志放进 try 块，确保获取后到 fetch 之前任何异常src/app/v1/_lib/proxy/forwarder.ts:2861// ⚠️ 关键：catch 入口已经释放了代理 agent 的 ref count（见本 catch 块顶部），
Why this is a problem: CLAUDE.md prohibits emoji characters in code, including comments: "No Emoji in Code - Never use emoji characters in any code, comments, or string literals".
Suggested fix (remove emojis, keep the intent):
// IMPORTANT: Put agent acquisition and logging inside the try so pre-fetch failures use unified cleanup.
// IMPORTANT: After proxy->direct fallback, clear proxy tracking to avoid double releaseAgent().
| if (pending) { | ||
| // Wait for the pending creation and return its result | ||
| const result = await pending; | ||
| // ⚠️ 关键：等待 pending 创建的调用者也必须计入 activeRequests。 |
There was a problem hiding this comment.
[MEDIUM] [STANDARD-VIOLATION] Emoji character in comment violates repository rule
Evidence: src/lib/proxy-agent/agent-pool.ts:247
// ⚠️ 关键：等待 pending 创建的调用者也必须计入 activeRequests。Why this is a problem: CLAUDE.md forbids emoji characters in code comments.
Suggested fix:
// IMPORTANT: Pending-creation waiters must also increment activeRequests.
| latencyPassed: isSuccess && latencyMs < 5000, | ||
| latencyMs, | ||
| contentPassed: isSuccess, | ||
| contentTarget: "pong", |
There was a problem hiding this comment.
[MEDIUM] [LOGIC-BUG] Gemini content validation is hard-coded to "pong" and always marked as passed
Evidence: src/app/[locale]/settings/providers/_components/forms/api-test-button.tsx:259
contentPassed: isSuccess,
contentTarget: "pong",Why this is a problem: This makes the 3-tier validation UI misleading: it reports a content check against "pong" even though the Gemini test request does not enforce that target, and the UI never verifies the returned content.
Suggested fix (make the content tier reflect actual data for Gemini):
const hasContent = Boolean(details?.content?.trim());
validationDetails: {
httpPassed: isSuccess,
latencyPassed: isSuccess && latencyMs < 5000,
latencyMs,
contentPassed: isSuccess && hasContent,
contentTarget: undefined,
},
There was a problem hiding this comment.
Code Review Summary
This is an XL release PR that mixes core proxy runtime changes (agent pooling + forwarding/response handling), provider-testing plumbing, and settings UI/schema updates. The main risks are (1) silent error swallowing in the new agent-release path and (2) a remaining eviction path that can destroy in-flight dispatchers under LRU pressure.
PR Size: XL
- Lines changed: 4182
- Files changed: 52
- Split suggestion (required for XL):
- PR 1: Agent pool refcounting +
ProxyForwarder/ProxyResponseHandlerintegration - PR 2: Provider testing service (
src/lib/provider-testing/*) + actions/UI wiring - PR 3: Provider rule limits/schemas + settings editors + i18n message updates
- PR 1: Agent pool refcounting +
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 1 | 1 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 1 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 2 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
src/app/v1/_lib/proxy/response-handler.ts:52[CRITICAL] [ERROR-SILENT] Emptycatch {}inreleaseSessionAgentswallows exceptions (Confidence: 95)src/lib/proxy-agent/agent-pool.ts:437[HIGH] [LOGIC-BUG]enforceMaxSize()can still evict agents withactiveRequests > 0(Confidence: 95)
High Priority Issues (Should Fix)
src/app/[locale]/settings/providers/_components/forms/api-test-button.tsx:260[MEDIUM] [LOGIC-BUG] Gemini content validation is hard-coded to "pong" and always marked passed (Confidence: 88)src/app/v1/_lib/proxy/forwarder.ts:2503[MEDIUM] [STANDARD-VIOLATION] Emoji characters added in code comments (Confidence: 80)src/lib/proxy-agent/agent-pool.ts:247[MEDIUM] [STANDARD-VIOLATION] Emoji character added in code comment (Confidence: 80)
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Codex AI
There was a problem hiding this comment.
- Identified PR
#1014(release v0.6.8) as XL (4182 lines changed across 52 files) and applied the labelsize/XL. - Posted inline review comments on the diff (silent error swallowing in agent release, in-flight agent eviction risk under LRU, Gemini content-validation reporting, and new emoji-in-comment rule violations).
- Submitted the required “Code Review Summary” as a PR review comment via
gh pr review.
- Use nullish coalescing (??) instead of logical OR (||) for parsed.content
in test-service.ts to preserve empty string as valid content
- Wrap parseResponse() in try-catch to prevent parse exceptions from
masking HTTP status codes as network_error
- Add releaseSessionAgent() to early-return branches in response-handler.ts
to prevent agent pool reference count leaks
- Fix error-rule-detector reload() to always queue a fresh pass when
called during an active reload, preventing stale snapshot reads
- Remove pre-clearing of isInitialized in event handler to avoid
retry storms when database is unavailable
- Use ICU {max, number} format in i18n messages for locale-aware
number formatting of rule limits across all 5 languages
- Remove emoji from test comments and fix pool cleanup in finally block
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/lib/error-rule-detector.ts (1)
102-106:⚠️ Potential issue | 🟠 Major不要在事件回调里预先清掉
dbLoadedSuccessfully。Line 105 仍然会让
ensureInitialized()失去快速返回条件，因为它要求dbLoadedSuccessfully && isInitialized同时为true。一旦事件驱动的reload()失败，旧缓存虽然还可用，但后续请求会再次触发打库，和这里注释里“避免每次请求重试”的目标相反。建议修改
const handleUpdated = () => { // 标记数据可能过期，让 reload 成功后刷新标记。 // 注意：不清 isInitialized，避免 reload 失败时 ensureInitialized() 在每次请求上重试打库。 - this.dbLoadedSuccessfully = false; this.reload({ queueIfRunning: true }).catch((error) => { logger.error("[ErrorRuleDetector] Failed to reload cache on event:", error); }); };如果还需要表达“缓存已过期”，建议单独引入一个 stale 标记，而不是复用
dbLoadedSuccessfully。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/error-rule-detector.ts` around lines 102 - 106, The event callback handleUpdated must not clear dbLoadedSuccessfully because that causes ensureInitialized() to lose its fast-return condition; instead, introduce a separate stale flag (e.g., isStale) and set isStale = true in handleUpdated, leaving dbLoadedSuccessfully untouched; update reload({ queueIfRunning: true }) to clear isStale only on successful completion and ensure ensureInitialized() checks dbLoadedSuccessfully && isInitialized to short-circuit while also considering isStale when deciding whether to reload.
🧹 Nitpick comments (1)
src/lib/provider-testing/test-service.ts (1)
280-288: 超时处理可能超出预期的 timeoutMs。Line 282 使用
Math.max(1000, deadline - Date.now())确保每次尝试至少有 1 秒。但若前几次尝试耗时较长导致 deadline 已过，后续尝试仍会获得 1000ms，使总执行时间可能超出原始timeoutMs。如果这是预期行为（给每个模板公平机会），可忽略此建议。否则可考虑在循环开始时检查是否已超时：
可选优化
for (const plan of plans) { + if (Date.now() >= deadline) { + break; + } const remainingTimeoutMs = Math.max(1000, deadline - Date.now());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/provider-testing/test-service.ts` around lines 280 - 288, The loop uses Math.max(1000, deadline - Date.now()) which can give each plan a full extra 1s even after the global deadline, potentially exceeding the intended timeout; update the loop in test-service.ts that iterates over plans (where fallbackResult, runSingleAttempt, shouldRetryWithNextTemplate and deadline are used) to check remaining time at the start of each iteration (compute remaining = deadline - Date.now()) and if remaining <= 0 return or break and return fallbackResult, otherwise pass the actual remaining (not a forced 1000ms) into runSingleAttempt; if you intentionally want a minimum per-attempt allowance, document that behavior and use a bounded remaining = Math.min(remaining, someMax) but do not unconditionally force a 1000ms when remaining is already expired.
🤖 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/lib/error-rule-detector.ts`:
- Around line 102-106: The event callback handleUpdated must not clear
dbLoadedSuccessfully because that causes ensureInitialized() to lose its
fast-return condition; instead, introduce a separate stale flag (e.g., isStale)
and set isStale = true in handleUpdated, leaving dbLoadedSuccessfully untouched;
update reload({ queueIfRunning: true }) to clear isStale only on successful
completion and ensure ensureInitialized() checks dbLoadedSuccessfully &&
isInitialized to short-circuit while also considering isStale when deciding
whether to reload.
---
Nitpick comments:
In `@src/lib/provider-testing/test-service.ts`:
- Around line 280-288: The loop uses Math.max(1000, deadline - Date.now()) which
can give each plan a full extra 1s even after the global deadline, potentially
exceeding the intended timeout; update the loop in test-service.ts that iterates
over plans (where fallbackResult, runSingleAttempt, shouldRetryWithNextTemplate
and deadline are used) to check remaining time at the start of each iteration
(compute remaining = deadline - Date.now()) and if remaining <= 0 return or
break and return fallbackResult, otherwise pass the actual remaining (not a
forced 1000ms) into runSingleAttempt; if you intentionally want a minimum
per-attempt allowance, document that behavior and use a bounded remaining =
Math.min(remaining, someMax) but do not unconditionally force a 1000ms when
remaining is already expired.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f1c2fa06-931f-48e7-83fa-e268a4815242
📒 Files selected for processing (14)
messages/en/settings/providers/form/allowedModelRules.jsonmessages/en/settings/providers/form/modelRedirect.jsonmessages/ja/settings/providers/form/allowedModelRules.jsonmessages/ja/settings/providers/form/modelRedirect.jsonmessages/ru/settings/providers/form/allowedModelRules.jsonmessages/ru/settings/providers/form/modelRedirect.jsonmessages/zh-CN/settings/providers/form/allowedModelRules.jsonmessages/zh-CN/settings/providers/form/modelRedirect.jsonmessages/zh-TW/settings/providers/form/allowedModelRules.jsonmessages/zh-TW/settings/providers/form/modelRedirect.jsonsrc/app/v1/_lib/proxy/response-handler.tssrc/lib/error-rule-detector.tssrc/lib/provider-testing/test-service.tstests/unit/lib/proxy-agent/agent-pool.test.ts
✅ Files skipped from review due to trivial changes (9)
- messages/en/settings/providers/form/allowedModelRules.json
- messages/en/settings/providers/form/modelRedirect.json
- messages/zh-CN/settings/providers/form/modelRedirect.json
- messages/zh-TW/settings/providers/form/modelRedirect.json
- messages/ru/settings/providers/form/modelRedirect.json
- messages/zh-CN/settings/providers/form/allowedModelRules.json
- messages/zh-TW/settings/providers/form/allowedModelRules.json
- messages/ru/settings/providers/form/allowedModelRules.json
- tests/unit/lib/proxy-agent/agent-pool.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- messages/ja/settings/providers/form/allowedModelRules.json
- messages/ja/settings/providers/form/modelRedirect.json
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a71530214
ℹ️ 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".
| // Never expire agents with in-flight requests | ||
| if (cached.activeRequests > 0) return false; | ||
|
|
||
| return now - cached.lastUsedAt > this.config.agentTtlMs; |
There was a problem hiding this comment.
This new activeRequests guard claims in-flight agents should not expire, but only the TTL path uses it; enforceMaxSize() still evicts purely by LRU and can destroy a dispatcher that is actively serving a stream when the pool exceeds maxTotalAgents. In high-cardinality traffic (many endpoint/proxy combinations), this can still trigger mid-stream transport failures (UND_ERR_DESTROYED/stream processing errors) even though ref-counting was added.
Useful? React with 👍 / 👎.
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
1 participant
Summary
Release v0.6.8 consolidating five merged PRs: relaxed provider routing rule limits, improved provider detection templates, fixed error rule reload race conditions, prevented agent pool premature eviction of streaming connections, and corrected case-sensitive model ID handling.
Changes Included
1. Relax provider routing rule limits (PR #1000)
PROVIDER_RULE_LIMITSconstant{max}interpolation2. Improve provider detection template selection (PR #1003)
oa_chat_basic,oa_chat_stream) instead of reusing Codex Responses templatesgenerateContenttemplates (non-streaming,thinkingBudget: 0)rawResponsepassthroughstreamGenerateContent?alt=ssetogenerateContentfor reliability3. Fix error rule reload race condition (PR #1006)
queueIfRunning)activeReloadPromisecoalescing so concurrent callers await the same reloadensureInitialized()waits for queued reruns to complete4. Prevent STREAM_PROCESSING_ERROR from premature agent pool eviction (PR #1009)
activeRequestsreference counting to the agent pool (getAgentincrements,releaseAgentdecrements)dispatcherIdgeneration token to prevent cross-generation release miscountreleaseAgentcalls throughout forwarder fetch paths and response-handler finally blocksUND_ERR_DESTROYED,UND_ERR_CLOSED,ClientDestroyedError,ClientClosedError, andERR_HTTP2_STREAM_ERRORas transport errors5. Respect case-sensitive provider model IDs (PR #1011)
.toLowerCase()from duplicate detection in allowlist and redirect rule editors/schemasRelated Issues
Breaking Changes
None. All changes are backward-compatible:
Testing
Automated Tests
ErrorRuleDetectorreload queue race conditions (230 lines)AgentPoolreference counting (307 lines)AllowedModelRuleEditorandModelRedirectEditorcase sensitivityApiTestButtonsimplified componentChecklist
Description enhanced by Claude AI
Greptile Summary
This release consolidates five merged PRs: provider routing rule limit relaxation (100 → 100,000), reworked provider detection templates with per-suite fallback retry, error rule reload race-condition fix (queued-reload pattern), agent pool premature eviction prevention via
activeRequestsreference counting withdispatcherIdgeneration tokens, and case-sensitive model ID dedup alignment.The core changes are well-implemented and comprehensively tested (1,200+ new test lines across the five areas).
Confidence Score: 5/5
Safe to merge — all five sub-PRs are backward-compatible and well-tested; no new P0/P1 issues found.
All core changes (reference counting, reload queue, template fallback, case-sensitive dedup, limit relaxation) are correctly implemented with comprehensive test coverage (1,200+ new test lines). The three agent-release code paths in forwarder.ts are properly isolated with no double-release. The error-rule reload do/while queue handles the acknowledged narrow race window. Previous thread concerns are P2 style/documentation items that do not block merge.
No files require special attention. The most complex change is forwarder.ts with its three-path agent release, but the logic is correct and tested.
Important Files Changed
Sequence Diagram
sequenceDiagram participant F as ProxyForwarder participant AP as AgentPool participant RH as ResponseHandler F->>AP: getAgent(endpoint, http2=true) AP-->>F: {agent, cacheKey, dispatcherId} activeRequests++ alt fetch network failure F->>AP: releaseAgent(cacheKey, dispatcherId) Note over AP: activeRequests-- (guard >0) else HTTP 4xx/5xx response F->>AP: releaseAgent in !response.ok finally Note over AP: activeRequests-- (guard >0) else HTTP 200 streaming success F->>RH: response + session.releaseAgent callback Note over F,RH: isExpired returns false while activeRequests > 0 RH->>AP: releaseSessionAgent idempotent clears callback Note over AP: activeRequests-- (guard >0) end Note over AP: cleanup evicts only when activeRequests==0 AND TTL exceeded hard cap 30 minComments Outside Diff (1)
src/lib/proxy-agent/agent-pool.ts, line 504-519 (link)activeRequests, bypassing the streaming-protection fixenforceMaxSizecallsevictByKeydirectly without checkingactiveRequests, so when the pool grows pastmaxTotalAgents(100 by default), the oldest agents — including ones serving live streams — are evicted. This reintroducesSTREAM_PROCESSING_ERRORin any deployment with >100 distinct (endpoint × proxy × protocol) combos, which the rest of this PR was designed to prevent.isExpiredandcleanuprespectactiveRequests > 0, butenforceMaxSizebypasses that guard entirely. The 30-minute hard cap inisExpiredis not applied here either.Prompt To Fix With AI
Reviews (3): Last reviewed commit: "chore: format code (dev-56698a2)" | Re-trigger Greptile