Skip to content

fix(proxy): enforce structured stream fallback boundaries - #1373

Merged
ding113 merged 2 commits into
devfrom
fix/stream-precommit-fallback-i18n
Aug 1, 2026
Merged

fix(proxy): enforce structured stream fallback boundaries#1373
ding113 merged 2 commits into
devfrom
fix/stream-precommit-fallback-i18n

Conversation

@ding113

@ding113 ding113 commented Aug 1, 2026

Copy link
Copy Markdown
Owner

Summary

  • align enforced stream gating and Discovery/affinity on the same first-valid-content classifier
  • fall back without exposing buffered bytes when malformed/error/transport abort happens before real content
  • keep postcommit malformed bytes flowing to the client without splicing a fallback response
  • make malformed streams Replay-ineligible while keeping observer resource exhaustion as observation_incomplete
  • add the missing cloud_official pricing-source translations in all five locales and both log namespaces

Root cause

The two streaming decision paths had drifted on what counted as committed content. Lifecycle metadata and empty structured blocks could mark an attempt ready before the first real content frame, so later protocol or transport failures were already past the reversible fallback boundary.

The asynchronous response observer was also incomplete across native Gemini paths, conflated local observation limits with upstream malformed frames, and accepted terminal usage evidence too broadly. That allowed some malformed streams to bypass fallback or be settled with early, empty, zero, or input-only usage evidence.

Behavior after this change

  • Before the first real content frame:
    • malformed frames, protocol error frames, and upstream transport aborts trigger the next provider
    • the failed attempt's buffered prefix is discarded
    • the client receives zero bytes from that attempt
  • After real content is committed:
    • malformed frames and subsequent upstream bytes continue to pass through
    • the gateway does not interrupt the downstream stream and does not start fallback mid-response
    • a naturally completed stream is successful and billable only when the protocol terminal event contains positive output-token usage
    • early usage, empty usage, all-zero usage, and input-only usage do not establish success
  • Replay:
    • any observed malformed frame immediately aborts Replay eligibility, even when terminal usage allows successful billing
    • observation_incomplete is not treated as malformed and does not block a normally successful Replay entry
    • the observer retains up to 10 MiB, with a separate bounded request-echo allowance
  • Gemini:
    • native bytes are observed before passthrough or conversion
    • raw NDJSON terminal usage is supported
    • passthrough accumulation no longer races finalization

i18n

Added cloud_official to:

  • dashboard.logs.details.billingDetails.pricingSource
  • dashboard.logs.billingDetails.pricingSource

Locales: en, zh-CN, zh-TW, ja, ru.

Validation

  • bun run lint:fix
  • bun run lint
  • bun run typecheck
  • git diff --check
  • bun run test: 828 test files passed, 8016 tests passed, 2 files / 13 tests skipped
  • bun run build
  • bun run i18n:audit-messages-no-emoji:fail
  • focused stream terminal and hedge lifecycle tests: 45 passed

bun run i18n:audit-placeholders:fail still reports the existing ja and zh-TW same_as_zh-CN baseline entries. Neither new cloud_official translation appears in that failure list.

Greptile Summary

The PR aligns Discovery, stream gating, finalization, and Replay eligibility around a shared structured-frame classifier.

  • Defers stream commitment until protocol-specific content arrives and preserves fallback before that boundary.
  • Distinguishes malformed upstream frames from incomplete local observation.
  • Requires terminal positive output usage before accepting naturally completed postcommit-malformed streams.
  • Extends native Gemini observation and adds cloud_official pricing-source translations across all locales.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
