Conversation
* feat(settings): 增加批量测试供应商能力 -【前端】新增批量测试弹窗、并发执行与结果筛选 -【前端】在批量操作中加入“测试”入口与多语言文案 -【后端】新增按供应商 ID 执行测试的接口与 Action -【类型】补充接口与 schema，支持批量测试请求 -【测试】增加动作与 hook 的单元测试覆盖 * fix(settings): 修复批量测试评审与流水线问题 -【流水线】重新生成 OpenAPI 类型，移除本仓库不存在的接口定义 -【前端】关闭弹窗时停止派发新测试，避免后台消耗供应商额度 -【前端】重新测试时重置结果筛选，避免列表显示为空 -【后端】Gemini JSON 凭证先换取 access token 并改用 Bearer 认证 -【测试】provider-manager 测试 mock 批量测试弹窗；补充 Gemini 凭证与请求头用例 * fix(api): 以最新依赖重新生成 OpenAPI 类型对齐 CI 校验 --------- Co-authored-by: NightYu <lzy200816@gmail.com>
* fix(proxy): cap client abort drain window * fix(proxy): decouple client abort drain from idle timeout * fix(proxy): enforce idle timeout during abort drain * fix(proxy): preserve abort drain idle deadline * fix(proxy): keep abort-drain idle as client abort
产生竞速赢家(launchedProviderCount>1)时,无条件把 Session 复用绑定改绑到赢家。updateSessionBindingSmart 新增 forceUpdate 短路智能决策;commitWinner 传 isActualHedgeWin。含 TDD 单测与深度评审加固。
📝 WalkthroughWalkthrough新增完整的供应商批量测试功能：前端 Changes供应商批量测试功能
Proxy 层修复：Hedge Session 强制改绑 + SSE Drain/Idle 重构
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
enhancement
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a batch testing feature for providers, adding a new API endpoint, server-side actions, a batch test dialog UI, and corresponding localization files. It also refactors the session binding logic to unconditionally rebind to hedge race winners, and updates the streaming idle timeout handling in ProxyResponseHandler to manage client aborts and stream finalization more robustly. However, a critical ReferenceError was identified in response-handler.ts where clientAborted is accessed in a catch block outside of its declared try block scope, which will cause a runtime crash during idle timeouts.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| await finalizeStream( | ||
| allContent, | ||
| false, | ||
| clientAborted, | ||
| clientAborted ? "CLIENT_ABORTED" : "STREAM_IDLE_TIMEOUT" | ||
| ); |
There was a problem hiding this comment.
There is a potential ReferenceError here because clientAborted is declared inside the try block of the stream reading loop (line 2803) and is block-scoped. In the catch block (where this finalizeStream call resides), clientAborted is not in scope and will cause a runtime crash when an idle timeout occurs. You should use session.clientAbortSignal?.aborted ?? false directly or declare a variable in the outer scope.
const isAborted = session.clientAbortSignal?.aborted ?? false;
await finalizeStream(
allContent,
false,
isAborted,
isAborted ? "CLIENT_ABORTED" : "STREAM_IDLE_TIMEOUT"
);
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/session-manager.ts (1)
764-808:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
forceUpdate会被isFirstAttempt分支提前短路，竞速强制改绑可能失效当前先走
isFirstAttempt，在SET NX失败时会直接返回，导致后续forceUpdate分支无法执行。若竞速赢家路径出现isFirstAttempt=true && forceUpdate=true，旧绑定会被保留，和“无条件改绑”目标冲突。建议修复（最小改动）
- if (isFirstAttempt) { + if (isFirstAttempt && !isFailoverSuccess && !forceUpdate) {这与本次修复目标（竞速赢家必须覆盖旧绑定）不一致，建议优先修正分支优先级。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/session-manager.ts` around lines 764 - 808, The isFirstAttempt branch is executing before the forceUpdate check, causing the function to return early when SET NX fails, which prevents the forceUpdate logic from ever executing. This breaks the race-winning scenario where forceUpdate should unconditionally override the old binding. Reorder the conditional logic in the bindProviderToSession method so that the check for isFailoverSuccess or forceUpdate is evaluated before the isFirstAttempt branch, ensuring that force update scenarios bypass the first-attempt early return and execute the pipeline-based unconditional update instead.
🧹 Nitpick comments (1)
tests/unit/proxy/response-handler-client-abort-drain.test.ts (1)
704-891: ⚡ Quick win建议补一个“未触发 client abort 时的 idle timeout”回归用例。
当前新增用例都先触发了 client abort，无法覆盖
statusCode=502 + errorMessage=STREAM_IDLE_TIMEOUT的专用分支。建议再加一条“客户端保持连接，仅等待 idle timeout”的断言，避免原因码回归。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/proxy/response-handler-client-abort-drain.test.ts` around lines 704 - 891, Add a new test case that covers the scenario where the streaming idle timeout expires without any client abort occurring. This test should create a session with a streamingIdleTimeoutMs value, dispatch a response handler with a hanging response stream, wait for the idle timeout to elapse (without calling clientController.abort()), and then verify that the upstream connection is aborted with updateMessageRequestDetails being called with statusCode 502 and errorMessage "STREAM_IDLE_TIMEOUT". This ensures the idle timeout code path independent of client abort is properly tested and prevents regression of the error handling logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/actions/providers.ts`:
- Around line 4737-4740: The hardcoded Chinese status text values ("可用", "波动",
"不可用") and the constructed message variable violate i18n guidelines. Replace the
statusText and message construction logic to return localization keys and
parameters instead of hardcoded Chinese strings. Specifically, instead of
constructing statusText based on result.status and building the message
directly, return a messageKey (such as a reference to the i18n key) and
messageParams that include the status and subStatus values. This allows
next-intl on the frontend to render the appropriate localized text based on the
user's locale.
In
`@src/app/`[locale]/settings/providers/_components/batch-test/batch-test-dialog.tsx:
- Around line 178-224: The undo callback in the applyBulkEnabledPatch function
has an inconsistency with query cache invalidation timing. On line 206 in the
main success path, queryClient.invalidateQueries is awaited, but on line 224
within the undo action onClick handler, the same queryClient.invalidateQueries
call lacks an await. This can cause the undo success toast to display before the
cache is actually invalidated, allowing users to see stale data. Add await
before queryClient.invalidateQueries on line 224 to ensure the cache is
invalidated before the success toast is shown, maintaining consistency with the
main success path behavior.
In
`@src/app/`[locale]/settings/providers/_components/batch-test/use-batch-provider-test.ts:
- Around line 91-98: The type assertion of `result.data` to `UnifiedTestData` at
line 91 lacks runtime validation, and the underlying API client function
`testProviderById` in src/lib/api-client/v1/actions/providers.ts does not
specify an explicit return type, allowing structural mismatches to go
undetected. Fix this by first adding an explicit return type specification to
the `testProviderById` function in the API client to properly constrain the
response type at the source, and then add runtime validation (using Zod or
similar validation library) at the component layer where `result.data` is being
accessed to ensure the response structure matches expectations before accessing
individual properties in the setRow call.
In `@src/app/v1/_lib/proxy/response-handler.ts`:
- Around line 2357-2360: The idle timeout error injected at
streamController.error is not being properly identified as an idle timeout at
the error categorization logic around line 2846, causing it to be misattributed
to STREAM_PROCESSING_ERROR instead of STREAM_IDLE_TIMEOUT. Ensure the Error
created with message "Streaming idle timeout" in the streamController.error call
is properly recognizable by the idle timeout checking logic. This may require
either creating a distinctive error type (such as an AbortError or custom error
class) for idle timeouts, or adding an identifiable property/marker to the error
that the categorization logic at line 2846 can check to properly route it as
STREAM_IDLE_TIMEOUT. Apply the same fix approach at all affected locations
including the sibling site at lines 2843-2847 to maintain consistency across all
idle timeout handling paths.
In `@src/lib/api/v1/schemas/providers.ts`:
- Around line 313-317: The newly added ProviderTestByIdSchema is missing a
corresponding TypeScript type export that other schemas in the file have. At the
end of the file where other schema types are exported (around lines 513-524),
add a type export for ProviderTestByIdSchema using the z.infer pattern to
maintain consistency with existing schema types like ProviderUnifiedTestInput.
The new type should be named ProviderTestByIdInput and should infer its type
from the ProviderTestByIdSchema definition.
- Around line 313-317: The ProviderTestByIdSchema has an inconsistent validation
constraint on the model field compared to the similar ProviderApiTestSchema at
Line 283. Remove the .min(1) constraint from the model field in
ProviderTestByIdSchema so that it matches ProviderApiTestSchema which only uses
.trim().optional() without length validation. This allows whitespace and empty
strings to be accepted at the API layer, ensuring consistent behavior with the
action layer which is expected to ignore blank model overrides and fall back to
defaults according to the test cases.
---
Outside diff comments:
In `@src/lib/session-manager.ts`:
- Around line 764-808: The isFirstAttempt branch is executing before the
forceUpdate check, causing the function to return early when SET NX fails, which
prevents the forceUpdate logic from ever executing. This breaks the race-winning
scenario where forceUpdate should unconditionally override the old binding.
Reorder the conditional logic in the bindProviderToSession method so that the
check for isFailoverSuccess or forceUpdate is evaluated before the
isFirstAttempt branch, ensuring that force update scenarios bypass the
first-attempt early return and execute the pipeline-based unconditional update
instead.
---
Nitpick comments:
In `@tests/unit/proxy/response-handler-client-abort-drain.test.ts`:
- Around line 704-891: Add a new test case that covers the scenario where the
streaming idle timeout expires without any client abort occurring. This test
should create a session with a streamingIdleTimeoutMs value, dispatch a response
handler with a hanging response stream, wait for the idle timeout to elapse
(without calling clientController.abort()), and then verify that the upstream
connection is aborted with updateMessageRequestDetails being called with
statusCode 502 and errorMessage "STREAM_IDLE_TIMEOUT". This ensures the idle
timeout code path independent of client abort is properly tested and prevents
regression of the error handling logic.
🪄 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: b0e3c3cd-a46b-414c-a0ba-30290ecd6375
📒 Files selected for processing (40)
.gitignoremessages/en/settings/index.tsmessages/en/settings/providers/batchEdit.jsonmessages/en/settings/providers/batchTest.jsonmessages/ja/settings/index.tsmessages/ja/settings/providers/batchEdit.jsonmessages/ja/settings/providers/batchTest.jsonmessages/ru/settings/index.tsmessages/ru/settings/providers/batchEdit.jsonmessages/ru/settings/providers/batchTest.jsonmessages/zh-CN/settings/index.tsmessages/zh-CN/settings/providers/batchEdit.jsonmessages/zh-CN/settings/providers/batchTest.jsonmessages/zh-TW/settings/index.tsmessages/zh-TW/settings/providers/batchEdit.jsonmessages/zh-TW/settings/providers/batchTest.jsonsrc/actions/providers.tssrc/app/[locale]/settings/providers/_components/batch-edit/provider-batch-actions.tsxsrc/app/[locale]/settings/providers/_components/batch-test/batch-test-dialog.tsxsrc/app/[locale]/settings/providers/_components/batch-test/index.tssrc/app/[locale]/settings/providers/_components/batch-test/use-batch-provider-test.tssrc/app/[locale]/settings/providers/_components/provider-manager.tsxsrc/app/api/v1/resources/providers/handlers.tssrc/app/api/v1/resources/providers/router.tssrc/app/v1/_lib/proxy/forwarder.tssrc/app/v1/_lib/proxy/response-handler.tssrc/lib/api-client/v1/actions/providers.tssrc/lib/api-client/v1/openapi-types.gen.tssrc/lib/api/v1/schemas/providers.tssrc/lib/provider-testing/test-service.tssrc/lib/provider-testing/types.tssrc/lib/provider-testing/utils/test-prompts.tssrc/lib/session-manager.tstests/unit/actions/providers-test-by-id.test.tstests/unit/lib/session-manager-binding-smart.test.tstests/unit/provider-testing/test-prompts-headers.test.tstests/unit/proxy/proxy-forwarder-hedge-first-byte.test.tstests/unit/proxy/response-handler-client-abort-drain.test.tstests/unit/settings/providers/provider-manager.test.tsxtests/unit/settings/providers/use-batch-provider-test.test.tsx
| const statusText = | ||
| result.status === "green" ? "可用" : result.status === "yellow" ? "波动" : "不可用"; | ||
| const message = `供应商 ${statusText}: ${SUB_STATUS_MESSAGES[result.subStatus]}`; | ||
|
|
There was a problem hiding this comment.
将统一测试结果文案改为可本地化键值，而不是硬编码中文。
Line 4737-4740 在服务端拼接中文 message，但该字段会被前端直接展示，非中文语言环境会出现错误语言。建议返回 messageKey/messageParams（或按 locale 生成文案）并由 next-intl 渲染。
As per coding guidelines: “All user-facing strings must use i18n (5 languages supported: zh-CN, zh-TW, en, ja, ru). Never hardcode display text”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/actions/providers.ts` around lines 4737 - 4740, The hardcoded Chinese
status text values ("可用", "波动", "不可用") and the constructed message variable
violate i18n guidelines. Replace the statusText and message construction logic
to return localization keys and parameters instead of hardcoded Chinese strings.
Specifically, instead of constructing statusText based on result.status and
building the message directly, return a messageKey (such as a reference to the
i18n key) and messageParams that include the status and subStatus values. This
allows next-intl on the frontend to render the appropriate localized text based
on the user's locale.
Source: Coding guidelines
| const applyBulkEnabledPatch = useCallback( | ||
| async (providerIds: number[], enabled: boolean) => { | ||
| if (providerIds.length === 0 || isBulkUpdating) return; | ||
| setIsBulkUpdating(true); | ||
| try { | ||
| const patch = { is_enabled: { set: enabled } }; | ||
| const preview = await previewProviderBatchPatch({ providerIds, patch }); | ||
| if (!preview.ok) { | ||
| toast.error(t("toast.bulkFailed", { error: preview.error })); | ||
| return; | ||
| } | ||
| const result = await applyProviderBatchPatch({ | ||
| previewToken: preview.data.previewToken, | ||
| previewRevision: preview.data.previewRevision, | ||
| providerIds, | ||
| patch, | ||
| excludeProviderIds: [], | ||
| }); | ||
| if (!result.ok) { | ||
| toast.error(t("toast.bulkFailed", { error: result.error })); | ||
| return; | ||
| } | ||
|
|
||
| setEnabledOverrides((prev) => { | ||
| const next = new Map(prev); | ||
| for (const id of providerIds) next.set(id, enabled); | ||
| return next; | ||
| }); | ||
| await queryClient.invalidateQueries({ queryKey: ["providers"] }); | ||
|
|
||
| const undoToken = result.data.undoToken; | ||
| const operationId = result.data.operationId; | ||
| toast.success(t("toast.bulkApplied", { count: result.data.updatedCount }), { | ||
| duration: 10000, | ||
| action: { | ||
| label: t("toast.undo"), | ||
| onClick: async () => { | ||
| try { | ||
| const undoResult = await undoProviderPatch({ undoToken, operationId }); | ||
| if (undoResult.ok) { | ||
| setEnabledOverrides((prev) => { | ||
| const next = new Map(prev); | ||
| for (const id of providerIds) next.delete(id); | ||
| return next; | ||
| }); | ||
| toast.success(t("toast.undoSuccess", { count: undoResult.data.revertedCount })); | ||
| queryClient.invalidateQueries({ queryKey: ["providers"] }); |
There was a problem hiding this comment.
撤销回调中缺少查询失效的 await，可能导致时序问题。
Line 224 调用 queryClient.invalidateQueries 时未使用 await，而主成功路径（line 206）中有 await。这种不一致可能导致撤销成功 toast 在查询缓存实际失效前就显示，用户可能短暂看到过期数据。
建议的修复
const undoResult = await undoProviderPatch({ undoToken, operationId });
if (undoResult.ok) {
setEnabledOverrides((prev) => {
const next = new Map(prev);
for (const id of providerIds) next.delete(id);
return next;
});
toast.success(t("toast.undoSuccess", { count: undoResult.data.revertedCount }));
- queryClient.invalidateQueries({ queryKey: ["providers"] });
+ await queryClient.invalidateQueries({ queryKey: ["providers"] });
} else {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/app/`[locale]/settings/providers/_components/batch-test/batch-test-dialog.tsx
around lines 178 - 224, The undo callback in the applyBulkEnabledPatch function
has an inconsistency with query cache invalidation timing. On line 206 in the
main success path, queryClient.invalidateQueries is awaited, but on line 224
within the undo action onClick handler, the same queryClient.invalidateQueries
call lacks an await. This can cause the undo success toast to display before the
cache is actually invalidated, allowing users to see stale data. Add await
before queryClient.invalidateQueries on line 224 to ensure the cache is
invalidated before the success toast is shown, maintaining consistency with the
main success path behavior.
| const data = result.data as UnifiedTestData; | ||
| setRow(providerId, { | ||
| status: data.status, | ||
| latencyMs: data.latencyMs, | ||
| message: data.errorMessage ?? data.message, | ||
| responseModel: data.model, | ||
| httpStatusCode: data.httpStatusCode, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 验证 testProviderById action 的返回类型定义
# 查找 testProviderById action 实现及其返回类型
rg -nP -A10 'export\s+(async\s+)?function\s+testProviderById' --type=ts
# 查找 buildUnifiedTestSuccessData 函数定义（根据 review stack context 提到的辅助函数）
rg -nP -A15 'buildUnifiedTestSuccessData' --type=tsRepository: ding113/claude-code-hub
Length of output: 5460
🏁 Script executed:
cat -n src/app/[locale]/settings/providers/_components/batch-test/use-batch-provider-test.ts | sed -n '80,110p'Repository: ding113/claude-code-hub
Length of output: 1415
🏁 Script executed:
rg -nP 'type UnifiedTestData|interface UnifiedTestData' --type=tsRepository: ding113/claude-code-hub
Length of output: 184
🏁 Script executed:
rg -nP 'type UnifiedTestResult|interface UnifiedTestResult' --type=ts -A 15Repository: ding113/claude-code-hub
Length of output: 4269
🏁 Script executed:
rg -nP 'toActionResult' --type=ts -A 10 src/lib/api-client/v1/actions/providers.tsRepository: ding113/claude-code-hub
Length of output: 6920
🏁 Script executed:
cat -n src/app/[locale]/settings/providers/_components/batch-test/use-batch-provider-test.ts | sed -n '1,40p'Repository: ding113/claude-code-hub
Length of output: 1275
🏁 Script executed:
rg -nP 'type ActionResult|interface ActionResult' --type=ts -A 5Repository: ding113/claude-code-hub
Length of output: 284
🏁 Script executed:
cat -n src/lib/api-client/v1/actions/_compat.ts | head -100Repository: ding113/claude-code-hub
Length of output: 4346
🏁 Script executed:
rg -nP 'export function.*apiPost.*Promise<.*Result' src/lib/api-client --type=ts -A 3 | head -50Repository: ding113/claude-code-hub
Length of output: 49
🏁 Script executed:
rg -nP 'export.*function.*\): Promise<.*Test' src/actions/providers.ts --type=ts | head -20Repository: ding113/claude-code-hub
Length of output: 167
🏁 Script executed:
rg -n 'import.*zod|from.*zod' src/lib/api-client src/app/\[locale\]/settings/providers --type=ts | head -10Repository: ding113/claude-code-hub
Length of output: 49
🏁 Script executed:
rg -nP 'Schema.*=.*z\.' src/app/api/v1 --type=ts | head -10Repository: ding113/claude-code-hub
Length of output: 324
🏁 Script executed:
cat -n src/lib/api-client/v1/actions/providers.ts | sed -n '207,211p'Repository: ding113/claude-code-hub
Length of output: 304
🏁 Script executed:
cat -n src/actions/providers.ts | sed -n '4588,4610p'Repository: ding113/claude-code-hub
Length of output: 767
🏁 Script executed:
rg -nP 'buildUnifiedTestSuccessData' src/actions/providers.ts --type=ts -A 20 | head -30Repository: ding113/claude-code-hub
Length of output: 1168
🏁 Script executed:
rg -nP 'export function.*providerId.*\): Promise<ActionResult' src/lib/api-client --type=ts -A 2Repository: ding113/claude-code-hub
Length of output: 49
🏁 Script executed:
cat -n src/lib/api-client/v1/actions/types.ts | head -40Repository: ding113/claude-code-hub
Length of output: 484
🏁 Script executed:
rg -nP 'ProviderTestByIdSchema|UnifiedTestDataSchema' src --type=tsRepository: ding113/claude-code-hub
Length of output: 533
🏁 Script executed:
cat -n src/app/api/v1/resources/providers/handlers.ts | sed -n '432,460p'Repository: ding113/claude-code-hub
Length of output: 1408
🏁 Script executed:
rg -nP 'errorMessage' src/app/\[locale\]/settings/providers/_components/batch-test --type=ts -B 2 -A 2Repository: ding113/claude-code-hub
Length of output: 1269
🏁 Script executed:
rg -nP 'errorMessage' src/actions/providers.ts --type=ts | head -5Repository: ding113/claude-code-hub
Length of output: 267
🏁 Script executed:
cat -n src/actions/providers.ts | sed -n '4588,4615p'Repository: ding113/claude-code-hub
Length of output: 917
🏁 Script executed:
rg -n 'errorMessage' src/actions/providers.ts | grep -i 'return\|data\|buildUnified'Repository: ding113/claude-code-hub
Length of output: 49
🏁 Script executed:
rg -nP 'export.*function.*\(.*providerId.*\).*return.*toActionResult.*apiPost' src/lib/api-client --type=ts -A 2 | head -30Repository: ding113/claude-code-hub
Length of output: 49
类型断言缺少运行时校验，且 UnifiedTestData 类型定义不完整。
第 91 行将 result.data 断言为 UnifiedTestData，但该类型仅包含 7 个字段，而后端 UnifiedTestResult 实际返回 15+ 个字段（包括 content、requestUrl、rawResponse、usage、streamInfo 等）。若后端响应结构变更，客户端无法在编译时检测，容易导致静默失败。
根本原因是 API 客户端函数 testProviderById（src/lib/api-client/v1/actions/providers.ts:207）未指定返回类型，导致 toActionResult 使用默认的 any 泛型，丧失类型约束。
建议：
- 修复 API 客户端函数签名，显式指定后端返回类型
- 在组件层添加运行时校验，防御意外的响应结构变化
若采用运行时校验方案，可参考：
使用 Zod 进行响应验证
+import { z } from "zod";
+
+const UnifiedTestDataSchema = z.object({
+ success: z.boolean(),
+ status: z.enum(["green", "yellow", "red"]),
+ message: z.string(),
+ latencyMs: z.number(),
+ httpStatusCode: z.number().optional(),
+ model: z.string().optional(),
+ errorMessage: z.string().optional(),
+});
if (result.ok) {
- const data = result.data as UnifiedTestData;
+ const parseResult = UnifiedTestDataSchema.safeParse(result.data);
+ if (!parseResult.success) {
+ setRow(providerId, { status: "error", message: "Invalid response format" });
+ return;
+ }
+ const data = parseResult.data;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/app/`[locale]/settings/providers/_components/batch-test/use-batch-provider-test.ts
around lines 91 - 98, The type assertion of `result.data` to `UnifiedTestData`
at line 91 lacks runtime validation, and the underlying API client function
`testProviderById` in src/lib/api-client/v1/actions/providers.ts does not
specify an explicit return type, allowing structural mismatches to go
undetected. Fix this by first adding an explicit return type specification to
the `testProviderById` function in the API client to properly constrain the
response type at the source, and then add runtime validation (using Zod or
similar validation library) at the component layer where `result.data` is being
accessed to ensure the response structure matches expectations before accessing
individual properties in the setRow call.
| try { | ||
| if (streamController) { | ||
| streamController.error(new Error("Streaming idle timeout")); | ||
| logger.debug("ResponseHandler: Client stream closed due to idle timeout", { |
There was a problem hiding this comment.
Idle timeout 在该分支可能被误归因为 STREAM_PROCESSING_ERROR。
Line 2359 注入的是 new Error("Streaming idle timeout")，但 Line 2846 仅按 "streaming_idle" 判定 idle，且该错误默认不是 AbortError。这会让 idle 触发后更容易绕过 idle 专用分支，最终以通用处理路径结算，丢失 STREAM_IDLE_TIMEOUT 语义。
可直接应用的修复示例
- streamController.error(new Error("Streaming idle timeout"));
+ const idleTimeoutError = new Error("streaming_idle");
+ idleTimeoutError.name = "AbortError";
+ streamController.error(idleTimeoutError);
- const isIdleTimeout = err.message?.includes("streaming_idle");
+ const isIdleTimeout =
+ err.message?.includes("streaming_idle") ||
+ err.message?.includes("Streaming idle timeout");Also applies to: 2843-2847
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/v1/_lib/proxy/response-handler.ts` around lines 2357 - 2360, The idle
timeout error injected at streamController.error is not being properly
identified as an idle timeout at the error categorization logic around line
2846, causing it to be misattributed to STREAM_PROCESSING_ERROR instead of
STREAM_IDLE_TIMEOUT. Ensure the Error created with message "Streaming idle
timeout" in the streamController.error call is properly recognizable by the idle
timeout checking logic. This may require either creating a distinctive error
type (such as an AbortError or custom error class) for idle timeouts, or adding
an identifiable property/marker to the error that the categorization logic at
line 2846 can check to properly route it as STREAM_IDLE_TIMEOUT. Apply the same
fix approach at all affected locations including the sibling site at lines
2843-2847 to maintain consistency across all idle timeout handling paths.
| export const ProviderTestByIdSchema = z | ||
| .object({ | ||
| model: z.string().trim().min(1).optional().describe("Optional model override."), | ||
| }) | ||
| .strict(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
缺少对应的 TypeScript 类型导出
新增的 ProviderTestByIdSchema 缺少相应的类型别名导出。对比文件末尾（Lines 513-524），其他 schema 都导出了对应的类型，例如：
export type ProviderUnifiedTestInput = z.infer<typeof ProviderUnifiedTestSchema>;建议在文件末尾补充类型导出以保持一致性：
export type ProviderTestByIdInput = z.infer<typeof ProviderTestByIdSchema>;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/api/v1/schemas/providers.ts` around lines 313 - 317, The newly added
ProviderTestByIdSchema is missing a corresponding TypeScript type export that
other schemas in the file have. At the end of the file where other schema types
are exported (around lines 513-524), add a type export for
ProviderTestByIdSchema using the z.infer pattern to maintain consistency with
existing schema types like ProviderUnifiedTestInput. The new type should be
named ProviderTestByIdInput and should infer its type from the
ProviderTestByIdSchema definition.
model 字段的 .min(1) 约束与类似 schema 不一致
当前 model 字段使用 z.string().trim().min(1).optional()，这会拒绝纯空格字符串（trim 后长度为 0）。但对比 Line 283 的 ProviderApiTestSchema，其 model 字段仅使用 .trim().optional() 而无 .min(1) 约束。
根据相关测试用例（tests/unit/actions/providers-test-by-id.test.ts:218-225），空白 model 覆盖应被忽略并回退到默认值。当前的 .min(1) 约束可能在 API 层拒绝此类请求，导致与 action 层的行为不一致。
建议移除 .min(1) 约束以保持一致性：
export const ProviderTestByIdSchema = z
.object({
- model: z.string().trim().min(1).optional().describe("Optional model override."),
+ model: z.string().trim().optional().describe("Optional model override."),
})
.strict();📝 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.
| export const ProviderTestByIdSchema = z | |
| .object({ | |
| model: z.string().trim().min(1).optional().describe("Optional model override."), | |
| }) | |
| .strict(); | |
| export const ProviderTestByIdSchema = z | |
| .object({ | |
| model: z.string().trim().optional().describe("Optional model override."), | |
| }) | |
| .strict(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/api/v1/schemas/providers.ts` around lines 313 - 317, The
ProviderTestByIdSchema has an inconsistent validation constraint on the model
field compared to the similar ProviderApiTestSchema at Line 283. Remove the
.min(1) constraint from the model field in ProviderTestByIdSchema so that it
matches ProviderApiTestSchema which only uses .trim().optional() without length
validation. This allows whitespace and empty strings to be accepted at the API
layer, ensuring consistent behavior with the action layer which is expected to
ignore blank model overrides and fall back to defaults according to the test
cases.
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (!idleTimeoutId) { | ||
| startIdleTimer(); | ||
| } |
There was a problem hiding this comment.
idleTimeoutId is assigned via setTimeout(...) inside startIdleTimer but is never cleared when the callback actually executes. After the idle timer fires and calls abortController.abort(), idleTimeoutId still holds a reference to the fired (already-expired) timer. The guard if (!idleTimeoutId) in the client-abort listener therefore evaluates to false even after the timer has already fired, so the listener would not restart the idle timer if re-triggered. In practice this is benign (the abort already killed the stream), but the stale handle could cause the clearTimeout in the finally block to behave unexpectedly in edge cases. Clearing idleTimeoutId = null at the top of the timer callback (before abortController.abort()) would keep the state consistent.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/response-handler.ts
Line: 2408-2410
Comment:
**Idle timer ID remains set after the timer fires**
`idleTimeoutId` is assigned via `setTimeout(...)` inside `startIdleTimer` but is never cleared when the callback actually executes. After the idle timer fires and calls `abortController.abort()`, `idleTimeoutId` still holds a reference to the fired (already-expired) timer. The guard `if (!idleTimeoutId)` in the client-abort listener therefore evaluates to `false` even after the timer has already fired, so the listener would not restart the idle timer if re-triggered. In practice this is benign (the abort already killed the stream), but the stale handle could cause the `clearTimeout` in the `finally` block to behave unexpectedly in edge cases. Clearing `idleTimeoutId = null` at the top of the timer callback (before `abortController.abort()`) would keep the state consistent.
How can I resolve this? If you propose a fix, please make it concise.| ); | ||
| } | ||
|
|
||
| export async function testProviderById(c: Context): Promise<Response> { | ||
| const id = Number(c.req.param("id")); | ||
| if (!Number.isInteger(id) || id <= 0) { | ||
| return createProblemResponse({ | ||
| status: 400, | ||
| instance: new URL(c.req.url).pathname, | ||
| errorCode: "request.validation_failed", | ||
| detail: "Provider id is invalid.", | ||
| }); | ||
| } | ||
| const body = await parseJson(c, ProviderTestByIdSchema); | ||
| if (body instanceof Response) return body; | ||
| const existing = await findVisibleProvider(c, id); | ||
| if (existing instanceof Response) return existing; | ||
| if (!existing) return providerNotFound(c); | ||
| const providerActions = await import("@/actions/providers"); | ||
| return actionJson( | ||
| c, | ||
| await callAction(c, providerActions.testProviderById, [id, body] as never[], c.get("auth")) | ||
| ); | ||
| } | ||
|
|
||
| export async function testProviderAnthropic(c: Context): Promise<Response> { | ||
| return callProviderTest(c, ProviderApiTestSchema, "testProviderAnthropicMessages"); | ||
| } |
There was a problem hiding this comment.
testProviderById in the handler calls findVisibleProvider(c, id) to assert existence, but the resolved existing object is never forwarded — the action just receives [id, body] and independently calls findProviderById(providerId) again. This performs two DB round-trips for every test request. Passing existing directly to the action (or exposing a variant that accepts a pre-loaded provider) would halve the database work, which matters under the batch-test concurrency load (up to 5 concurrent tests × 2 queries each).
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/api/v1/resources/providers/handlers.ts
Line: 429-456
Comment:
**Redundant provider lookup between handler and action**
`testProviderById` in the handler calls `findVisibleProvider(c, id)` to assert existence, but the resolved `existing` object is never forwarded — the action just receives `[id, body]` and independently calls `findProviderById(providerId)` again. This performs two DB round-trips for every test request. Passing `existing` directly to the action (or exposing a variant that accepts a pre-loaded provider) would halve the database work, which matters under the batch-test concurrency load (up to 5 concurrent tests × 2 queries each).
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| * Server-side variant of testProviderUnified: loads URL, key, proxy and custom | ||
| * headers from the database so the plaintext key never reaches the client. | ||
| * Used by the batch provider testing UI. The test result does not touch the | ||
| * circuit breaker or usage statistics. | ||
| */ | ||
| export async function testProviderById( | ||
| providerId: number, | ||
| args?: TestProviderByIdArgs | ||
| ): Promise<UnifiedTestResult> { | ||
| const session = await getSession(); | ||
| if (!session || session.user.role !== "admin") { | ||
| return { | ||
| ok: false, | ||
| error: "未授权", | ||
| }; |
There was a problem hiding this comment.
If GeminiAuth.getAccessToken throws (e.g. network failure retrieving the access token), apiKey is left as the original provider.key value and geminiBearerAuth stays false. The test then proceeds, sending a raw JSON credential string as x-goog-api-key, which Gemini will reject with an opaque 401/403. The provider test result will show red without a clear message that the failure was in the auth pre-processing step rather than in the provider itself. Consider returning { ok: false, error: "Gemini 凭证预处理失败: ..." } on catch to surface the real failure mode to the operator.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/actions/providers.ts
Line: 4782-4796
Comment:
**Gemini auth failure silently continues with raw key**
If `GeminiAuth.getAccessToken` throws (e.g. network failure retrieving the access token), `apiKey` is left as the original `provider.key` value and `geminiBearerAuth` stays `false`. The test then proceeds, sending a raw JSON credential string as `x-goog-api-key`, which Gemini will reject with an opaque 401/403. The provider test result will show `red` without a clear message that the failure was in the auth pre-processing step rather than in the provider itself. Consider returning `{ ok: false, error: "Gemini 凭证预处理失败: ..." }` on catch to surface the real failure mode to the operator.
How can I resolve this? If you propose a fix, please make it concise.![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 release merge (v0.8.7) bundles 3 previously-reviewed feature/fix PRs: batch provider testing (#1276), hedge-winner session rebinding (#1279), and client-abort drain window capping (#1277). The new code is well-structured, properly error-handled, and backed by solid unit coverage (6 new/expanded test files). No issues cleared the high-confidence reporting threshold.
PR Size: XL
- Lines changed: 2455 (2337 additions / 118 deletions)
- Files changed: 40
Split context
This is a dev -> main release merge aggregating 3 already-merged sub-PRs, each with its own review history. The size is inherent to a release cut, not a single feature. Logical boundaries:
- #1276 Batch provider testing:
src/actions/providers.ts,src/lib/provider-testing/*,src/app/.../batch-test/*,src/app/api/v1/resources/providers/*, i18n (10 message files) - #1279 Hedge-winner rebinding:
src/lib/session-manager.ts,src/app/v1/_lib/proxy/forwarder.ts - #1277 Client-abort drain cap:
src/app/v1/_lib/proxy/response-handler.ts
Notes (below reporting threshold, non-blocking)
- Server-action error/status strings in
testProviderById(e.g. "未授权", "测试执行失败") are hardcoded Chinese. This faithfully follows the established pattern already present intestProviderUnifiedand the sharedbuildUnifiedTestSuccessDatahelper in the same file, so it is treated as intentional design rather than a new i18n regression.
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean (admin-gated route, SSRF URL validation preserved on the by-id path, plaintext key never reaches client)
- Error handling - Clean (Gemini auth preprocess failure is logged + the downstream test surfaces a real error; all client-side failures surfaced via toast)
- Type safety - Clean
- Documentation accuracy - Comments explain non-obvious decisions (Gemini bearer auth, idle/drain decoupling, forceUpdate short-circuit)
- Test coverage - Strong (auth gating, validation, gemini JSON credentials, concurrency cap, cancel semantics, client-abort drain edge cases)
- Code clarity - Good
Automated review by Claude AI
Code Review SummaryThis release merge (v0.8.7) bundles 3 previously-reviewed feature/fix PRs: batch provider testing (#1276), hedge-winner session rebinding (#1279), and client-abort drain window capping (#1277). The new code is well-structured, properly error-handled, and backed by solid unit coverage (6 new/expanded test files). No issues cleared the high-confidence reporting threshold. PR Size: XL
Split contextThis is a
Notes (below reporting threshold, non-blocking)
Review Coverage
Automated review by Claude AI |
3 participants
Release v0.8.7
dev→main,自 v0.8.6(d0a178ca)以来累计 3 个 PR,40 文件+2337 / -118。无分叉,可直接快进/合并。✨ Features
#1276 批量测试供应商能力 — @NightYuYyy
#1279 竞速赢家无条件改绑 Session 复用绑定 — @ding113
launchedProviderCount > 1)时,无条件把 Session 复用绑定改绑到赢家。updateSessionBindingSmart新增forceUpdate,在读取当前绑定/优先级/熔断状态之前短路智能决策;commitWinner传isActualHedgeWin。🐛 Fixes
#1277 限制客户端中断后的 drain 窗口 — @Brisbanehuang
影响面
response-handler.ts(abort drain)、forwarder.ts+session-manager.ts(竞速赢家绑定)。备注
VERSION文件当前仍为0.8.6(main 与 dev 一致)。若版本号 bump 由 release 流程/自动化处理则无需改动;如需在本 PR 内 bump 到0.8.7,请告知。Description enhanced by Claude AI
Greptile Summary
This release bundles three PRs into
main: a batch provider-testing UI (with concurrent execution, per-row enable toggles, and bulk enable/disable with undo), a fix that decouples the client-abort drain window from the streaming idle timeout, and a session-binding change that unconditionally rebinds the hedge race winner.#1276): newtestProviderByIdserver action + API endpoint (POST /providers/{id}/test), a 5-worker concurrency pool hook (useBatchProviderTest), a dialog with real-time progress/filter UI, and Gemini JSON-credential → Bearer-token exchange mirroring the production auth flow.#1277):clientAbortDrainTimeoutMsis now alwaysCLIENT_ABORT_DRAIN_MAX_MS(60 s); after client disconnect the idle timer is started if not already running, so pre-body silence drains early without waiting the full 60 s; thefinalizeStreamcall in the idle-timeout branch now correctly passes theclientAbortedflag.#1279):updateSessionBindingSmartgains aforceUpdateparameter that short-circuits all smart-decision logic;commitWinnerpassesisActualHedgeWinto ensure the winning provider becomes the session binding even when it was also the initial provider.Confidence Score: 4/5
The proxy and session-manager changes are narrow and well-tested; the batch test feature is purely additive. Safe to merge with minor follow-up items.
The drain/idle decoupling logic is well-reasoned and covered by new test cases. The hedge-winner force-rebind is a one-line parameter addition with thorough unit tests. The batch test feature is purely additive (new API route, new UI components). The only things worth a second look are:
idleTimeoutIdnot being cleared inside the timer callback (stale handle after fire), the double DB lookup in thetestProviderByIdhandler+action pair, and the Gemini auth failure path that silently continues with the raw key rather than surfacing a clear error to the operator — none of which block the request.response-handler.tsfor the timer-handle lifecycle;handlers.ts+actions/providers.tsfor the redundant provider fetch in the test-by-id path.Important Files Changed
CLIENT_ABORT_DRAIN_MAX_MSconstant introduced;startIdleTimer/clearIdleTimer/chunkshoisted to outer scope; client-abort handler now starts idle timer if not already running; idle-timeout finalization correctly passesclientAbortedflag. Logic is sound butidleTimeoutIdis not cleared inside the timer callback, leaving a stale handle after fire.forceUpdateparameter toupdateSessionBindingSmartthat short-circuits smart-decision logic and unconditionally rebinds to the winning provider; logging and return reasons are properly differentiated betweenfailover_successandrace_winner_forced.isActualHedgeWin(launchedProviderCount > 1) as the newforceUpdateargument toupdateSessionBindingSmart, correctly forcing binding rebind whenever a real race winner is determined.testProviderUnifiedto use sharedbuildUnifiedTestSuccessData; addedtestProviderByIdserver action with admin auth check, URL safety validation, Gemini JSON-credential-to-Bearer token exchange, and configurable per-type timeouts. Auth failure silently falls back to raw key, which could produce misleading test results.testProviderById. UsesrunIdReffor stale-run guard andcancelRefto stop new dispatches without aborting in-flight tests. JavaScript single-threaded guarantees make the sharedcursorincrement safe.testProviderByIdhandler: validates id, parses schema, callsfindVisibleProviderfor existence check, then delegates to action. The pre-check result is not forwarded to the action, causing a redundant DB lookup.geminiBearerAuthoverride togetTestHeaders; when true, sends access token asAuthorization: Bearerinstead ofx-goog-api-key, correctly mirroring the production Gemini auth flow for JSON credentials.Sequence Diagram
sequenceDiagram participant C as Client participant BT as BatchTestDialog participant Hook as useBatchProviderTest participant API as ProviderTestAPI participant Action as testProviderById participant Gemini as GeminiAuth C->>BT: "Open dialog and click Start" BT->>Hook: "run(providerIds, model)" loop "Up to 5 concurrent workers" Hook->>API: "POST /providers/{id}/test" API->>Action: "testProviderById(id, body)" alt "Gemini provider" Action->>Gemini: "getAccessToken(key)" Gemini-->>Action: "accessToken" end Action-->>API: "UnifiedTestResult" API-->>Hook: "ok + data" Hook->>BT: "setRow(id, {status, latencyMs})" end BT->>C: "Show results with bulk enable/disable" C->>BT: "Close dialog" BT->>Hook: "cancel()"Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "feat(proxy): 竞速赢家无条件改绑 Session 复用绑定 (#12..." | Re-trigger Greptile