Conversation
Summary of ChangesHello @tesgth032, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 此拉取请求旨在解决代理服务中一个关键的“假200”问题，即上游服务可能返回HTTP 200状态码，但其流式响应（SSE）内容实际上包含错误信息。通过将流式响应的最终成功或失败判定延迟到数据流完全结束后，并引入一套新的响应体内容检测机制，系统现在能够准确识别这些隐性错误。这显著提升了代理的健壮性，确保了熔断器、故障转移和会话绑定策略能够基于真实的响应状态进行决策，从而避免了客户端因误判成功而重复使用故障提供者的问题。 Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthrough引入对 SSE 流的延迟最终化机制：forwarder 在 SSE 启动时保存最终化元数据并早返回原始响应；response-handler 在流结束后消费该元数据、检测上游“假200”并决定生效状态/错误码，然后将 effectiveStatusCode、errorMessage 与 providerIdForPersistence 贯穿到统计、会话与断路器更新；同时新增上游错误检测工具、相应测试与会话响应体存储开关及多语言前端提示与测试。 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // 情况 2：SSE 文本。快速过滤：既无 "error" 也无 "message" key 时跳过解析 | ||
| // 注意：这里用 key 形式的引号匹配，尽量避免 assistant 正文里出现 "error" 造成的无谓解析 | ||
| if (!text.includes("\"error\"") && !text.includes("\"message\"")) { | ||
| return { isError: false }; |
There was a problem hiding this comment.
quick filter may miss error JSON if assistant response contains quoted strings with "error" or "message" as content (not as keys), e.g. data: {"content": "explain \"error\" handling"} would incorrectly skip the check
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/utils/upstream-error-detection.ts
Line: 122:125
Comment:
quick filter may miss error JSON if assistant response contains quoted strings with `"error"` or `"message"` as content (not as keys), e.g. `data: {"content": "explain \"error\" handling"}` would incorrectly skip the check
How can I resolve this? If you propose a fix, please make it concise.
![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 deferred streaming finalization mechanism to accurately determine the success or failure of streaming (SSE) responses, particularly addressing "fake 200" scenarios where an upstream service returns an HTTP 200 status but an error within the response body. Key changes include a new stream-finalization utility to store temporary metadata on the session, modifications to ProxyForwarder to defer finalization for SSE responses, and updates to ProxyResponseHandler to consume this deferred metadata. A new upstream-error-detection utility is used to analyze the complete streamed content for errors, subsequently updating circuit breakers, session bindings, and request statistics. Review comments identify several issues: the error detection logic is vulnerable to prompt injection due to generic keyword matching on AI-generated content; potential inconsistencies in logging and state management if provider IDs mismatch during deferred finalization; risks of logging and storing sensitive information from upstream error messages without redaction; suggestions to move dynamic imports to the top of files; a recommendation to replace type assertion for deferred finalization metadata with a WeakMap for better type safety and maintainability; and the need to extend the error detection utility to handle JSON array responses.
| if (rawJsonChars < options.maxJsonCharsForMessageCheck) { | ||
| const message = | ||
| typeof obj.message === "string" | ||
| ? obj.message | ||
| : isPlainRecord(obj.error) && typeof obj.error.message === "string" | ||
| ? obj.error.message | ||
| : null; | ||
|
|
||
| if (message && options.messageKeyword.test(message)) { | ||
| return { | ||
| isError: true, | ||
| reason: `上游返回 200 但 JSON.message 命中关键字: ${truncateForReason(message)}`, | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
The detectFromJsonObject function identifies upstream errors by checking if the message or error.message fields in a JSON response contain the keyword 'error' (case-insensitive). Since these fields can contain content generated by the AI model, an attacker can use prompt injection to force the AI to output a response that triggers this detection (e.g., by asking the AI to output a JSON object containing the word 'error'). This results in the proxy incorrectly identifying a successful response as a provider failure, which trips the circuit breaker and can lead to a Denial of Service for that provider for all users.
Remediation: Avoid using generic keyword matching on fields that may contain AI-generated content. Instead, use more specific patterns that match known error formats from supported providers, or rely on HTTP status codes and specific error types.
| if (provider.id !== meta.providerId) { | ||
| logger.warn("[ResponseHandler] Deferred streaming meta provider mismatch", { | ||
| sessionId: session.sessionId ?? null, | ||
| metaProviderId: meta.providerId, | ||
| currentProviderId: provider.id, | ||
| }); | ||
| } |
There was a problem hiding this comment.
当从会话中获取的 provider.id 与 meta.providerId 不匹配时，当前逻辑仅记录一条警告日志，但后续流程会继续执行。这可能导致状态不一致，因为 addProviderToChain 方法使用的是从会话中获取的、可能已过时的 provider 对象，而熔断器和会话绑定逻辑使用的是 meta 中的信息。这会导致决策链（provider chain）中记录错误的供应商信息，影响问题排查。
为了保证数据一致性，建议在此处采取更严格的措施。例如，如果出现不匹配，可以考虑使用 meta 中的信息来获取正确的供应商对象，或者至少在调用 addProviderToChain 时使用 meta 中的 providerId 和 providerName，以确保审计日志的准确性。
| if (detected.isError) { | ||
| logger.warn("[ResponseHandler] SSE completed but body indicates error (fake 200)", { | ||
| providerId: meta.providerId, | ||
| providerName: meta.providerName, | ||
| upstreamStatusCode: meta.upstreamStatusCode, | ||
| effectiveStatusCode, | ||
| reason: detected.reason, | ||
| }); |
There was a problem hiding this comment.
The application extracts error reasons from upstream responses and logs them via logger.warn. These error messages can contain sensitive information such as API keys, PII, or snippets of private prompts echoed back by the provider. Logging this data without redaction poses a risk of information exposure in log management systems.
Remediation: Implement redaction logic for error messages before logging them. Ensure that sensitive data like API keys or PII are removed from the detected.reason string.
| const payload: SessionUsageUpdate = { | ||
| status: effectiveStatusCode >= 200 && effectiveStatusCode < 300 ? "completed" : "error", | ||
| statusCode: effectiveStatusCode, | ||
| ...(streamErrorMessage ? { errorMessage: streamErrorMessage } : {}), | ||
| }; |
There was a problem hiding this comment.
The streamErrorMessage (derived from the upstream response body) is stored in the errorMessage field of the session usage payload in Redis and the message request details in the database. Storing raw error messages from upstream providers without redaction can lead to the persistent storage of sensitive information such as API keys or PII.
Remediation: Sanitize or redact the streamErrorMessage before storing it in Redis or the database. Use existing redaction utilities to ensure sensitive patterns are removed.
|
|
||
| // 计入熔断器：让后续请求能正确触发故障转移/熔断 | ||
| try { | ||
| const { recordFailure } = await import("@/lib/circuit-breaker"); |
There was a problem hiding this comment.
在 finalizeDeferredStreamingFinalizationIfNeeded 函数内部，多次使用了动态 import() 来加载 @/lib/circuit-breaker 等模块。如果不存在循环依赖的问题，建议将这些导入语句移至文件顶部。
这样做有以下好处：
- 提高可读性：将所有依赖项集中在文件开头，使代码结构更清晰。
- 性能：避免在函数调用路径中重复解析和加载模块，减少潜在的性能开销。
| type SessionWithDeferred = ProxySession & { | ||
| deferredStreamingFinalization?: DeferredStreamingFinalization; | ||
| }; | ||
|
|
||
| export function setDeferredStreamingFinalization( | ||
| session: ProxySession, | ||
| meta: DeferredStreamingFinalization | ||
| ): void { | ||
| (session as SessionWithDeferred).deferredStreamingFinalization = meta; | ||
| } | ||
|
|
||
| export function consumeDeferredStreamingFinalization( | ||
| session: ProxySession | ||
| ): DeferredStreamingFinalization | null { | ||
| const s = session as SessionWithDeferred; | ||
| const meta = s.deferredStreamingFinalization ?? null; | ||
| if (meta) { | ||
| s.deferredStreamingFinalization = undefined; | ||
| } | ||
| return meta; | ||
| } |
There was a problem hiding this comment.
当前通过类型断言 (session as SessionWithDeferred) 的方式向 ProxySession 对象动态添加 deferredStreamingFinalization 属性。虽然可行，但这会修改原始对象，并且不够类型安全。
建议使用 WeakMap 来存储会话与延迟最终处理元数据之间的关系。这种方法可以更好地隔离数据，避免污染 ProxySession 对象，也无需修改其原始类型定义，从而提高代码的可维护性和健壮性。
const deferredFinalizationMap = new WeakMap<ProxySession, DeferredStreamingFinalization>();
export function setDeferredStreamingFinalization(
session: ProxySession,
meta: DeferredStreamingFinalization
): void {
deferredFinalizationMap.set(session, meta);
}
export function consumeDeferredStreamingFinalization(
session: ProxySession
): DeferredStreamingFinalization | null {
const meta = deferredFinalizationMap.get(session) ?? null;
if (meta) {
deferredFinalizationMap.delete(session);
}
return meta;
}| if (trimmed.startsWith("{") || trimmed.startsWith("[")) { | ||
| try { | ||
| const parsed = JSON.parse(trimmed) as unknown; | ||
| if (isPlainRecord(parsed)) { | ||
| return detectFromJsonObject(parsed, trimmed.length, merged); | ||
| } | ||
| } catch { | ||
| // JSON 解析失败：不视为错误，交由上层逻辑处理 | ||
| } | ||
| return { isError: false }; | ||
| } |
There was a problem hiding this comment.
当前对纯JSON响应体的错误检测逻辑只处理了根节点为对象（isPlainRecord）的情况。如果上游服务返回的错误信息是一个JSON数组（例如 [{"error": "..."}]），当前的实现会直接跳过错误检测，将其视为正常响应。
建议扩展此处的逻辑，增加对JSON数组的处理，遍历数组中的每个元素并应用 detectFromJsonObject 检查，以覆盖更多可能的错误格式。
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/app/v1/_lib/proxy/response-handler.ts`:
- Around line 155-196: Remove emoji characters from the comments introduced
around the session binding logic: replace comments like "// ⭐ 成功后绑定 session
到供应商（智能绑定策略）" and "// ⭐ 统一更新两个数据源（确保监控数据一致）" with plain-text equivalents (e.g.
"// 成功后绑定 session 到供应商（智能绑定策略）" and "// 统一更新两个数据源，确保监控数据一致"). Check and update
the other occurrences noted (lines around 198-204) to remove any emoji as well;
the changes are adjacent to the calls to
SessionManager.updateSessionBindingSmart and
SessionManager.updateSessionProvider and the
logger.info/logger.debug/logger.error blocks so you can find them by those
symbols.
In `@src/lib/utils/upstream-error-detection.test.ts`:
- Around line 4-52: Tests are missing coverage for error-as-object-with-message
and error-as-non-string/non-object branches in
detectUpstreamErrorFromSseOrJsonText; add unit tests that (1) pass a JSON body
'{"error":{"message":"rate limited"}}' and assert isError: true, and (2) pass
JSON bodies like '{"error":true}' and '{"error":42}' and assert isError: true,
also add a test for a JSON array input like '[{"error":"something"}]' asserting
isError: false; place these new tests alongside existing cases in
upstream-error-detection.test.ts referencing the
detectUpstreamErrorFromSseOrJsonText function so the core branches (error object
handling, non-string error handling, and array handling) are exercised to raise
coverage above 80%.
In `@src/lib/utils/upstream-error-detection.ts`:
- Around line 52-66: Replace the hardcoded Chinese reason strings in
src/lib/utils/upstream-error-detection.ts with i18n lookups so user-facing text
is localized (e.g. use the project i18n API such as i18n.t or translate). Update
the three places that build reasons (the branches handling typeof errorValue ===
"string", isPlainRecord(errorValue) && typeof errorValue.message === "string",
and the other message/empty-body cases referenced in the comment) to call the
i18n function with keys and pass the truncated value via interpolation (use
truncateForReason(errorValue) or truncateForReason(errorValue.message) as the
interpolated variable). Keep the same semantics of the messages but replace
literal Chinese text with i18n keys and ensure the keys exist for zh-CN, zh-TW,
en, ja, ru in the translations file.
🧹 Nitpick comments (4)
src/lib/utils/upstream-error-detection.ts (3)
24-26:isPlainRecord与session-messages-guards.ts中的实现重复
src/app/[locale]/dashboard/sessions/[sessionId]/messages/_components/session-messages-guards.ts中已存在一个更严谨的isPlainRecord实现（使用了Object.prototype.toString.call检查）。建议抽取到公共 utils 中复用，避免两处维护不同步。当前简化版对
JSON.parse的输出足够安全，但长期维护上存在分叉风险。
69-75:obj.error.message分支在此上下文中不可达执行到 line 69 时，
hasNonEmptyValue(obj.error)必然为false（否则已在 line 50 的if中返回）。当obj.error不含非空值时，isPlainRecord(obj.error) && typeof obj.error.message === "string"恒为false——因为若error是含有message属性的对象，hasNonEmptyValue就会返回true。line 73-74 的
obj.error.message分支是死代码，可以移除以减少误导。简化建议
if (rawJsonChars < options.maxJsonCharsForMessageCheck) { const message = typeof obj.message === "string" ? obj.message - : isPlainRecord(obj.error) && typeof obj.error.message === "string" - ? obj.error.message - : null; + : null; if (message && options.messageKeyword.test(message)) {
128-140: SSE 事件的chars计算存在性能浪费对每个 SSE 事件都重新
JSON.stringify(evt.data)来获取字符长度，当事件较多时开销不小。考虑到maxJsonCharsForMessageCheck的默认阈值为 1000，可以用一个粗略的估算（如原始 SSE data 行的长度）来代替精确的 stringify。不过这仅在 SSE body 包含
"error"或"message"关键字时才执行（line 124 的快速过滤），实际触发频率应该不高，影响有限。src/app/v1/_lib/proxy/response-handler.ts (1)
108-116:recordFailure失败时仅 warn，不影响后续流程line 111 的
try/catch捕获 circuit-breaker 记录失败后只打 warn 日志并继续。这是合理的——circuit-breaker 记录失败不应阻断请求统计的写入。但如果 circuit-breaker 持续不可用，假 200 仍不会触发熔断，与 PR 目标不完全一致。建议在 warn 日志中增加
sessionId以方便排查：建议
logger.warn("[ResponseHandler] Failed to record fake-200 error in circuit breaker", { providerId: meta.providerId, + sessionId: session.sessionId ?? null, error: cbError, });
| if (typeof errorValue === "string") { | ||
| return { | ||
| isError: true, | ||
| reason: `上游返回 200 但 JSON.error 非空: ${truncateForReason(errorValue)}`, | ||
| }; | ||
| } | ||
|
|
||
| if (isPlainRecord(errorValue) && typeof errorValue.message === "string") { | ||
| return { | ||
| isError: true, | ||
| reason: `上游返回 200 但 JSON.error.message 非空: ${truncateForReason(errorValue.message)}`, | ||
| }; | ||
| } | ||
|
|
||
| return { isError: true, reason: "上游返回 200 但 JSON.error 非空" }; |
There was a problem hiding this comment.
reason 字符串硬编码了中文，未使用 i18n
这些 reason 值会经由 finalizeDeferredStreamingFinalizationIfNeeded 传播到 errorMessage，最终写入 updateMessageRequestDetails 和 SessionManager.updateSessionUsage，在仪表盘中对用户可见。按照编码规范，所有用户可见的文本应使用 i18n（支持 zh-CN, zh-TW, en, ja, ru）。
涉及的硬编码中文字符串（同样出现在 line 62、66、80、106）：
"上游返回 200 但 JSON.error 非空: ...""上游返回 200 但 JSON.error.message 非空: ...""上游返回 200 但 JSON.message 命中关键字: ...""上游返回 200 但响应体为空"
As per coding guidelines, **/*.{ts,tsx,js,jsx}: "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
In `@src/lib/utils/upstream-error-detection.ts` around lines 52 - 66, Replace the
hardcoded Chinese reason strings in src/lib/utils/upstream-error-detection.ts
with i18n lookups so user-facing text is localized (e.g. use the project i18n
API such as i18n.t or translate). Update the three places that build reasons
(the branches handling typeof errorValue === "string", isPlainRecord(errorValue)
&& typeof errorValue.message === "string", and the other message/empty-body
cases referenced in the comment) to call the i18n function with keys and pass
the truncated value via interpolation (use truncateForReason(errorValue) or
truncateForReason(errorValue.message) as the interpolated variable). Keep the
same semantics of the messages but replace literal Chinese text with i18n keys
and ensure the keys exist for zh-CN, zh-TW, en, ja, ru in the translations file.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
This PR implements deferred streaming finalization to detect fake-200 errors from upstream providers. The approach is sound: SSE responses are no longer immediately marked as successful at the HTTP header stage; instead, the full response body is inspected after the stream ends. One logic gap was found in the error path where endpoint-level circuit breaker failures are not recorded, creating an asymmetry with the success path.
PR Size: M
- Lines changed: 504 (486 additions, 18 deletions)
- Files changed: 5
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 1 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 1 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
High Priority Issues (Should Fix)
- [LOGIC-BUG]
response-handler.ts:116- MissingrecordEndpointFailurein fake-200 error path. The success path callsrecordEndpointSuccessbut the error path only calls provider-levelrecordFailure, skipping endpoint-level failure recording. This causes asymmetric endpoint health tracking.
Medium Priority Issues (Consider Fixing)
- [TEST-INCOMPLETE]
upstream-error-detection.test.ts- Missing test forerroras object format (e.g.,{"error": {"message": "...", "type": "..."}}), which is the standard OpenAI error response shape. The code handles this correctly at lines 59-64 ofupstream-error-detection.ts, but there is no test verifying this behavior.
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Claude AI
| providerId: meta.providerId, | ||
| error: cbError, | ||
| }); | ||
| } |
There was a problem hiding this comment.
[High] [LOGIC-BUG] Missing recordEndpointFailure in fake-200 error path
Why this is a problem: When a fake-200 error is detected after SSE completes, the code records a provider-level circuit breaker failure (recordFailure) but does NOT record an endpoint-level failure (recordEndpointFailure). Compare with:
- The success path in this same function (line 132-142) which correctly calls
recordEndpointSuccess - The forwarder error path (
forwarder.ts:581) which callsrecordEndpointFailure
This asymmetry means endpoint health tracking will be inaccurate for fake-200 errors: the endpoint will never accumulate failures from this path, potentially preventing endpoint-level circuit breaking from triggering.
Suggested fix:
// Record provider-level failure
try {
const { recordFailure } = await import("@/lib/circuit-breaker");
await recordFailure(meta.providerId, new Error(detected.reason));
} catch (cbError) {
logger.warn("[ResponseHandler] Failed to record fake-200 error in circuit breaker", {
providerId: meta.providerId,
error: cbError,
});
}
// Record endpoint-level failure (mirrors recordEndpointSuccess in success path)
if (meta.endpointId != null) {
try {
const { recordEndpointFailure } = await import("@/lib/endpoint-circuit-breaker");
await recordEndpointFailure(meta.endpointId, new Error(detected.reason));
} catch (endpointError) {
logger.warn("[ResponseHandler] Failed to record endpoint failure for fake-200", {
endpointId: meta.endpointId,
providerId: meta.providerId,
error: endpointError,
});
}
}| const res = detectUpstreamErrorFromSseOrJsonText(sse); | ||
| expect(res.isError).toBe(false); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
[Medium] [TEST-INCOMPLETE] Missing test for error as object format
Why this is a problem: The standard OpenAI error response format is {"error": {"message": "...", "type": "..."}} where error is an object, not a string. The detection code handles this correctly at upstream-error-detection.ts:59-64, but there is no test verifying this important code path. This is a common real-world error format that should be covered.
Suggested fix - add these test cases:
test("JSON: error as object with message is detected as error", () => {
const res = detectUpstreamErrorFromSseOrJsonText(
JSON.stringify({ error: { message: "rate limit exceeded", type: "rate_limit_error" } })
);
expect(res.isError).toBe(true);
if (res.isError) {
expect(res.reason).toContain("rate limit exceeded");
}
});
test("JSON: error as non-empty object without message is detected as error", () => {
const res = detectUpstreamErrorFromSseOrJsonText(
JSON.stringify({ error: { type: "server_error", code: 500 } })
);
expect(res.isError).toBe(true);
});
test("JSON: error as null is not detected as error", () => {
const res = detectUpstreamErrorFromSseOrJsonText('{"error": null}');
expect(res.isError).toBe(false);
});
| export function consumeDeferredStreamingFinalization( | ||
| session: ProxySession | ||
| ): DeferredStreamingFinalization | null { | ||
| const s = session as SessionWithDeferred; | ||
| const meta = s.deferredStreamingFinalization ?? null; | ||
| if (meta) { | ||
| // 只允许消费一次：避免重复结算（例如多个后台统计任务并行时）。 | ||
| s.deferredStreamingFinalization = undefined; | ||
| } | ||
| return meta; |
There was a problem hiding this comment.
potential race condition if called concurrently from multiple tasks
if two background tasks call this simultaneously, both could read the same meta before either clears it, causing duplicate finalization
consider atomic operations or document single-invocation requirement
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/stream-finalization.ts
Line: 41:50
Comment:
potential race condition if called concurrently from multiple tasks
if two background tasks call this simultaneously, both could read the same `meta` before either clears it, causing duplicate finalization
consider atomic operations or document single-invocation requirement
How can I resolve this? If you propose a fix, please make it concise.| const detected = streamEndedNormally | ||
| ? detectUpstreamErrorFromSseOrJsonText(allContent) | ||
| : ({ isError: false } as const); |
There was a problem hiding this comment.
abnormal terminations (timeouts, aborts) skip error detection, keeping upstreamStatusCode (200)
fake 200 responses that timeout won't be detected as errors, missing circuit breaker recording
should abnormal terminations always map to error status?
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/response-handler.ts
Line: 101:103
Comment:
abnormal terminations (timeouts, aborts) skip error detection, keeping `upstreamStatusCode` (200)
fake 200 responses that timeout won't be detected as errors, missing circuit breaker recording
should abnormal terminations always map to error status?
How can I resolve this? If you propose a fix, please make it concise.| const message = | ||
| typeof obj.message === "string" | ||
| ? obj.message | ||
| : isPlainRecord(obj.error) && typeof obj.error.message === "string" | ||
| ? obj.error.message | ||
| : null; |
There was a problem hiding this comment.
nested ternary logic is correct but could be clearer
checking isPlainRecord(obj.error) at line 107 seems redundant since line 84-100 already handled the obj.error cases
consider extracting to separate function for readability
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/utils/upstream-error-detection.ts
Line: 104:109
Comment:
nested ternary logic is correct but could be clearer
checking `isPlainRecord(obj.error)` at line 107 seems redundant since line 84-100 already handled the `obj.error` cases
consider extracting to separate function for readability
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| // 情况 2：SSE 文本。快速过滤：既无 "error" 也无 "message" key 时跳过解析 | ||
| // 注意：这里用 key 形式的引号匹配，尽量避免模型正文里出现 error 造成的无谓解析。 | ||
| // 代价：如果上游返回的并非标准 JSON key（极少见），这里可能漏检；但我们偏向保守与低误判。 | ||
| if (!text.includes("\"error\"") && !text.includes("\"message\"")) { | ||
| return { isError: false }; | ||
| } |
There was a problem hiding this comment.
quick filter text.includes('"error"') may miss errors with non-standard spacing like { "error" : "msg" } or miss legitimate content containing "error" triggering unnecessary parsing
tradeoff between performance and accuracy is acknowledged in comments but worth noting
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/utils/upstream-error-detection.ts
Line: 162:167
Comment:
quick filter `text.includes('"error"')` may miss errors with non-standard spacing like `{ "error" : "msg" }` or miss legitimate content containing "error" triggering unnecessary parsing
tradeoff between performance and accuracy is acknowledged in comments but worth noting
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
| * @param streamEndedNormally - 必须是 reader 读到 done=true 的“自然结束”；超时/中断等异常结束由其它逻辑处理。 | ||
| */ | ||
| async function finalizeDeferredStreamingFinalizationIfNeeded( | ||
| session: ProxySession, |
There was a problem hiding this comment.
Provider mismatch breaks attribution
finalizeDeferredStreamingFinalizationIfNeeded() computes provider from session.provider (current state) but uses meta.providerId for circuit-breaker and chain updates. If session.provider has been switched (or unset) by later logic before stream end, the code will still call session.addProviderToChain(provider, ...) with the current provider object while recording success/failure against the meta provider id, producing inconsistent providerChain + stats attribution. Consider looking up the provider by meta.providerId (or storing the provider snapshot in meta) and using that consistently for addProviderToChain / session provider updates.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/response-handler.ts
Line: 90:93
Comment:
**Provider mismatch breaks attribution**
`finalizeDeferredStreamingFinalizationIfNeeded()` computes `provider` from `session.provider` (current state) but uses `meta.providerId` for circuit-breaker and chain updates. If `session.provider` has been switched (or unset) by later logic before stream end, the code will still call `session.addProviderToChain(provider, ...)` with the *current* provider object while recording success/failure against the *meta* provider id, producing inconsistent providerChain + stats attribution. Consider looking up the provider by `meta.providerId` (or storing the provider snapshot in meta) and using that consistently for `addProviderToChain` / session provider updates.
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| // 计入熔断器：让后续请求能正确触发故障转移/熔断 | ||
| try { | ||
| // 动态导入：避免 proxy 模块与熔断器模块之间潜在的循环依赖。 |
There was a problem hiding this comment.
False-success on abort path
When !meta || !provider || !streamEndedNormally, the function returns effectiveStatusCode (possibly still 200) and errorMessage without updating circuit breaker / chain. In the streaming handler, the “client aborted” branch calls finalizeStream(allContent, false), so effectiveStatusCode stays as the upstream statusCode (often 200) and the later updateSessionUsage / updateMessageRequestDetails paths will record a completed 200 even though the stream did not end normally. This seems to reintroduce incorrect success recording for aborted/partial streams; you likely want aborted streams to force an error effectiveStatusCode (e.g. 499/502) for internal stats, or skip updating usage/details when streamEndedNormally=false.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/response-handler.ts
Line: 133:136
Comment:
**False-success on abort path**
When `!meta || !provider || !streamEndedNormally`, the function returns `effectiveStatusCode` (possibly still 200) and `errorMessage` without updating circuit breaker / chain. In the streaming handler, the “client aborted” branch calls `finalizeStream(allContent, false)`, so `effectiveStatusCode` stays as the upstream `statusCode` (often 200) and the later `updateSessionUsage` / `updateMessageRequestDetails` paths will record a completed 200 even though the stream did not end normally. This seems to reintroduce incorrect success recording for aborted/partial streams; you likely want aborted streams to force an error `effectiveStatusCode` (e.g. 499/502) for internal stats, or skip updating usage/details when `streamEndedNormally=false`.
How can I resolve this? If you propose a fix, please make it concise.| import { describe, expect, test } from "vitest"; | ||
| import { detectUpstreamErrorFromSseOrJsonText } from "./upstream-error-detection"; | ||
|
|
There was a problem hiding this comment.
Test count mismatch
The PR description says this test file has 7 cases, but the file currently defines 13 tests. If CI/test expectations or documentation rely on the declared count, the description should be updated to avoid confusion during review and future maintenance.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/utils/upstream-error-detection.test.ts
Line: 1:3
Comment:
**Test count mismatch**
The PR description says this test file has 7 cases, but the file currently defines 13 tests. If CI/test expectations or documentation rely on the declared count, the description should be updated to avoid confusion during review and future maintenance.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/app/v1/_lib/proxy/response-handler.ts
Line: 261:263
Comment:
**Deferred finalize uses wrong provider**
`finalizeDeferredStreamingFinalizationIfNeeded()` consumes meta (`meta.providerId`) but then uses `const provider = session.provider` for `session.addProviderToChain(provider, ...)` and logs, which can diverge if `session.provider` is mutated during retries/failover before stream completion. This will produce inconsistent attribution (providerChain vs circuit-breaker/session binding). Use a provider lookup/snapshot based on `meta.providerId` for *all* downstream updates, or store the provider object in the deferred meta and use that consistently.
How can I resolve this? If you propose a fix, please make it concise. |
|
已按审查建议更新并推送：
已验证：npm test / npm run typecheck 均通过。 |
Additional Comments (3)
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/app/v1/_lib/proxy/response-handler.ts
Line: 280:282
Comment:
**Provider object mismatch**
`finalizeDeferredStreamingFinalizationIfNeeded()` uses `meta.providerId` for circuit-breaker/session binding, but `session.addProviderToChain(provider, ...)` uses `provider = session.provider` (current mutable state). If `session.provider` has changed by the time the stream ends, the providerChain entry can be attributed to a different provider than the one whose circuit breaker was updated. This will make monitoring and retry decisions inconsistent. Use a provider lookup/snapshot from `meta.providerId` (or store the provider object in `DeferredStreamingFinalization`) and use that consistently for `addProviderToChain` and related fields.
How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/app/v1/_lib/proxy/response-handler.ts
Line: 273:277
Comment:
**Aborted streams recorded as 200**
When `streamEndedNormally=false`, this returns `effectiveStatusCode` based on `detected.isError` (forced to false) and `upstreamStatusCode` (often 200), then skips circuit-breaker/chain updates. Callers still persist `statusCode: effectiveStatusCode`, so client aborts/timeouts can be recorded internally as “completed 200”, reintroducing the false-success problem for partial streams. Consider forcing a non-2xx internal status (e.g. 499/502) or skipping usage/details persistence when the stream did not end normally.
How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/app/v1/_lib/proxy/response-handler.ts
Line: 264:266
Comment:
**Fake-200 check skipped on abnormal end**
`detectUpstreamErrorFromSseOrJsonText(allContent)` is only executed when `streamEndedNormally` is true. If the upstream emits an error JSON early but the connection times out/aborts before natural EOF, this path won’t detect it and will keep the internal `effectiveStatusCode` as the upstream status (commonly 200), missing failure recording. If the goal is to avoid circuit-breaker misses, consider running detection whenever `allContent` is non-empty, regardless of how the stream ended, and only gate *binding/success* on `streamEndedNormally`.
How can I resolve this? If you propose a fix, please make it concise. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/app/v1/_lib/proxy/response-handler.ts`:
- Around line 117-123: When provider.id !== meta.providerId, choose a single
canonical provider ID to use for downstream metrics and state updates: switch
recordFailure and recordSuccess (and any calls that currently pass
meta.providerId) to use the actual current provider.id (the same object passed
to session.addProviderToChain) and add a short comment above the mismatch check
explaining that we intentionally prefer the runtime provider.id for
circuit-breaker accuracy; keep the existing logger.warn but include which ID
will be used for recording so future readers know the decision.
In `@src/lib/utils/upstream-error-detection.ts`:
- Around line 144-155: The JSON branch currently treats inputs starting with "["
as JSON but only calls detectFromJsonObject when isPlainRecord(parsed) is true,
causing JSON arrays to be ignored; update the logic in the block that checks
trimmed.startsWith("{") || trimmed.startsWith("[") so that after JSON.parse you
handle arrays as well: if parsed is an array, iterate its elements and call
detectFromJsonObject on any plain-record elements (or aggregate results and
return an error if any element indicates an error); alternatively, if arrays
should be unsupported, remove the trimmed.startsWith("[") check to make the
intent explicit; key symbols: the trimmed.startsWith check, JSON.parse,
isPlainRecord(parsed), detectFromJsonObject(parsed, ...), and the final return {
isError: false } to adjust.
🧹 Nitpick comments (5)
src/lib/utils/upstream-error-detection.ts (3)
50-52:isPlainRecord实现与session-messages-guards.ts中的同名函数不一致
session-messages-guards.ts中的版本额外检查了Object.prototype.toString.call(value) === "[object Object]"，能排除Date、RegExp等特殊对象。本文件的版本只检查typeof === "object"且非数组，对于边缘场景（如上游返回的 JSON 被反序列化后包含特殊对象）可能行为不同。建议统一提取为共享工具函数，避免两处实现漂移。
建议提取为共享实现
-function isPlainRecord(value: unknown): value is Record<string, unknown> { - return !!value && typeof value === "object" && !Array.isArray(value); -} +// 复用已有的 isPlainRecord 实现 +import { isPlainRecord } from "@/app/[locale]/dashboard/sessions/[sessionId]/messages/_components/session-messages-guards";或者将
session-messages-guards.ts中的实现迁移到@/lib/utils/下的共享模块，两处统一引用。
164-177: SSE 事件检测中对evt.data重新JSON.stringify计算字符数，性能可优化
parseSSEData已经从文本中解析出事件，但这里又将evt.data重新序列化以获取字符数（Line 170）。对于大量 SSE 事件的场景，这会增加不必要的序列化开销。考虑到这是流结束后的一次性检测，且
parseSSEData返回的是已解析对象（无法直接获取原始 JSON 长度），当前做法可接受，但值得留意。如果性能成为问题，可以在parseSSEData中同时返回原始 data 字符串长度。
101-115:message关键字检测仅检查顶层obj.message，未检查obj.error.message注释（Line 82）提到 "message / error.message 命中关键字" 两种场景，但代码在 Line 84-101 的
errorValue分支中，如果error.message存在，会在 Line 93-97 直接返回"非空错误"结果，不会走到 Line 103-113 的关键字检测。这意味着当
error字段非空时，无论error.message是否包含 "error" 关键字都会判定为错误（这是合理的强信号逻辑）。但如果error为空/不存在，则只检查了顶层message，没有检查嵌套的error.message——这在error字段为空对象{}或0等场景下，hasNonEmptyValue返回 false，此时error.message不会被检查。实际上这个边界很窄（
error: {}是空对象，hasNonEmptyValue返回 false），但注释与代码行为存在细微差异，建议更新注释以准确反映实际逻辑。src/app/v1/_lib/proxy/response-handler.ts (2)
101-103: 非正常结束时跳过假 200 检测的逻辑正确，但存在细微边界当
streamEndedNormally为false时，detected被强制设为{ isError: false }，意味着异常中断的流不会触发假 200 判定。这符合注释说明（Line 90-91）。但考虑一个边界场景：如果上游在发送了一条包含
error字段的 SSE 事件后立刻断开连接（streamEndedNormally = false），此时allContent中已经包含了错误信息，但因为非正常结束而被忽略。这种情况下，异常中断的处理逻辑（persistRequestFailure）会把它当作网络错误记录，而不是上游业务错误。这在当前保守策略下是合理的（避免误判），只是值得在注释中说明这个 trade-off。
134-145: 大量重复的动态import()调用模式可考虑提取
await import("@/lib/circuit-breaker")和await import("@/lib/endpoint-circuit-breaker")的 try/catch 模式在成功路径和失败路径中重复出现，且与文件其他位置（Lines 614-625, 1213-1224, 1249-1260）的模式完全一致。可以提取为辅助函数（如
safeRecordFailure(providerId, error)/safeRecordSuccess(providerId)），减少重复并统一错误处理。Also applies to: 148-159, 190-198
|
进一步按 review 意见加固（仅触及本 PR 新增/改动逻辑）：
关于 prompt injection/误判风险：按需求保留了“<1000 chars 且顶层 已验证： |
| type FinalizeDeferredStreamingResult = { | ||
| /** | ||
| * “内部结算用”的状态码。 | ||
| * | ||
| * 注意：这不会改变客户端实际收到的 HTTP 状态码（SSE 已经开始透传后无法回头改）。 | ||
| * 这里的目的仅是让内部统计/熔断/会话绑定把“假 200”按失败处理。 | ||
| */ | ||
| effectiveStatusCode: number; | ||
| /** | ||
| * 内部记录的错误原因（用于写入 DB/监控，帮助定位“假 200”问题）。 | ||
| */ | ||
| errorMessage: string | null; | ||
| }; | ||
|
|
||
| /** | ||
| * 若本次 SSE 被标记为“延迟结算”，则在流结束后补齐成功/失败的最终判定。 | ||
| * | ||
| * 触发条件 | ||
| * - Forwarder 收到 Response 且识别为 SSE 时，会在 session 上挂载 DeferredStreamingFinalization 元信息。 | ||
| * - ResponseHandler 在后台读取完整 SSE 内容后，调用本函数： | ||
| * - 如果内容看起来是上游错误 JSON（假 200），则： | ||
| * - 计入熔断器失败； | ||
| * - 不更新 session 智能绑定（避免把会话粘到坏 provider）； | ||
| * - 内部状态码改为 502（只影响统计与后续重试选择，不影响本次客户端响应）。 | ||
| * - 如果流正常结束且未命中错误判定，则按成功结算并更新绑定/熔断/endpoint 成功率。 | ||
| * | ||
| * @param streamEndedNormally - 必须是 reader 读到 done=true 的“自然结束”；超时/中断等异常结束由其它逻辑处理。 | ||
| */ | ||
| async function finalizeDeferredStreamingFinalizationIfNeeded( |
There was a problem hiding this comment.
if session.provider has changed between response start (when meta was set) and stream end, providerForChain (line 60) uses the current session.provider while circuit breaker and session updates use meta.providerId. this creates inconsistent attribution: addProviderToChain at lines 127-137 and 207-213 will record the current provider object, but success/failure recording uses meta.providerId
the fallback logic at lines 73-90 tries to resolve this, but providerForChain is set before the mismatch check, so the initial value is wrong
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/response-handler.ts
Line: 64:92
Comment:
if `session.provider` has changed between response start (when meta was set) and stream end, `providerForChain` (line 60) uses the *current* `session.provider` while circuit breaker and session updates use `meta.providerId`. this creates inconsistent attribution: `addProviderToChain` at lines 127-137 and 207-213 will record the *current* provider object, but success/failure recording uses `meta.providerId`
the fallback logic at lines 73-90 tries to resolve this, but `providerForChain` is set before the mismatch check, so the initial value is wrong
How can I resolve this? If you propose a fix, please make it concise.| // 情况 2：SSE 文本。快速过滤：既无 "error" 也无 "message" key 时跳过解析 | ||
| // 注意：这里用 key 形式的引号匹配，尽量避免模型正文里出现 error 造成的无谓解析。 | ||
| // 代价：如果上游返回的并非标准 JSON key（极少见），这里可能漏检；但我们偏向保守与低误判。 | ||
| // | ||
| // 额外说明：这里刻意只匹配 `"error"` / `"message"`（含双引号）， | ||
| // 若正文里出现被转义的 `\"error\"`（字符串内容），不会命中，这是为了避免误判。 | ||
| if (!text.includes("\"error\"") && !text.includes("\"message\"")) { | ||
| return { isError: false }; | ||
| } |
There was a problem hiding this comment.
quick filter may miss errors if SSE data contains "error" or "message" as quoted string content (not keys), e.g. data: {"content": "the word \"error\" appears here"} would incorrectly trigger parsing even though no error field exists
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/utils/upstream-error-detection.ts
Line: 197:205
Comment:
quick filter may miss errors if SSE data contains `"error"` or `"message"` as quoted string content (not keys), e.g. `data: {"content": "the word \"error\" appears here"}` would incorrectly trigger parsing even though no error field exists
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
consider forcing Prompt To Fix With AIThis is a comment left during a code review.
Path: src/app/v1/_lib/proxy/response-handler.ts
Line: 51:55
Comment:
when `!meta || !provider || !streamEndedNormally`, the function returns `upstreamStatusCode` (often 200) without triggering circuit breaker updates. this means aborted streams (where `finalizeStream(allContent, false)` is called at line 1339) will record the fake 200 as success in `finalizeRequestStats`, undermining the entire fake-200 detection
consider forcing `effectiveStatusCode = 502` or skipping stats updates when `streamEndedNormally=false`
How can I resolve this? If you propose a fix, please make it concise. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/app/v1/_lib/proxy/response-handler.ts`:
- Around line 190-200: Add a new reason value "request_failed" to the
provider-chain reason enum(s) (the type used in session.addProviderToChain / the
message reason enum) and update the error-path call site in response-handler.ts
(where session.addProviderToChain is invoked) to use meta.isFirstAttempt ?
"request_failed" : "retry_failed" instead of always "retry_failed"; then update
consumer UI/formatters (provider-chain-formatter, provider-chain-popover) to
handle and display the new "request_failed" reason.
- Around line 163-174: detected.reason is a user-facing string that flows into
triggerCircuitBreakerAlert → sendCircuitBreakerAlert →
buildCircuitBreakerMessage and must be internationalized; update the block that
calls recordFailure (dynamic import of recordFailure) to pass either an i18n key
plus params or a language-independent error code instead of raw detected.reason,
e.g., map the current detected.reason cases to an i18n key (via your
i18n.translate or similar) or to a stable errorCode property and pass that (and
any non-localized metadata) into recordFailure; keep the dynamic import of
recordFailure but ensure recordFailure receives only i18n keys/error codes (not
raw Chinese strings) so downstream alert builders can localize per user locale.
🧹 Nitpick comments (4)
src/lib/utils/upstream-error-detection.ts (2)
52-54:isPlainRecord与session-messages-guards.ts中的同名函数重复在
src/app/[locale]/dashboard/sessions/[sessionId]/messages/_components/session-messages-guards.ts中已有一个更严格的isPlainRecord实现（额外检查了Object.prototype.toString.call(value) === "[object Object]"）。虽然本文件的输入来源于JSON.parse，不会产生Date/Map/Set等非普通对象，因此当前简化版本在功能上没有问题，但建议考虑将这两个实现统一为一个共享的工具函数，避免日后维护不一致。
197-205: SSE 快速过滤使用text（未 trim）而 JSON 路径使用trimmed——逻辑正确但值得加注释Line 203 使用原始
text做includes检查，而非trimmed。这在功能上是正确的（trim()不会影响中间内容），但与上方 Line 177-185 使用trimmed的风格不一致，乍看可能让人疑惑。建议在此处补充一句简短注释说明使用text的原因（如：trim 后已进入 JSON 分支返回，此处一定不以{开头，直接用原始文本匹配更安全）。src/lib/utils/upstream-error-detection.test.ts (1)
4-119: 测试覆盖整体良好，建议补充两个小场景覆盖率已覆盖主要分支，不过以下两个场景如果顺手补上会更完整：
- 纯空白字符串（如
" "）—— 验证trim()后视为空的边界行为。- 自定义
options（如自定义messageKeyword或maxJsonCharsForMessageCheck）—— 确认可配置项生效。这些不影响合并，仅建议后续完善。
src/app/v1/_lib/proxy/response-handler.ts (1)
1335-1339: 客户端中断时finalizeStream(allContent, false)的语义需确认客户端主动中断后，仍调用
finalizeStream(allContent, false)进行统计写入。由于streamEndedNormally=false，finalizeDeferredStreamingFinalizationIfNeeded会跳过错误检测并返回effectiveStatusCode=200（upstreamStatusCode）。这意味着即使客户端中断、内容不完整，session usage 的 status 仍会被记为"completed"。如果这是预期行为（客户端中断不算上游失败），建议在注释中明确说明这一点，以免后续维护者误解。
| // 记录到决策链（用于日志展示与 DB 持久化）。 | ||
| // 注意：这里用 effectiveStatusCode（502）而不是 upstreamStatusCode（200）， | ||
| // 以便让内部链路明确显示这是一次失败（否则会被误读为成功）。 | ||
| session.addProviderToChain(providerForChain, { | ||
| endpointId: meta.endpointId, | ||
| endpointUrl: meta.endpointUrl, | ||
| reason: "retry_failed", | ||
| attemptNumber: meta.attemptNumber, | ||
| statusCode: effectiveStatusCode, | ||
| errorMessage: detected.reason, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for reason field type definitions and enum values
rg -n --type=ts -C3 '(request_success|retry_success|retry_failed|reason.*:)' src/app/v1/_lib/proxy/Repository: ding113/claude-code-hub
Length of output: 32527
🏁 Script executed:
# Check the response-handler.ts file to see line 275 and surrounding context
wc -l src/app/v1/_lib/proxy/response-handler.tsRepository: ding113/claude-code-hub
Length of output: 113
🏁 Script executed:
# Search for addProviderToChain method signature and usage patterns
rg -n --type=ts 'addProviderToChain' -C2Repository: ding113/claude-code-hub
Length of output: 9099
🏁 Script executed:
# Look for reason field type definition in broader scope
rg -n --type=ts '"request_failed"|"request_success"|"retry_' --max-count=50Repository: ding113/claude-code-hub
Length of output: 6922
错误路径的 reason 未区分首次请求与重试，与成功路径不对称
成功路径（Line 275）会根据 meta.isFirstAttempt 区分 "request_success" 和 "retry_success"，但错误路径始终使用 "retry_failed"。当 isFirstAttempt=true 时，provider chain 审计记录会显示 "retry_failed"，容易造成误读（明明是首次尝试却标记为 retry 失败）。
需要同时在 src/app/v1/_lib/proxy/session.ts 和 src/types/message.ts 的 reason 类型枚举中添加 "request_failed"，然后更新 line 196 的逻辑，并同步更新 provider-chain-formatter.ts、provider-chain-popover.tsx 等仪表盘展示代码以处理新的 reason 值。
建议的修改
1. 更新类型定义 (src/app/v1/_lib/proxy/session.ts lines 450-460)：
reason?:
| "session_reuse"
| "initial_selection"
| "concurrent_limit_failed"
| "request_success"
+ | "request_failed"
| "retry_success"
| "retry_failed"2. 更新错误路径 (src/app/v1/_lib/proxy/response-handler.ts line 196)：
session.addProviderToChain(providerForChain, {
endpointId: meta.endpointId,
endpointUrl: meta.endpointUrl,
- reason: "retry_failed",
+ reason: meta.isFirstAttempt ? "request_failed" : "retry_failed",
attemptNumber: meta.attemptNumber,
statusCode: effectiveStatusCode,
errorMessage: detected.reason,
});📝 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.
| // 记录到决策链（用于日志展示与 DB 持久化）。 | |
| // 注意：这里用 effectiveStatusCode（502）而不是 upstreamStatusCode（200）， | |
| // 以便让内部链路明确显示这是一次失败（否则会被误读为成功）。 | |
| session.addProviderToChain(providerForChain, { | |
| endpointId: meta.endpointId, | |
| endpointUrl: meta.endpointUrl, | |
| reason: "retry_failed", | |
| attemptNumber: meta.attemptNumber, | |
| statusCode: effectiveStatusCode, | |
| errorMessage: detected.reason, | |
| }); | |
| // 记录到决策链（用于日志展示与 DB 持久化）。 | |
| // 注意：这里用 effectiveStatusCode（502）而不是 upstreamStatusCode（200）， | |
| // 以便让内部链路明确显示这是一次失败（否则会被误读为成功）。 | |
| session.addProviderToChain(providerForChain, { | |
| endpointId: meta.endpointId, | |
| endpointUrl: meta.endpointUrl, | |
| reason: meta.isFirstAttempt ? "request_failed" : "retry_failed", | |
| attemptNumber: meta.attemptNumber, | |
| statusCode: effectiveStatusCode, | |
| errorMessage: detected.reason, | |
| }); |
🤖 Prompt for AI Agents
In `@src/app/v1/_lib/proxy/response-handler.ts` around lines 190 - 200, Add a new
reason value "request_failed" to the provider-chain reason enum(s) (the type
used in session.addProviderToChain / the message reason enum) and update the
error-path call site in response-handler.ts (where session.addProviderToChain is
invoked) to use meta.isFirstAttempt ? "request_failed" : "retry_failed" instead
of always "retry_failed"; then update consumer UI/formatters
(provider-chain-formatter, provider-chain-popover) to handle and display the new
"request_failed" reason.
|
补充按 review 意见继续加固（仅改本 PR 相关逻辑）：
已验证： |
| if (messageValue.length >= merged.maxJsonCharsForMessageCheck) { | ||
| chars = merged.maxJsonCharsForMessageCheck; // >= 阈值即可跳过 message 关键字判定 | ||
| } else { | ||
| try { | ||
| chars = JSON.stringify(evt.data).length; | ||
| } catch { | ||
| // stringify 失败时回退为近似值（仍保持“仅小体积 JSON 才做 message 检测”的意图） | ||
| chars = messageValue.length; | ||
| } | ||
| } |
There was a problem hiding this comment.
JSON.stringify failure fallback uses only message length, not full object size
when stringify fails (line 233), code sets chars = messageValue.length which may be much smaller than actual JSON size, incorrectly allowing message keyword detection on large objects
| if (messageValue.length >= merged.maxJsonCharsForMessageCheck) { | |
| chars = merged.maxJsonCharsForMessageCheck; // >= 阈值即可跳过 message 关键字判定 | |
| } else { | |
| try { | |
| chars = JSON.stringify(evt.data).length; | |
| } catch { | |
| // stringify 失败时回退为近似值（仍保持“仅小体积 JSON 才做 message 检测”的意图） | |
| chars = messageValue.length; | |
| } | |
| } | |
| } catch { | |
| // stringify 失败时按"超过阈值"处理,跳过 message 检测 | |
| chars = merged.maxJsonCharsForMessageCheck; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/utils/upstream-error-detection.ts
Line: 229:238
Comment:
`JSON.stringify` failure fallback uses only message length, not full object size
when stringify fails (line 233), code sets `chars = messageValue.length` which may be much smaller than actual JSON size, incorrectly allowing message keyword detection on large objects
```suggestion
} catch {
// stringify 失败时按"超过阈值"处理,跳过 message 检测
chars = merged.maxJsonCharsForMessageCheck;
}
```
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/app/v1/_lib/proxy/response-handler.ts`:
- Around line 168-178: When handling the "!streamEndedNormally" branch in
response-handler.ts (the block that currently calls session.addProviderToChain
with providerForChain and returns { effectiveStatusCode, errorMessage }), also
call the circuit-breaker failure recorder—invoke session.recordFailure(...) for
providerForChain using the same metadata (meta.endpointId, meta.endpointUrl,
meta.attemptNumber, effectiveStatusCode and errorMessage) before returning so
that unexpected upstream stream interruptions are persisted to the breaker
consistently with the timeout paths; ensure you pass the appropriate failure
reason (e.g., "system_error" or similar) to match the existing
addProviderToChain usage.
🧹 Nitpick comments (3)
src/lib/utils/upstream-error-detection.test.ts (1)
2-2: 可选：考虑使用@/路径别名按照编码规范，引用
src/目录下的文件应使用@/路径别名。此处为相邻文件的相对引用，影响不大，可根据项目偏好决定是否调整。-import { detectUpstreamErrorFromSseOrJsonText } from "./upstream-error-detection"; +import { detectUpstreamErrorFromSseOrJsonText } from "@/lib/utils/upstream-error-detection";As per coding guidelines,
**/*.{ts,tsx,js,jsx}: "Use path alias@/to reference files in./src/directory".src/lib/utils/upstream-error-detection.ts (2)
61-63:isPlainRecord与session-messages-guards.ts中的实现重复
src/app/[locale]/dashboard/sessions/[sessionId]/messages/_components/session-messages-guards.ts中有一个更严格的isPlainRecord（额外检查了Object.prototype.toString.call）。两者对JSON.parse的输出行为一致，但存在代码重复。可以考虑提取到共享的 utils 中，但鉴于两个模块（dashboard 组件 vs proxy 内部工具）耦合度低，保持各自独立也可以接受。
80-114: 脱敏正则覆盖面不错，但sanitizeErrorTextForDetail未导出也无独立测试当前脱敏逻辑通过集成测试（test 文件中 Bearer token / API key 两个用例）间接覆盖，但 JWT、email、通用 key-value、配置路径等分支没有直接测试。如果这些脱敏模式在生产中被绕过，可能导致敏感信息泄露到日志/DB。建议补充针对这些模式的测试用例。
| const finalizeStream = async ( | ||
| allContent: string, | ||
| streamEndedNormally: boolean, | ||
| clientAborted: boolean | ||
| ): Promise<void> => { | ||
| const finalized = await finalizeDeferredStreamingFinalizationIfNeeded( | ||
| session, | ||
| allContent, | ||
| statusCode, | ||
| streamEndedNormally, | ||
| clientAborted | ||
| ); |
There was a problem hiding this comment.
Double-read of stream body
handleStream() uses controllableStream.tee() to create clientStream and internalStream for background processing, but the Gemini “stream passthrough” branch above separately spins up a background task that reads responseForStats.body.getReader() (clone’s body) and also returns the original response. If this branch is reachable inside handleStream(), you end up creating two independent background readers per request (one for the clone, one for the tee’d internal stream), doubling memory/CPU and potentially causing inconsistent finalization. Consider ensuring only one stats/finalization path runs for any given stream.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/response-handler.ts
Line: 1057:1068
Comment:
**Double-read of stream body**
`handleStream()` uses `controllableStream.tee()` to create `clientStream` and `internalStream` for background processing, but the Gemini “stream passthrough” branch above separately spins up a background task that reads `responseForStats.body.getReader()` (clone’s body) and *also* returns the original `response`. If this branch is reachable inside `handleStream()`, you end up creating two independent background readers per request (one for the clone, one for the tee’d internal stream), doubling memory/CPU and potentially causing inconsistent finalization. Consider ensuring only one stats/finalization path runs for any given stream.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/app/v1/_lib/proxy/response-handler.ts
Line: 860:869
Comment:
**Ignores response-body storage flag**
`SessionManager.storeSessionResponse()` already no-ops when `STORE_SESSION_RESPONSE_BODY` is false, but `handleStream()` still unconditionally builds and passes the full `allContent` string into that call (and also into the stats path) even when storage is disabled. This defeats the stated goal (“no persistence”) for large SSE responses because memory is still accumulated for the entire stream. If `STORE_SESSION_RESPONSE_BODY=false` is intended to reduce memory footprint (not just Redis writes), you need to gate the `chunks` accumulation / `allContent` construction accordingly; otherwise the env var doesn’t actually prevent large in-memory buffering in the streaming path.
How can I resolve this? If you propose a fix, please make it concise. |
| }); | ||
| } | ||
|
|
||
| void SessionManager.updateSessionUsage(session.sessionId, { | ||
| inputTokens: usageForCost.input_tokens, | ||
| outputTokens: usageForCost.output_tokens, | ||
| cacheCreationInputTokens: usageForCost.cache_creation_input_tokens, | ||
| cacheReadInputTokens: usageForCost.cache_read_input_tokens, | ||
| costUsd: costUsdStr, | ||
| status: statusCode >= 200 && statusCode < 300 ? "completed" : "error", | ||
| statusCode: statusCode, | ||
| }).catch((error: unknown) => { | ||
| const payload: SessionUsageUpdate = { | ||
| status: effectiveStatusCode >= 200 && effectiveStatusCode < 300 ? "completed" : "error", | ||
| statusCode: effectiveStatusCode, | ||
| ...(streamErrorMessage ? { errorMessage: streamErrorMessage } : {}), | ||
| }; | ||
|
|
||
| if (usageForCost) { | ||
| payload.inputTokens = usageForCost.input_tokens; | ||
| payload.outputTokens = usageForCost.output_tokens; | ||
| payload.cacheCreationInputTokens = usageForCost.cache_creation_input_tokens; | ||
| payload.cacheReadInputTokens = usageForCost.cache_read_input_tokens; | ||
| payload.costUsd = costUsdStr; | ||
| } |
There was a problem hiding this comment.
Redundant conditional nesting
the outer if (session.sessionId) already establishes that session.sessionId is truthy, so the inner check at line 1154 if (usageForCost && session.request.model) doesn't need to recheck session.sessionId
| }); | |
| } | |
| void SessionManager.updateSessionUsage(session.sessionId, { | |
| inputTokens: usageForCost.input_tokens, | |
| outputTokens: usageForCost.output_tokens, | |
| cacheCreationInputTokens: usageForCost.cache_creation_input_tokens, | |
| cacheReadInputTokens: usageForCost.cache_read_input_tokens, | |
| costUsd: costUsdStr, | |
| status: statusCode >= 200 && statusCode < 300 ? "completed" : "error", | |
| statusCode: statusCode, | |
| }).catch((error: unknown) => { | |
| const payload: SessionUsageUpdate = { | |
| status: effectiveStatusCode >= 200 && effectiveStatusCode < 300 ? "completed" : "error", | |
| statusCode: effectiveStatusCode, | |
| ...(streamErrorMessage ? { errorMessage: streamErrorMessage } : {}), | |
| }; | |
| if (usageForCost) { | |
| payload.inputTokens = usageForCost.input_tokens; | |
| payload.outputTokens = usageForCost.output_tokens; | |
| payload.cacheCreationInputTokens = usageForCost.cache_creation_input_tokens; | |
| payload.cacheReadInputTokens = usageForCost.cache_read_input_tokens; | |
| payload.costUsd = costUsdStr; | |
| } | |
| if (usageForCost && session.request.model) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/response-handler.ts
Line: 1151:1166
Comment:
Redundant conditional nesting
the outer `if (session.sessionId)` already establishes that `session.sessionId` is truthy, so the inner check at line 1154 `if (usageForCost && session.request.model)` doesn't need to recheck `session.sessionId`
```suggestion
if (usageForCost && session.request.model) {
```
How can I resolve this? If you propose a fix, please make it concise.| const errorValue = evt.data.error; | ||
| const messageValue = evt.data.message; | ||
| if (!hasNonEmptyValue(errorValue) && typeof messageValue === "string") { | ||
| if (messageValue.length >= merged.maxJsonCharsForMessageCheck) { | ||
| chars = merged.maxJsonCharsForMessageCheck; // >= 阈值即可跳过 message 关键字判定 | ||
| } else { | ||
| try { | ||
| chars = JSON.stringify(evt.data).length; | ||
| } catch { | ||
| // stringify 失败时回退为近似值（仍保持“仅小体积 JSON 才做 message 检测”的意图） | ||
| chars = messageValue.length; | ||
| } | ||
| } |
There was a problem hiding this comment.
Performance optimization could skip stringify in more cases
when errorValue is non-empty (line 226), you know the result will be error regardless of JSON size, so the stringify at line 233 is unnecessary. only compute chars when both conditions are true: !hasNonEmptyValue(errorValue) AND typeof messageValue === "string"
| const errorValue = evt.data.error; | |
| const messageValue = evt.data.message; | |
| if (!hasNonEmptyValue(errorValue) && typeof messageValue === "string") { | |
| if (messageValue.length >= merged.maxJsonCharsForMessageCheck) { | |
| chars = merged.maxJsonCharsForMessageCheck; // >= 阈值即可跳过 message 关键字判定 | |
| } else { | |
| try { | |
| chars = JSON.stringify(evt.data).length; | |
| } catch { | |
| // stringify 失败时回退为近似值（仍保持“仅小体积 JSON 才做 message 检测”的意图） | |
| chars = messageValue.length; | |
| } | |
| } | |
| let chars = 0; | |
| const errorValue = evt.data.error; | |
| const messageValue = evt.data.message; | |
| // Only compute JSON size when we need message keyword check | |
| if (!hasNonEmptyValue(errorValue) && typeof messageValue === "string") { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/utils/upstream-error-detection.ts
Line: 226:238
Comment:
Performance optimization could skip stringify in more cases
when `errorValue` is non-empty (line 226), you know the result will be error regardless of JSON size, so the stringify at line 233 is unnecessary. only compute `chars` when both conditions are true: `!hasNonEmptyValue(errorValue)` AND `typeof messageValue === "string"`
```suggestion
let chars = 0;
const errorValue = evt.data.error;
const messageValue = evt.data.message;
// Only compute JSON size when we need message keyword check
if (!hasNonEmptyValue(errorValue) && typeof messageValue === "string") {
```
How can I resolve this? If you propose a fix, please make it concise.- streamEndedNormally=false 且非 client abort：记录 provider/endpoint failure\n- 补充注释说明 STORE_SESSION_RESPONSE_BODY 仅影响持久化\n- upstream-error-detection 增补脱敏/截断单测
|
继续跟进 AI review（仅触及本 PR 新增/改动逻辑）：
未采纳：新增 provider-chain reason="request_failed"（会牵涉多处类型与 UI 文案扩展；当前已有 attemptNumber/isFirstAttempt 等字段可区分语义，若 maintainer 认为必要可单独 PR 推进）。 本地验证：npm test / npm run typecheck 通过。 |
| @@ -887,13 +1220,15 @@ export class ProxyResponseHandler { | |||
| cacheCreation1hInputTokens: usageForCost?.cache_creation_1h_input_tokens, | |||
| cacheTtlApplied: usageForCost?.cache_ttl ?? null, | |||
| providerChain: session.getProviderChain(), | |||
| ...(streamErrorMessage ? { errorMessage: streamErrorMessage } : {}), | |||
| model: session.getCurrentModel() ?? undefined, // ⭐ 更新重定向后的模型 | |||
| providerId: session.provider?.id, // ⭐ 更新最终供应商ID（重试切换后） | |||
| context1mApplied: session.getContext1mApplied(), | |||
| }); | |||
There was a problem hiding this comment.
Provider attribution mismatch
In deferred streaming finalization, circuit-breaker + session.addProviderToChain(...) are recorded against meta.providerId, but request details are persisted with providerId: session.provider?.id. For the fake-200 path you intentionally skip session binding updates, so session.provider can remain pointing at a different provider, making providerId inconsistent with the providerChain/stats for the same request. Consider writing providerId from the deferred meta (or the resolved providerForChain.id) here so DB/UI attribution matches the finalization target.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/response-handler.ts
Line: 1212:1227
Comment:
**Provider attribution mismatch**
In deferred streaming finalization, circuit-breaker + `session.addProviderToChain(...)` are recorded against `meta.providerId`, but request details are persisted with `providerId: session.provider?.id`. For the fake-200 path you intentionally skip session binding updates, so `session.provider` can remain pointing at a different provider, making `providerId` inconsistent with the providerChain/stats for the same request. Consider writing `providerId` from the deferred meta (or the resolved `providerForChain.id`) here so DB/UI attribution matches the finalization target.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/v1/_lib/proxy/response-handler.ts (1)
1374-1393:⚠️ Potential issue | 🟠 Major上游意外中断路径缺少熔断器记录和延迟结算调用
在 catch 分支中，当判定为上游意外中断（
isClientAbortError && !clientAborted && !isResponseTimeout && !isIdleTimeout）时，仅调用了persistRequestFailure，但：
- 未调用
finalizeStream（或finalizeDeferredStreamingFinalizationIfNeeded），导致延迟结算元数据未被消费，circuit breaker 不会被更新。- 未直接调用
recordFailure，与响应超时（line 1314）和静默期超时（line 1350）路径不对称。相比之下，客户端主动中断路径（line 1408-1410）正确调用了
finalizeStream(allContent, false, true)。建议与客户端中断路径保持一致，在此处补充
finalizeStream调用或直接添加recordFailure：建议的修复
} else if (!clientAborted) { // 上游在流式过程中意外中断：视为供应商/网络错误 logger.error("ResponseHandler: Upstream stream aborted unexpectedly", { taskId, providerId: provider.id, providerName: provider.name, messageId: messageContext.id, chunksCollected: chunks.length, errorName: err.name, errorMessage: err.message || "(empty message)", }); + try { + const allContent = flushAndJoin(); + await finalizeStream(allContent, false, false); + } catch (finalizeError) { + logger.error("ResponseHandler: Failed to finalize upstream-aborted stream", { + taskId, + messageId: messageContext.id, + finalizeError, + }); + } + await persistRequestFailure({ session, messageContext, statusCode: 502, error: err, taskId, phase: "stream", });
🧹 Nitpick comments (1)
src/app/v1/_lib/proxy/response-handler.ts (1)
103-105:!streamEndedNormally时跳过错误检测，返回{isError: false}— 语义上可能令人困惑当流未正常结束时，
detected被设为{isError: false}，但后续!streamEndedNormally分支（line 172）仍将其视为失败。逻辑上没有 bug，但isError: false搭配实际被当作失败处理的路径，在阅读时容易造成误解。建议加一行简短注释说明此处isError: false的含义是"跳过假200检测，由后续分支按中断处理"。
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/v1/_lib/proxy/response-handler.ts (1)
1288-1395:⚠️ Potential issue | 🔴 Critical三个异常路径（响应超时/空闲超时/上游中断）缺少 deferred meta 消费，导致 provider chain 记录丢失。
在 catch 分支中，response timeout（line 1338）、idle timeout（line 1375）和上游意外中断（line 1391）三条路径仅调用
persistRequestFailure()但未调用finalizeStream()。而finalizeDeferredStreamingFinalizationIfNeeded()只在finalizeStream()内被调用（line 1100），因此这些路径无法消费 forwarder 设置的 deferred meta，导致无法通过addProviderToChain()记录 provider 决策链。建议在这三条路径中补充调用
finalizeStream()或直接调用finalizeDeferredStreamingFinalizationIfNeeded()并适当处理 allContent，确保异常流式响应也能完整记录 provider chain 信息。
🧹 Nitpick comments (3)
src/lib/utils/upstream-error-detection.test.ts (2)
2-2: 建议使用@/路径别名引用同目录模块。编码规范要求使用
@/别名引用./src/下的文件。建议修改
-import { detectUpstreamErrorFromSseOrJsonText } from "./upstream-error-detection"; +import { detectUpstreamErrorFromSseOrJsonText } from "@/lib/utils/upstream-error-detection";As per coding guidelines,
**/*.{ts,tsx,js,jsx}: "Use path alias@/to reference files in./src/directory".
149-188: SSE 场景测试覆盖合理。error 字段、error 对象、message 关键字、message 为对象不误判、无 error/message key、
[DONE]等关键路径均已测试。建议后续可补充两个边缘用例以进一步提升覆盖率：
- 纯空白字符串输入（如
" "）—— 验证trim()后走空体逻辑。- 自定义
options（如自定义messageKeyword或maxJsonCharsForMessageCheck）—— 验证配置覆盖生效。src/app/v1/_lib/proxy/response-handler.ts (1)
93-99: 函数命名冗余：finalizeDeferredStreamingFinalizationIfNeeded函数名中 "Finalize...Finalization" 存在语义重复，建议简化为
finalizeDeferredStreamingIfNeeded或handleDeferredStreamingFinalization。
| if (!hasNonEmptyValue(errorValue) && typeof messageValue === "string") { | ||
| if (messageValue.length >= merged.maxJsonCharsForMessageCheck) { | ||
| chars = merged.maxJsonCharsForMessageCheck; // >= 阈值即可跳过 message 关键字判定 | ||
| } else { | ||
| try { | ||
| chars = JSON.stringify(evt.data).length; | ||
| } catch { | ||
| // stringify 失败时回退为近似值（仍保持“仅小体积 JSON 才做 message 检测”的意图） | ||
| chars = messageValue.length; |
There was a problem hiding this comment.
could optimize by skipping stringify when error already detected
when errorValue is non-empty (line 84-147), result is error regardless of JSON size, so computing chars via JSON.stringify on line 233 is unnecessary work. only compute when both conditions need checking: !hasNonEmptyValue(errorValue) AND message keyword detection is needed
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/utils/upstream-error-detection.ts
Line: 226:234
Comment:
could optimize by skipping stringify when error already detected
when `errorValue` is non-empty (line 84-147), result is error regardless of JSON size, so computing `chars` via `JSON.stringify` on line 233 is unnecessary work. only compute when both conditions need checking: `!hasNonEmptyValue(errorValue)` AND message keyword detection is needed
How can I resolve this? If you propose a fix, please make it concise.- 超时/中断/异常路径也调用 finalizeStream，补齐 providerChain 与熔断归因\n- 持久化 providerId 优先使用 deferred meta，避免假200/异常流归因错位\n- upstream-error-detection 增补 options/空白输入用例
|
已按最新 CodeRabbit/Greptile 建议补齐（仅涉及本 PR 新增/改动逻辑）：
本地验证： 备注：上游仓库最新两次 GitHub Actions 运行显示 �ction_required（0s/无 job），需要 maintainer 在 UI 里批准工作流运行后才会产出 CI checks。 |
| try { | ||
| chars = JSON.stringify(evt.data).length; | ||
| } catch { | ||
| // stringify 失败时回退为近似值（仍保持“仅小体积 JSON 才做 message 检测”的意图） |
There was a problem hiding this comment.
unnecessary work when error already detected
JSON.stringify is expensive - if hasNonEmptyValue(errorValue) is true (lines 128-147), the function will return error regardless of JSON size, so computing chars here serves no purpose
skip stringify when error detected:
| // stringify 失败时回退为近似值（仍保持“仅小体积 JSON 才做 message 检测”的意图） | |
| const messageValue = evt.data.message; | |
| if (hasNonEmptyValue(errorValue)) { | |
| chars = 0; // error already detected, skip message check | |
| } else if (typeof messageValue === "string") { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/utils/upstream-error-detection.ts
Line: 233:233
Comment:
unnecessary work when error already detected
`JSON.stringify` is expensive - if `hasNonEmptyValue(errorValue)` is true (lines 128-147), the function will return error regardless of JSON size, so computing `chars` here serves no purpose
skip stringify when error detected:
```suggestion
const messageValue = evt.data.message;
if (hasNonEmptyValue(errorValue)) {
chars = 0; // error already detected, skip message check
} else if (typeof messageValue === "string") {
```
How can I resolve this? If you propose a fix, please make it concise.| // 情况 2：SSE 文本。快速过滤：既无 "error" 也无 "message" key 时跳过解析 | ||
| // 注意：这里用 key 形式的引号匹配，尽量避免模型正文里出现 error 造成的无谓解析。 | ||
| // 代价：如果上游返回的并非标准 JSON key（极少见），这里可能漏检；但我们偏向保守与低误判。 | ||
| // | ||
| // 额外说明：这里刻意只匹配 `"error"` / `"message"`（含双引号）， |
There was a problem hiding this comment.
quick filter can cause false positives
the check text.includes('"error"') will match legitimate assistant responses like:
data: {"content": "to handle \"error\" cases, use try-catch"}
this triggers unnecessary SSE parsing for most model responses that discuss error handling
tradeoff noted in comments, but consider tracking false positive rate in production
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/utils/upstream-error-detection.ts
Line: 207:211
Comment:
quick filter can cause false positives
the check `text.includes('"error"')` will match legitimate assistant responses like:
```
data: {"content": "to handle \"error\" cases, use try-catch"}
```
this triggers unnecessary SSE parsing for most model responses that discuss error handling
tradeoff noted in comments, but consider tracking false positive rate in production
How can I resolve this? If you propose a fix, please make it concise.| let providerForChain = provider; | ||
| if (provider.id !== meta.providerId) { | ||
| logger.warn("[ResponseHandler] Deferred streaming meta provider mismatch", { | ||
| sessionId: session.sessionId ?? null, | ||
| metaProviderId: meta.providerId, | ||
| currentProviderId: provider.id, | ||
| canonicalProviderId: meta.providerId, | ||
| }); | ||
|
|
||
| // 尝试用 meta.providerId 找回正确的 Provider 对象，保证 providerChain 的审计数据一致 | ||
| try { | ||
| const providers = await session.getProvidersSnapshot(); | ||
| const resolved = providers.find((p) => p.id === meta.providerId); | ||
| if (resolved) { | ||
| providerForChain = resolved; | ||
| } else { | ||
| logger.warn("[ResponseHandler] Deferred streaming meta provider not found in snapshot", { | ||
| sessionId: session.sessionId ?? null, | ||
| metaProviderId: meta.providerId, | ||
| currentProviderId: provider.id, | ||
| }); | ||
| } | ||
| } catch (resolveError) { | ||
| logger.warn("[ResponseHandler] Failed to resolve meta provider from snapshot", { | ||
| sessionId: session.sessionId ?? null, | ||
| metaProviderId: meta.providerId, | ||
| currentProviderId: provider.id, | ||
| error: resolveError, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
verify provider resolution doesn't fail silently
when session.provider.id !== meta.providerId, the code attempts to resolve the correct provider from snapshot. if resolution fails (provider not in snapshot or error thrown), providerForChain remains as the mismatched provider, causing inconsistent attribution
verify circuit breaker / session updates use meta.providerId directly (not providerForChain.id) to ensure stats accuracy
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/response-handler.ts
Line: 148:178
Comment:
verify provider resolution doesn't fail silently
when `session.provider.id !== meta.providerId`, the code attempts to resolve the correct provider from snapshot. if resolution fails (provider not in snapshot or error thrown), `providerForChain` remains as the mismatched provider, causing inconsistent attribution
verify circuit breaker / session updates use `meta.providerId` directly (not `providerForChain.id`) to ensure stats accuracy
How can I resolve this? If you propose a fix, please make it concise.| providerId: session.provider?.id, // ⭐ 更新最终供应商ID（重试切换后） | ||
| ...(streamErrorMessage ? { errorMessage: streamErrorMessage } : {}), | ||
| model: session.getCurrentModel() ?? undefined, // 更新重定向后的模型 | ||
| providerId: providerIdForPersistence ?? session.provider?.id, // 更新最终供应商ID（重试切换后） |
There was a problem hiding this comment.
DB attribution should match circuit breaker
for fake-200 path, circuit breaker uses meta.providerId (line 240), but here providerId falls back to session.provider?.id which could differ
use providerIdForPersistence from finalized result:
| providerId: providerIdForPersistence ?? session.provider?.id, // 更新最终供应商ID（重试切换后） | |
| providerId: providerIdForPersistence ?? session.provider?.id, // 更新最终供应商ID（重试切换后） |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/response-handler.ts
Line: 1244:1244
Comment:
DB attribution should match circuit breaker
for fake-200 path, circuit breaker uses `meta.providerId` (line 240), but here `providerId` falls back to `session.provider?.id` which could differ
use `providerIdForPersistence` from finalized result:
```suggestion
providerId: providerIdForPersistence ?? session.provider?.id, // 更新最终供应商ID（重试切换后）
```
How can I resolve this? If you propose a fix, please make it concise.
|
Greptile encountered an error while reviewing this PR. Please reach out to support@greptile.com for assistance. |
|
补充：我这边本地已验证 |
github-project-automation
bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
2 participants
背景
部分上游在鉴权/配额/风控等失败场景下会返回 HTTP 200，但响应体却是错误 JSON（“假 200”）。在 SSE 已开始透传 后，CCH 无法再把本次响应的 HTTP 状态码改回 4xx/5xx，进而导致：
方案
对 SSE 场景引入“延迟结算”：
error/message做启发式判断（不扫描模型正文如content/choices，降低误判）error字段非空 => 视为错误message命中/error/i=> 视为错误effectiveStatusCode=502计入 provider/endpoint 失败与请求统计，并 跳过 session 绑定更新（避免粘到坏 provider）变更点
src/app/v1/_lib/proxy/forwarder.ts：识别 SSE 时写入一次性 DeferredStreamingFinalization 元信息src/app/v1/_lib/proxy/response-handler.ts：流结束后补齐成功/失败结算（熔断/endpoint/绑定/统计）src/lib/utils/upstream-error-detection.ts：新增假 200 检测工具STORE_SESSION_RESPONSE_BODY（默认true）false时不将会话响应体写入 Redis（不落盘/不持久化）测试
npm testnpm run typecheck限制
Greptile Overview
Greptile Summary
This PR implements deferred finalization for SSE responses to detect "fake 200" errors (HTTP 200 with error JSON body). The approach is architecturally sound:
Core Changes:
errorfield,messagekeyword matching), then updates circuit breakers, session binding, and provider chains accordinglyImplementation Quality:
Remaining Concerns:
upstream-error-detection.ts:207may have false positives when"error"or"message"appear in quoted string contentJSON.stringifyperformance optimization could skip unnecessary work when error is already detected (line 233)Confidence Score: 4/5
src/lib/utils/upstream-error-detection.tsfor the quick filter optimization andsrc/app/v1/_lib/proxy/response-handler.tsfor provider attribution consistencyImportant Files Changed
Sequence Diagram
sequenceDiagram participant Client participant Forwarder participant Upstream participant ResponseHandler participant ErrorDetection participant CircuitBreaker participant SessionManager Client->>Forwarder: POST /v1/chat/completions Forwarder->>Upstream: Forward request Upstream-->>Forwarder: HTTP 200 + SSE headers Note over Forwarder: Detect Content-Type: text/event-stream Forwarder->>Forwarder: setDeferredStreamingFinalization()<br/>(store providerId, endpointId, etc) Note over Forwarder: Skip immediate success recording<br/>Return response to start streaming Forwarder-->>Client: HTTP 200 + Start SSE stream Forwarder->>ResponseHandler: Pass response for handling ResponseHandler->>ResponseHandler: Create tee() streams<br/>(clientStream + internalStream) ResponseHandler-->>Client: Stream data chunks<br/>(via clientStream) par Background processing ResponseHandler->>ResponseHandler: Read internalStream<br/>Accumulate all content Upstream-->>ResponseHandler: data: {"error":"no credentials"} Upstream-->>ResponseHandler: [stream ends] ResponseHandler->>ResponseHandler: consumeDeferredStreamingFinalization()<br/>(get metadata, clear WeakMap) ResponseHandler->>ErrorDetection: detectUpstreamErrorFromSseOrJsonText(allContent) Note over ErrorDetection: Check empty body<br/>Check "error" field<br/>Check "message" keyword (if <1000 chars) ErrorDetection-->>ResponseHandler: {isError: true, code: "FAKE_200_..."} alt Fake 200 detected ResponseHandler->>ResponseHandler: effectiveStatusCode = 502<br/>(internal use only) ResponseHandler->>CircuitBreaker: recordFailure(meta.providerId) ResponseHandler->>CircuitBreaker: recordEndpointFailure(meta.endpointId) ResponseHandler->>ResponseHandler: addProviderToChain(...)<br/>(reason: "retry_failed") Note over ResponseHandler,SessionManager: Skip session binding update<br/>(avoid sticking to bad provider) else Stream ended normally, no error ResponseHandler->>CircuitBreaker: recordSuccess(meta.providerId) ResponseHandler->>CircuitBreaker: recordEndpointSuccess(meta.endpointId) ResponseHandler->>SessionManager: updateSessionBindingSmart() ResponseHandler->>ResponseHandler: addProviderToChain(...)<br/>(reason: "request_success") end ResponseHandler->>SessionManager: storeSessionResponse(allContent)<br/>(if STORE_SESSION_RESPONSE_BODY=true) ResponseHandler->>ResponseHandler: updateMessageRequestDetails()<br/>(with effectiveStatusCode) end Note over Client: Receives complete stream<br/>(may include error content)