src/app/v1/_lib/proxy/stream-gate/frame-classifier.ts Centralizes protocol-specific content, error, malformed, and terminal classification, including Gemini response-envelope handling.
src/app/v1/_lib/proxy/stream-gate/sse-frames.ts Reworks incremental SSE and NDJSON framing with bounded buffering, request-echo exemptions, and cross-chunk CR/LF handling.
src/app/v1/_lib/proxy/stream-gate/stream-protocol-observer.ts Separates incomplete local observation from upstream malformed frames and records whether failures occur after content.
src/app/v1/_lib/proxy/discovery-validity.ts Reuses the shared frame classifier so Discovery readiness and enforced stream gating apply consistent content boundaries.
src/app/v1/_lib/proxy/forwarder.ts Selects the provider-native protocol family when classifying raw Discovery stream bytes.
src/app/v1/_lib/proxy/response-handler.ts Adds terminal usage validation for postcommit malformed streams and prevents malformed observations from becoming Replay sources.

Sequence Diagram

sequenceDiagram
  participant Client
  participant Gate as Stream Gate
  participant Upstream
  participant Observer as Protocol Observer
  participant Finalizer
  participant Fallback
  Upstream-->>Gate: Structured stream frames
  Gate->>Observer: Observe native bytes
  alt Failure before first content
    Observer-->>Gate: malformed/error
    Gate->>Fallback: Discard buffered prefix and retry
    Fallback-->>Client: Next provider response
  else First valid content
    Gate-->>Client: Commit buffered stream
    Upstream-->>Client: Continue passthrough
    Observer->>Finalizer: Terminal observation
    Finalizer->>Finalizer: Validate completion and output usage
    Finalizer->>Finalizer: Settle billing, affinity, and Replay eligibility
  end
Loading

Reviews (2): Last reviewed commit: "fix(proxy): address stream review edge c..." | Re-trigger Greptile

Context used (3)

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@coderabbitai

coderabbitai Bot commented Aug 1, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

本次变更统一多协议流式帧分类和 SSE 解析,更新协议观察、流式结算及 Replay 资格判断,扩展回退生命周期测试,并为五种语言补充 cloud_official 定价来源翻译。

Changes

流式协议与本地化

Layer / File(s) Summary
帧分类与 Discovery 有效性
src/app/v1/_lib/proxy/discovery-validity.ts, src/app/v1/_lib/proxy/forwarder.ts, src/app/v1/_lib/proxy/stream-gate/frame-classifier.ts, tests/unit/proxy/discovery-validity.test.ts, tests/unit/proxy/stream-gate-frame-classifier.test.ts
统一多协议帧分类。Discovery 解析器现在处理 SSE 事件、裸 JSON、协议错误和结构化载荷。
SSE 解析与协议观察
src/app/v1/_lib/proxy/stream-gate/sse-frames.ts, src/app/v1/_lib/proxy/stream-gate/stream-protocol-observer.ts, tests/unit/proxy/stream-gate-sse-frames.test.ts, tests/unit/proxy/stream-gate-protocol-observer.test.ts
解析器支持增量分片、CRLF、裸 JSON 和缓冲区豁免。观察器支持 request echo,并将解析异常标记为观察不完整。
流式结算与 Replay 处理
src/app/v1/_lib/proxy/response-handler.ts, tests/integration/proxy-hedge-lifecycle.test.ts, tests/unit/proxy/response-handler-stream-terminal.test.ts, tests/unit/proxy/response-handler-endpoint-circuit-isolation.test.ts
共享协议观察结果,识别终态 usage,区分提交前后协议异常,并更新 Replay 和提供商回退行为。
定价来源翻译
messages/*/dashboard.json, tests/unit/i18n/pricing-source-keys.test.ts
五种语言的两个定价来源列表增加 cloud_official,测试验证完整键集合和非空翻译。

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

Suggested reviewers: brisbanehuang

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 17.65% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed 标题准确概括了结构化流回退边界的主要修复内容,简洁且与变更集相关。
Description check ✅ Passed 描述涵盖流式回退、Replay、Gemini 处理及国际化翻译,与变更集直接相关。
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/stream-precommit-fallback-i18n

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the size/XL Extra Large PR (> 1000 lines) label Aug 1, 2026
@github-actions

github-actions Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

🧪 测试结果

测试类型 状态
代码质量
单元测试
集成测试
API 测试

总体结果: ✅ 所有测试通过

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (3)
tests/unit/proxy/response-handler-stream-terminal.test.ts (1)

389-390: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

建议让 Replay abort 断言更精确。

Mock 的 spool 把 isTerminal 固定为 false。因此 observeChunk 中的流内 abort(原因形如 stream_protocol_malformed_after_content)与终态 abort(原因 protocol_malformed)都会被调用。当前 toHaveBeenCalledWith 只要任一次匹配即通过,无法区分是哪一条路径生效。

若本测试的目标是验证终态 Replay 资格判定,可断言最后一次调用的原因。

♻️ 建议的改动
-    expect(mocks.replayAbort).toHaveBeenCalledWith(expect.stringContaining("protocol_malformed"));
+    expect(mocks.replayAbort).toHaveBeenLastCalledWith("protocol_malformed");
     expect(mocks.replayComplete).not.toHaveBeenCalled();
🤖 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-stream-terminal.test.ts` around lines 389 -
390, Update the replayAbort assertion in this test to verify the final call’s
reason is exactly the terminal “protocol_malformed” value, rather than allowing
any matching invocation. Preserve the existing replayComplete negative assertion
and use the mock’s call history to distinguish the terminal abort from the
earlier stream abort.
src/app/v1/_lib/proxy/response-handler.ts (1)

1399-1457: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

可考虑复用已计算的完成标记结果。

hasTerminalStreamUsageEvidence 内部再次调用 parseSSEData(text),并在 claudeopenai 分支再次调用 inspectStreamCompletion(text, format)。调用方在 Line 1826 已经计算过 completionInspection。对同一份 allContent 重复解析,最多会解析三次。

该路径只在 postcommit malformed 场景求值,频率低。若希望减少大流式正文的重复解析,可把 hasMarker 作为参数传入。

🤖 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 1399 - 1457, Update
hasTerminalStreamUsageEvidence to accept the caller’s already-computed
completionInspection.hasMarker value, and use that parameter in the claude and
openai branches instead of calling inspectStreamCompletion(text, format) again.
Update the call site around completionInspection to pass the reused marker while
preserving all existing usage-evidence checks.
src/app/v1/_lib/proxy/stream-gate/stream-protocol-observer.ts (1)

26-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

建议让豁免上限不低于普通上限。

assertBufferLimit 只在缓冲超过 maxBufferedCharacters(10 MiB)之后才检查豁免。若 STREAM_GATE_PREBUFFER_BYTE_CAP 配置为小于 5 MiB 的值,则 exemption.maxBufferedCharacters(2x cap)小于 10 MiB,豁免分支永远无法命中。此时大 request echo 帧会让观察被禁用并标记 observationIncomplete,协议判定退化为 fail-open。

行为仍然安全,但豁免配置在小 cap 下静默失效。建议对两个上限取较大值。

♻️ 建议的改动
   const parser = new SseFrameParser({
     bufferLimitExemption: {
       // 门禁对 request echo 的豁免额度最多把总缓冲抬到 2x cap;observer 采用同一边界,
       // 允许合法的大请求回显,同时继续阻止伪装 echo 的无界单帧。
-      maxBufferedCharacters: streamGatePrebufferCharacters * 2,
+      maxBufferedCharacters: Math.max(
+        streamGatePrebufferCharacters * 2,
+        STREAM_PROTOCOL_OBSERVER_MAX_BUFFER_CHARACTERS
+      ),
       matches: (eventName, dataHead) => isRequestEchoFrame(family, eventName, dataHead),
     },
     maxBufferedCharacters: STREAM_PROTOCOL_OBSERVER_MAX_BUFFER_CHARACTERS,
   });
🤖 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/stream-gate/stream-protocol-observer.ts` around lines
26 - 40, Update createStreamProtocolObserver so the request-echo exemption
maxBufferedCharacters is at least
STREAM_PROTOCOL_OBSERVER_MAX_BUFFER_CHARACTERS, using the larger of that
constant and streamGatePrebufferCharacters * 2. Preserve the existing
cap-derived behavior when it exceeds the ordinary observer limit.
🤖 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.

Nitpick comments:
In `@src/app/v1/_lib/proxy/response-handler.ts`:
- Around line 1399-1457: Update hasTerminalStreamUsageEvidence to accept the
caller’s already-computed completionInspection.hasMarker value, and use that
parameter in the claude and openai branches instead of calling
inspectStreamCompletion(text, format) again. Update the call site around
completionInspection to pass the reused marker while preserving all existing
usage-evidence checks.

In `@src/app/v1/_lib/proxy/stream-gate/stream-protocol-observer.ts`:
- Around line 26-40: Update createStreamProtocolObserver so the request-echo
exemption maxBufferedCharacters is at least
STREAM_PROTOCOL_OBSERVER_MAX_BUFFER_CHARACTERS, using the larger of that
constant and streamGatePrebufferCharacters * 2. Preserve the existing
cap-derived behavior when it exceeds the ordinary observer limit.

In `@tests/unit/proxy/response-handler-stream-terminal.test.ts`:
- Around line 389-390: Update the replayAbort assertion in this test to verify
the final call’s reason is exactly the terminal “protocol_malformed” value,
rather than allowing any matching invocation. Preserve the existing
replayComplete negative assertion and use the mock’s call history to distinguish
the terminal abort from the earlier stream abort.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 27a0da72-2322-4ead-a363-a30f0521e352

📥 Commits

Reviewing files that changed from the base of the PR and between c7174f6 and 8e19f91.

📒 Files selected for processing (19)
  • messages/en/dashboard.json
  • messages/ja/dashboard.json
  • messages/ru/dashboard.json
  • messages/zh-CN/dashboard.json
  • messages/zh-TW/dashboard.json
  • src/app/v1/_lib/proxy/discovery-validity.ts
  • src/app/v1/_lib/proxy/forwarder.ts
  • src/app/v1/_lib/proxy/response-handler.ts
  • src/app/v1/_lib/proxy/stream-gate/frame-classifier.ts
  • src/app/v1/_lib/proxy/stream-gate/sse-frames.ts
  • src/app/v1/_lib/proxy/stream-gate/stream-protocol-observer.ts
  • tests/integration/proxy-hedge-lifecycle.test.ts
  • tests/unit/i18n/pricing-source-keys.test.ts
  • tests/unit/proxy/discovery-validity.test.ts
  • tests/unit/proxy/response-handler-endpoint-circuit-isolation.test.ts
  • tests/unit/proxy/response-handler-stream-terminal.test.ts
  • tests/unit/proxy/stream-gate-frame-classifier.test.ts
  • tests/unit/proxy/stream-gate-protocol-observer.test.ts
  • tests/unit/proxy/stream-gate-sse-frames.test.ts

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8e19f91acd

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +75 to +76
let verdict = classifyFrame(family, eventName, data);
if (verdict !== "neutral") return validityFromVerdict(verdict);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Check generic error flags before accepting content

When a fake-200 discovery frame contains failed: true or a nested response.error together with a nonempty content candidate, classifyFrame returns content here and bypasses isDiscoveryProtocolErrorPayload. The previous classifier checked these protocol errors before content, so this regression can mark the failing attempt ready and let it win the discovery race instead of falling back; preserve error precedence before returning a content verdict.

Useful? React with 👍 / 👎.

Comment on lines +1408 to +1415
case "response":
return events.some((event) => {
if (!isRecord(event.data)) return false;
const type = event.data.type;
if (type !== "response.completed" && type !== "response.done") return false;
const response = isRecord(event.data.response) ? event.data.response : null;
return hasPositiveUsage(event.data.usage) || hasPositiveUsage(response?.usage);
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Require a valid Responses completion marker for recovery

When a Responses stream has content followed by a malformed frame, any later payload whose data claims type: "response.completed" or response.done and reports positive usage authorizes the new postcommit-malformed success path, even if its SSE event name is unrelated or a response.completed payload lacks the required response object. inspectStreamCompletion rejects those shapes, but unlike the other protocol branches this branch never consults it, so an invalid terminal frame can make the request billable and persist it as a 2xx success; require the same completion-marker validation here.

Useful? React with 👍 / 👎.

Comment on lines 3615 to +3617
onChunk: (value) => {
passthroughShadowObserver?.observe(value);
streamProtocolObserver?.observe(value);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Unwrap Gemini response envelopes before observing frames

For supported Gemini NDJSON chunks shaped as {response:{...}}, the new passthrough observer feeds the wrapper directly to the Gemini classifier, which only examines top-level candidates, error, and finishReason. It therefore never records wrapped content; if a malformed frame then appears before a valid wrapped terminal-usage chunk, the failure is marked afterContent: false and the finalizer persists 502 instead of applying the intended postcommit-malformed success handling used for the equivalent unwrapped stream. Normalize the wrapper before protocol observation, as the discovery and terminal parsers already do.

Useful? React with 👍 / 👎.

Comment on lines +1878 to +1879
const successfulPostcommitMalformed =
postcommitMalformed && hasTerminalStreamUsageEvidence(allContent, session.originalFormat);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve later protocol errors after malformed frames

When a stream first emits a post-content malformed frame and later emits an explicit protocol error, the observer retains only the first failure, so this recovery check still sees verdict === "malformed". If the later error falls outside the bounded 10 MiB response-text snapshot while a terminal usage frame remains in the tail, neither body inspection sees the error and this condition reclassifies the request as a billable 2xx success. Track whether any protocol error was subsequently observed and never allow malformed recovery in that case.

Useful? React with 👍 / 👎.

} catch {
// A complete but non-JSON SSE event cannot establish protocol validity.
}
this.consumeFrame(candidate, eventName);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

**[HIGH] [LOGIC-BUG] Generic event: message headers now hide the payload event type

Why this is a problem: This new call passes the SSE event field into classifyFrame. The classifier only falls back to data.type when eventName is empty, so a valid relay frame such as event: message with data: {"type":"content_block_delta","delta":{"text":"hi"}} is classified as neutral (the same happens for Responses delta frames). Before this refactor, discovery parsed the JSON without the event name and recognized data.type; now Discovery can exhaust its precommit window and fail over on a valid stream. The completion code already treats message as a supported wrapper event, so this format is intended to be accepted.

Suggested fix:

const effectiveEventName = eventName?.trim() === "message" ? null : eventName;
this.consumeFrame(candidate, effectiveEventName);

Alternatively, make classifyFrame prefer the embedded type for generic wrapper event names, and add a regression test for event: message plus a typed content frame.

if (!isRecord(value)) return false;
const payload = isRecord(value.response) ? value.response : value;
return (
hasGeminiCompletionMarker(payload) &&

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

**[HIGH] [LOGIC-BUG] Any Gemini finishReason can authorize success after malformed data

Why this is a problem: This condition treats any non-empty Gemini finishReason as terminal usage evidence. However, the stream classifier explicitly marks reasons such as SAFETY, RECITATION, and MALFORMED_FUNCTION_CALL as error. If a malformed frame arrives after content first, the observer keeps only that first failure; a later terminal Gemini frame with finishReason: "SAFETY" and positive candidatesTokenCount makes this return true, so successfulPostcommitMalformed at response-handler.ts:1878 records the stream as HTTP 200, bills it, and allows successful binding side effects instead of preserving the protocol failure.

Suggested fix:

const finishReason = payload.candidates?.find(
  (candidate) => isRecord(candidate) && typeof candidate.finishReason === "string"
)?.finishReason;
const hasSuccessfulFinishReason = finishReason === "STOP" || finishReason === "MAX_TOKENS";
return (
  hasSuccessfulFinishReason &&
  (hasPositiveUsage(payload.usageMetadata) || hasPositiveUsage(payload.usage))
);

Keep the accepted finish-reason set aligned with the classifier's normal Gemini terminal reasons and add a regression test with malformed data followed by SAFETY.

for (const candidate of extractJsonChunks(text)) {
try {
payloads.push(JSON.parse(candidate) as unknown);
} catch {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

**[CRITICAL] [ERROR-SILENT] Malformed Gemini candidates are discarded without observability

Why this is a problem: This JSON.parse failure is caught and dropped, so a malformed or truncated Gemini candidate is indistinguishable from an intentionally incomplete payload. The same silent catch was added at src/app/v1/_lib/proxy/response-handler.ts:5616. That can leave completion and usage analysis with no diagnostic trail explaining why a candidate was skipped, which makes a real upstream/protocol failure invisible to operators and complicates billing/replay investigations.

Suggested fix:

} catch (error) {
  logger.debug("[ResponseHandler] Skipped malformed Gemini stream candidate", {
    format,
    error: error instanceof Error ? error.message : String(error),
  });
}

Apply equivalent bounded logging (or propagate an observationIncomplete/malformed count) in both catches while continuing to scan independent later chunks.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

The PR adds substantial structured-stream classification and replay/finalization handling, but five validated issues remain in the new paths. Four can change provider selection or terminal billing state, and two newly added parse catches silently hide malformed Gemini input; the full suite also reports one failure in unchanged src/components/ui/__tests__/language-switcher.test.tsx.

PR Size: XL

  • Lines changed: 1592
  • Files changed: 19
  • Split suggestions: Separate the stream parser/classifier and discovery changes from response finalization/replay/billing changes; keep the large integration/unit test additions and i18n key additions in focused follow-up PRs.

Issues Found

Category Critical High Medium Low
Logic/Bugs 0 4 0 0
Security 0 0 0 0
Error Handling 1 0 0 0
Types 0 0 0 0
Comments/Docs 0 0 0 0
Tests 0 0 0 0
Simplification 0 0 0 0

Critical Issues (Must Fix)

  • src/app/v1/_lib/proxy/response-handler.ts:1443[ERROR-SILENT] The new Gemini JSON.parse catch discards malformed candidates without logging or surfacing state; the same pattern is present at line 5616. Confidence: 95/100. Suggested fix: add bounded diagnostic logging or propagate an incomplete/malformed observation count while continuing later-chunk scanning.

High Priority Issues (Should Fix)

  • src/app/v1/_lib/proxy/discovery-validity.ts:227[LOGIC-BUG] Passing a generic SSE event: message name into the classifier prevents it from using the payload's typed data.type, so valid Anthropic/Responses relay frames remain neutral and can fail discovery. Confidence: 92/100. Suggested fix: normalize generic wrapper event names to null or make the classifier prefer the embedded type.
  • src/app/v1/_lib/proxy/response-handler.ts:3617[LOGIC-BUG] The native Gemini observer receives {response:{...}} wrappers without unwrapping, so valid wrapped content is not recorded; a later malformed frame is then treated as pre-content and the finalizer can persist 502 instead of applying postcommit handling. Confidence: 91/100. Suggested fix: normalize Gemini response envelopes before observer classification, matching discovery and terminal parsing.
  • src/app/v1/_lib/proxy/response-handler.ts:1879[LOGIC-BUG] The observer retains only the first protocol failure, so a later explicit error can be masked by an earlier malformed frame; if the error is outside the bounded snapshot, the recovery branch can bill a 2xx stream. Confidence: 93/100. Suggested fix: retain a later/hard protocol-error flag and never allow malformed recovery when any subsequent error was observed.
  • src/app/v1/_lib/proxy/response-handler.ts:1451[LOGIC-BUG] Any non-empty Gemini finishReason is accepted as success evidence, even though SAFETY and other abnormal reasons are classifier errors; with an earlier malformed frame, this can incorrectly finalize a stream as 200 and bill/bind it. Confidence: 94/100. Suggested fix: accept only normal terminal reasons such as STOP and MAX_TOKENS, aligned with the classifier.

Review Coverage

  • Logic and correctness
  • Security (OWASP Top 10)
  • Error handling
  • Type safety
  • Documentation accuracy
  • Test coverage
  • Code clarity

Automated review by Codex AI

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Posted

  • Completed review of PR #1373 and confirmed the size/XL label.
  • Posted three new inline comments and updated the mandatory summary with five validated issues: one Critical error-handling issue and four High-priority logic issues.
  • Key findings cover generic event: message handling, Gemini response-envelope observation, masked later protocol errors, abnormal Gemini finish reasons, and silent JSON parse failures.
  • Included XL split suggestions in the summary.

Validation

  • Focused proxy tests passed: 147 tests.
  • npx bun run typecheck, npx bun run lint, and git diff --check passed.
  • Full suite: 827 test files passed, 1 failed, 2 skipped; the failure is in unchanged src/components/ui/__tests__/language-switcher.test.tsx:153.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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/app/v1/_lib/proxy/discovery-validity.ts`:
- Around line 75-105: 检查 classifyFrameInner 是否已支持通用 eventName(如 "message")回退读取
data.type;若未支持,在 classifyProtocolFrame 中生成归一化的 effectiveEventName,并在原始调用及 Gemini
response 展开分支中统一传给 classifyFrame。补充回归测试覆盖 event: message 且 data.type
携带真实协议类型的帧,确认不会返回 neutral。
🪄 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 Plus

Run ID: f8c98b12-fef6-4ec5-914b-ceb6f18a080d

📥 Commits

Reviewing files that changed from the base of the PR and between 8e19f91 and 947036d.

📒 Files selected for processing (8)
  • src/app/v1/_lib/proxy/discovery-validity.ts
  • src/app/v1/_lib/proxy/response-handler.ts
  • src/app/v1/_lib/proxy/stream-gate/frame-classifier.ts
  • src/app/v1/_lib/proxy/stream-gate/stream-protocol-observer.ts
  • tests/unit/proxy/discovery-validity.test.ts
  • tests/unit/proxy/response-handler-stream-terminal.test.ts
  • tests/unit/proxy/stream-gate-frame-classifier.test.ts
  • tests/unit/proxy/stream-gate-protocol-observer.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
  • tests/unit/proxy/discovery-validity.test.ts
  • tests/unit/proxy/stream-gate-frame-classifier.test.ts
  • src/app/v1/_lib/proxy/stream-gate/frame-classifier.ts
  • tests/unit/proxy/stream-gate-protocol-observer.test.ts
  • tests/unit/proxy/response-handler-stream-terminal.test.ts
  • src/app/v1/_lib/proxy/stream-gate/stream-protocol-observer.ts
  • src/app/v1/_lib/proxy/response-handler.ts

Comment on lines +75 to +105
let parsed: unknown;
try {
parsed = JSON.parse(data) as unknown;
// 同帧的通用失败标志优先于 content;fake-200 失败响应不能赢得 Discovery。
if (isDiscoveryProtocolErrorPayload(parsed)) {
return { ready: false, terminal: true, error: true };
}
return {
ready:
(object.type === "response.output_text.delta" && hasContent(object.delta)) ||
(object.type === "response.function_call_arguments.delta" && hasContent(object.delta)) ||
(object.type === "response.reasoning_summary_text.delta" && hasContent(object.delta)) ||
(object.type === "response.output_item.added" && hasOpenAIResponsesOutputItem(object.item)),
terminal: false,
error: false,
};
}
if (protocol === "gemini") {
const response =
object.response && typeof object.response === "object" && !Array.isArray(object.response)
? (object.response as Record<string, unknown>)
: null;
const candidatesValue = response?.candidates ?? object.candidates;
const candidates = Array.isArray(candidatesValue) ? candidatesValue : [];
return {
ready: candidates.some((candidate) => hasContent(candidate)),
terminal: false,
error: false,
};
} catch {
parsed = undefined;
}
// Anthropic SSE data events: message_start/message_delta are metadata; a
// content_block_delta or tool use is the first deliverable event.

let verdict = classifyFrame(family, eventName, data);
if (verdict !== "neutral") return validityFromVerdict(verdict);

// Gemini SDK wrappers may expose the native candidate chunk under response.
if (
object.type === "message_start" ||
object.type === "message_delta" ||
object.type === "ping"
family === "gemini" &&
parsed &&
typeof parsed === "object" &&
!Array.isArray(parsed) &&
(parsed as Record<string, unknown>).response &&
typeof (parsed as Record<string, unknown>).response === "object"
) {
return { ready: false, terminal: false, error: false };
}
if (object.type === "message_stop") {
return { ready: false, terminal: true, error: false };
try {
verdict = classifyFrame(
family,
eventName,
JSON.stringify((parsed as Record<string, unknown>).response)
);
} catch {
return validityFromVerdict("malformed");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

确认通用包装事件名(如 "message")是否仍导致分类误判。

历史评审在旧提交的行 233 指出一个问题:当 SSE 帧使用通用 event: message 包装、而 data.type 携带真实类型(如 content_block_delta)时,classifyFrame 会把该帧误判为 neutral。原因是分类器只在 eventName 为空时才回退检查 data.type。这会导致 Discovery 阶段在有效流上耗尽 precommit 窗口,触发不必要的 provider fallback。

本次重构后的 classifyProtocolFrame 在行 86 和行 99-103 仍然把原始 eventName 直接传给 classifyFrame,没有对 "message" 这类通用包装事件名做归一化处理。由于 classifyFrameInner(位于 frame-classifier.ts)的内部实现未包含在本次审查内容中,无法确认该问题是否已在分类器内部修复。

如果该问题尚未修复,请在调用 classifyFrame 前对 eventName 做归一化,以恢复对 data.type 的回退检查。

🐛 建议修复(如问题仍然存在)
-  let verdict = classifyFrame(family, eventName, data);
+  const effectiveEventName = eventName?.trim() === "message" ? null : eventName;
+  let verdict = classifyFrame(family, effectiveEventName, data);
   if (verdict !== "neutral") return validityFromVerdict(verdict);

Gemini response 展开分支(行 99-103)需要使用同一个 effectiveEventName:

       verdict = classifyFrame(
         family,
-        eventName,
+        effectiveEventName,
         JSON.stringify((parsed as Record<string, unknown>).response)
       );

请运行以下脚本,确认 classifyFrameInner 是否已处理该场景:

#!/bin/bash
# 描述:检查 frame-classifier.ts 中对通用包装事件名(如 "message")的处理逻辑。
ast-grep outline src/app/v1/_lib/proxy/stream-gate/frame-classifier.ts --items all

echo "---"
rg -n -B5 -A30 'function classifyFrameInner' src/app/v1/_lib/proxy/stream-gate/frame-classifier.ts

请补充一条针对 event: message 包装 + data.type 携带真实类型帧的回归测试,验证该帧不会被误判为 neutral

🤖 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/discovery-validity.ts` around lines 75 - 105, 检查
classifyFrameInner 是否已支持通用 eventName(如 "message")回退读取 data.type;若未支持,在
classifyProtocolFrame 中生成归一化的 effectiveEventName,并在原始调用及 Gemini response
展开分支中统一传给 classifyFrame。补充回归测试覆盖 event: message 且 data.type 携带真实协议类型的帧,确认不会返回
neutral。

@github-actions

github-actions Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

🧪 测试结果

测试类型 状态
代码质量
单元测试
集成测试
API 测试

总体结果: ✅ 所有测试通过

@ding113
ding113 merged commit 8a7e890 into dev Aug 1, 2026
12 checks passed
@github-project-automation github-project-automation Bot moved this from Backlog to Done in Claude Code Hub Roadmap Aug 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XL Extra Large PR (> 1000 lines)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant