fix(proxy): prevent failed responses from entering replay - #1370
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughWalkthroughChanges该 PR 重构 Replay owner 的原子存储与生命周期管理，新增响应头恢复、有界流协议观察和响应终态校验，并将 Replay owner 纳入首内容门控；同时更新配置文案、API 描述及相关测试覆盖。 Replay 与流式门控
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/response-handler.ts`:
- Around line 2767-2774: Replace the mutable replaySpool binding in the
response-transform branch with a const value, or otherwise capture it in a const
before any closure uses it, while preserving null for responseTransformFailed
and the buffered spool otherwise. Ensure subsequent replaySpool truthiness
checks and responseText/statusCode handling use the stabilized binding.
In `@tests/unit/proxy/response-handler-stream-terminal.test.ts`:
- Around line 61-72: 在 response-handler-stream-terminal.test.ts 的 replay-spool
mock 中补充 abortReplayOwnership 导出，并将其连接到现有的 mocks 以保持与 response-handler.ts
命名导入的接口一致；不要改变 createReplaySpoolIfOwner 或现有流式测试行为。
🪄 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: cb9c916f-905c-4c13-b398-9b232f35c6a4
📒 Files selected for processing (21)
messages/en/settings/config.jsonmessages/ja/settings/config.jsonmessages/ru/settings/config.jsonmessages/zh-CN/settings/config.jsonmessages/zh-TW/settings/config.jsonsrc/app/v1/_lib/proxy/forwarder.tssrc/app/v1/_lib/proxy/replay/replay-guard.tssrc/app/v1/_lib/proxy/replay/replay-headers.tssrc/app/v1/_lib/proxy/replay/replay-spool.tssrc/app/v1/_lib/proxy/replay/replay-store.tssrc/app/v1/_lib/proxy/response-handler.tssrc/app/v1/_lib/proxy/stream-gate/stream-protocol-observer.tssrc/lib/api-client/v1/openapi-types.gen.tssrc/lib/api/v1/schemas/system-config.tstests/unit/proxy/replay-guard.test.tstests/unit/proxy/replay-spool.test.tstests/unit/proxy/replay-store.test.tstests/unit/proxy/response-handler-nonstream-terminal.test.tstests/unit/proxy/response-handler-stream-terminal.test.tstests/unit/proxy/stream-gate-forwarder-integration.test.tstests/unit/proxy/stream-gate-protocol-observer.test.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c932ae909d
ℹ️ 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".
| streamTextAccumulator.pushBytes(value); | ||
| AsyncTaskManager.touch(taskId); | ||
| shadowGateObserver?.observe(value); | ||
| replayProtocolObserver?.observe(value); |
There was a problem hiding this comment.
When a replay-owner stream contains a very large or unterminated SSE line, this observer feeds every chunk into SseFrameParser, whose lineTail grows until a line ending arrives. Unlike streamTextAccumulator and ReplaySpool, the observer has no byte limit and continues receiving chunks even after the spool disables itself for exceeding REPLAY_MAX_PAYLOAD_BYTES, so a malformed or hostile upstream can cause unbounded process memory growth. Stop observing when replay is disabled or add a bounded parser/observation limit.
Useful? React with 👍 / 👎.
| const replayDetected = detectUpstreamErrorFromSseOrJsonText(responseText); | ||
| if (statusCode >= 200 && statusCode < 300 && !replayDetected.isError) { | ||
| postTerminalSideEffects.push(() => | ||
| replaySpool.completeAfterBilling(messageContext?.id ?? null) |
There was a problem hiding this comment.
When a streaming request receives a 2xx non-SSE JSON response that cleanly ends with truncated or otherwise invalid JSON, detectUpstreamErrorFromSseOrJsonText explicitly treats the parse failure as isError: false, so this branch persists the malformed body as a completed replay. Subsequent identical requests then receive the same corrupt response from Redis or PostgreSQL until expiry instead of retrying an upstream provider; validate the buffered payload according to its content type/protocol before scheduling completion.
Useful? React with 👍 / 👎.
| try { | ||
| return await ProxyForwarder.sendInternal(session); | ||
| } catch (error) { | ||
| await abortReplayOwnership(session, "forward_failed"); |
There was a problem hiding this comment.
When an owner request fails forwarding while Redis still reports ready but commands are stalled, this awaited best-effort abort delays propagation of the original upstream error until the Redis command timeout expires (10 seconds by default and configurable up to 120 seconds). A simultaneous Redis degradation therefore adds substantial latency to every replay-owner failure even though replay is documented as fail-open; perform this cleanup asynchronously or impose a much shorter independent deadline.
Useful? React with 👍 / 👎.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if (finished || disabled || chunk.byteLength === 0) return; | ||
| try { | ||
| for (const frame of parser.push(chunk)) record(frame); | ||
| } catch { |
There was a problem hiding this comment.
[High] [ERROR-SILENT] Observer failures currently look identical to a verified clean stream
Why this is a problem: both catch blocks just flip disabled = true, and finish() then returns failure: null. ProxyResponseHandler treats that as a clean protocol observation and can still call replaySpool.completeAfterBilling() on an unverified stream. If the observer throws after content has started, this silently defeats the new "late protocol failures abort replay" safety guarantee.
Suggested fix:
} catch (error) {
logger.warn("[StreamProtocolObserver] protocol observation failed", {
error: error instanceof Error ? error.message : String(error),
});
observation.failure ??= { verdict: "malformed", eventName: null };
disabled = true;
}If you do not want to overload malformed, return an explicit observerFailed flag and make response-handler.ts treat it as non-replayable.
| }; | ||
| await getReplayStore() | ||
| .abortOwned(identity.replayId, ownerToken, meta) | ||
| .catch(() => false); |
There was a problem hiding this comment.
[High] [ERROR-SILENT] Failed pre-spool aborts are dropped without any signal
Why this is a problem: ReplayStore.abortOwned() returns false when Redis is unavailable or the owner token check fails. This helper clears session.replayState first and then ignores that false, so a forwarder/handler failure can leave the owner lease behind until TTL with no log. The next identical request cannot reclaim ownership and falls back to a duplicate upstream call for ~45 seconds, which is exactly the stale-owner window this helper was added to close.
Suggested fix:
const aborted = await getReplayStore().abortOwned(identity.replayId, ownerToken, meta);
if (!aborted) {
logger.warn("[ReplaySpool] failed to abort pre-spool replay ownership", {
replayId: identity.replayId.slice(0, 12),
reason,
});
await getReplayStore().releaseOwner(identity.replayId, ownerToken);
}
There was a problem hiding this comment.
Code Review Summary
This PR changes replay ownership, stream validation, response finalization, Redis fencing, and generated/docs surfaces in one stack. I found 2 high-priority defects in the new replay failure-handling path that can leave ownership stale or treat an unverified stream as safe to cache.
PR Size: XL
- Lines changed: 1729
- Files changed: 21
- Split suggestions: separate (1) Redis/replay storage primitives, (2) forwarder/response-handler protocol integration, and (3) docs/generated API surface updates.
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 2 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 2 | 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)
None.
High Priority Issues (Should Fix)
src/app/v1/_lib/proxy/replay/replay-spool.ts:418—abortReplayOwnership()ignores a failedabortOwned()result after clearingsession.replayState, so a pre-spool failure can leave the owner lease blocking identical requests until TTL with no signal.src/app/v1/_lib/proxy/stream-gate/stream-protocol-observer.ts:44— observer exceptions are silently converted intofailure: null, soresponse-handler.tscan still mark replay as completed even though the late-protocol verification path never finished.
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Codex AI
There was a problem hiding this comment.
Review Posted
- Reviewed PR
#1370, applied thesize/XLlabel, and posted the required summary review. - Left 2 inline comments on changed lines:
src/app/v1/_lib/proxy/replay/replay-spool.ts:418— failed pre-spool aborts are ignored, which can leave a stale owner lease blocking identical requests until TTL.src/app/v1/_lib/proxy/stream-gate/stream-protocol-observer.ts:44— observer exceptions are silently downgraded to a “clean” result, so replay can complete without verified late-protocol checks.
- Summary marks the PR as
XL(1729lines across21files) and includes split suggestions for replay storage, forwarder/response integration, and docs/generated changes.
If you want, I can do a follow-up pass after the author pushes fixes.
…ner writes Add buffered delivery mode so non-stream (JSON) responses are eligible for replay caching alongside the existing SSE stream path. Capture and restore semantic response headers while filtering hop-by-hop headers, so replayed JSON responses preserve content-type and custom headers. Replace the previous append-then-setMeta-then-renewOwner sequence with three new fenced Lua scripts (writeOwned, abortOwned, completeOwned) that atomically verify the owner token before mutating chunks, meta, or the lease. This prevents a stale owner whose lease expired mid-stream from corrupting a new owner's entry during the handoff window. A 15 s heartbeat timer now renews the lease independently of the write chain so PG persistence latency cannot cause premature lease expiry. Force replay owners through the pre-content stream gate regardless of the configured gate mode, ensuring error frames or empty streams trigger failover before any bytes reach the client. Add a StreamProtocolObserver that inspects every SSE frame through to EOF to detect late protocol errors (e.g. response.failed after response.completed) that fall outside the bounded text snapshot; such detections abort the replay entry and surface a 502 instead of caching a broken response. Abort replay ownership atomically when the forwarder or response handler terminates before spool creation, preventing stale entries from blocking duplicate requests for the full lease TTL.
…emory Prevent malformed upstream JSON from being persisted as a completed replay entry. Replay owners now receive strict buffered JSON validation in the forwarder before provider fallback is attempted; the response handler independently re-checks the body and aborts the replay spool with non_stream_malformed_json instead of calling completeAfterBilling. Non-JSON content types bypass the parse check and complete normally. When a replay JSON body exceeds the configured payload limit the forwarder releases ownership gracefully and passes the response through without triggering a spurious fallback. Add a 100 ms deadline to abortReplayOwnership so a stuck store abort cannot block the request hot path; if the fenced abort returns false the code falls back to releaseOwner and logs a warning. Bound SseFrameParser memory by accepting an optional maxBufferedCharacters limit; the stream protocol observer uses a 1 MiB cap so an unterminated SSE line fails closed with a malformed verdict instead of growing unbounded. Also fix a closure capture bug where the post-terminal replay spool abort could reference a stale spool instance, and ensure Gemini non-stream transforms set content-type to application/json on the rebuilt response.
c932ae9 to
ca6f46e
Compare
🧪 测试结果
总体结果: ✅ 所有测试通过 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca6f46e83d
ℹ️ 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".
| const parser = new SseFrameParser({ | ||
| maxBufferedCharacters: REPLAY_PROTOCOL_OBSERVER_MAX_BUFFER_CHARACTERS, | ||
| }); |
There was a problem hiding this comment.
When replay is enabled for a valid Codex/OpenAI Responses stream whose response.created, response.in_progress, or response.queued echo frame contains more than 1 MiB of request context, this new cap throws and the observer records a malformed failure; finalization then projects the naturally completed 2xx stream as UPSTREAM_PROTOCOL_ERROR/502 and aborts replay. The precommit gate explicitly permits request-echo payloads up to its larger configured bound, so treat observer overflow as disabling replay observation rather than an upstream failure, or make the bound echo-aware.
Useful? React with 👍 / 👎.
| throw new ProxyError("MALFORMED_BUFFERED_JSON", 502, { | ||
| body: "Upstream returned malformed JSON", |
There was a problem hiding this comment.
When every provider returns malformed buffered JSON, buildAllProvidersUnavailableError derives its client-safe candidate from this upstreamError.body, so with upstream-message passthrough enabled this English literal can be returned to API clients. Route the message through the repository's five-locale i18n mechanism instead of hardcoding the display text.
AGENTS.md reference: AGENTS.md:L12-L14
Useful? React with 👍 / 👎.
| replaySpool.observe(RESPONSE_TEXT_ENCODER.encode(clientVisibleResponseText)); | ||
| postTerminalSideEffects.push(() => | ||
| replaySpool.completeAfterBilling(messageContext?.id ?? null) |
There was a problem hiding this comment.
When a streaming request receives a successful non-SSE binary or otherwise non-UTF-8 response, buffered delivery accepts it, but the background reader decodes the body as text and this line re-encodes it before persistence. The owner receives the original bytes while later identical requests receive replacement characters or otherwise changed bytes from replay; restrict buffered replay to UTF-8 textual media types or spool the original Uint8Array data.
Useful? React with 👍 / 👎.
| local len = redis.call('LLEN', KEYS[3]) | ||
| if #ARGV > 4 then | ||
| len = redis.call('RPUSH', KEYS[3], unpack(ARGV, 5)) | ||
| if tonumber(ARGV[2]) > 0 then |
There was a problem hiding this comment.
When an upstream response is delivered as thousands of tiny chunks within one 64 KiB/100 ms flush window, every chunk becomes a separate ARGV value and this unpack attempts to place the entire batch on Redis Lua's limited VM stack. A sufficiently fragmented but otherwise small response therefore makes the script fail, causing writeOwned to report Redis unavailable and disable replay; concatenate the batch or issue bounded RPUSH slices inside the script.
Useful? React with 👍 / 👎.
| const replayMalformedJson = isMalformedJsonResponseBody( | ||
| response.headers.get("content-type"), | ||
| responseText | ||
| ); | ||
| const replayDetected = detectUpstreamErrorFromSseOrJsonText(responseText); | ||
| if ( | ||
| statusCode >= 200 && | ||
| statusCode < 300 && | ||
| !replayMalformedJson && | ||
| !replayDetected.isError |
There was a problem hiding this comment.
When a Gemini streaming request is answered with a 2xx non-SSE safety response such as {"promptFeedback":{"blockReason":"SAFETY"}} or a candidate with a safety finishReason, the JSON is well formed and this generic detector does not recognize the Gemini-specific failure signal, even though the stream classifier explicitly treats those fields as errors. The branch therefore completes and persists the blocked response, causing identical requests to replay it until expiry instead of retrying an available provider; apply the Gemini protocol failure rules before scheduling completion.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/v1/_lib/proxy/forwarder.ts (1)
4759-4826: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win非门控 hedge 分支遗漏 firstByteAt 记录，TTFB 统计缺失。
当
hedgeGateFamily为空（即STREAM_GATE_MODE非enforce且当前请求不是 Replay owner）时，else分支通过readFirstReadableChunk读到首块后并未设置attempt.firstByteAt，导致commitWinner中if (attempt.firstByteAt != null) { session.recordFirstByte(attempt.firstByteAt); }永远不会触发。对比 Discovery 路径（6590-6592、6139-6142），无论是否触发内容门控，attempt.firstByteAt都会无条件记录并调用recordTfft()。字段注释（"该 attempt 首字节到达时刻...只有赢家的值会被记为 session TTFB"）也暗示这一时间戳不应依赖门控是否运行。结果是：非 enforce 模式（当前大概率是默认/多数流量状态）下，hedge 赢家的 TTFB 会持续缺失，按代码自身注释会"低估 TTFB 并放大 TPS 的分母"，影响相关可观测性/计费指标。
🐛 建议修复
} else { const firstChunk = await ProxyForwarder.readFirstReadableChunk(attempt.reader); if (firstChunk.done) { await handleAttemptFailure( attempt, new EmptyResponseError(attempt.provider.id, attempt.provider.name, "empty_body") ); return; } + attempt.firstByteAt ??= Date.now(); // 保留首块：若本 attempt 落败且需要计费，drain 时需要补回首块的 usage。 attempt.firstChunk = firstChunk.value;🤖 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/forwarder.ts` around lines 4759 - 4826, 在非门控 hedge 分支中更新 `readFirstReadableChunk` 成功后的处理：在将首块赋给 `attempt.firstChunk` 并调用 `commitWinner` 前记录 `attempt.firstByteAt`，确保与门控分支一致，并让赢家的 `commitWinner` 流程能够调用 session 的首字节统计。仅在确实读到首块时设置该时间戳，保留现有空响应处理。
🧹 Nitpick comments (2)
tests/unit/proxy/replay-spool.test.ts (1)
163-181: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win不要在
beforeEach中混用mockResolvedValueOnce和mockClear()。当前文件已用
vi.clearAllMocks()保证调用记录隔离，但mockClear()不过滤mockResolvedValueOnce/mockImplementationOnce队列；如果一个用例提前 disable/terminal 导致writeOwned或completeOwned的 once 值未消费，会残留给后续用例。建议改用mockReset()/mockRestore()清理 once 队列，并在beforeEach重新注册所需的持久实现。🤖 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/replay-spool.test.ts` around lines 163 - 181, Update the beforeEach setup to use mockReset() for storeControl.store.completeOwned and storeControl.store.writeOwned instead of mockClear(), ensuring any unconsumed mockResolvedValueOnce or mockImplementationOnce queue is removed; then re-register their required persistent implementations in the setup while retaining vi.clearAllMocks() for call-history isolation.src/app/v1/_lib/proxy/response-handler.ts (1)
2756-2766: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win直接校验
transformed，不要序列化后反过来校验。
transformedBody === undefined对对象输入不会触发；随后JSON.parse(JSON.stringify(transformed))对大响应体是复制一遍再扫描一遍内存。GeminiAdapter.transformResponse返回OpenAICompatibleResponse，可直接校验transformed的结构再序列化。🤖 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 2756 - 2766, 在 GeminiAdapter.transformResponse 的响应处理流程中，直接校验 transformed 返回的 OpenAICompatibleResponse，而不要通过 transformedBody 序列化后再 JSON.parse 校验。将 undefined、非对象、null 和数组检查移到序列化之前，并在校验通过后仅序列化一次；移除针对 transformedBody 的重复解析校验。
🤖 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/response-content-type.ts`:
- Around line 4-5: Update the media-type classification logic around the
existing mediaType check to first require a valid type/subtype structure
containing exactly one “/” with non-empty components. Then inspect only the
subtype, returning true when it is “json” or ends with “+json”; otherwise return
false so values such as “vendor+json” are not classified as JSON.
---
Outside diff comments:
In `@src/app/v1/_lib/proxy/forwarder.ts`:
- Around line 4759-4826: 在非门控 hedge 分支中更新 `readFirstReadableChunk` 成功后的处理：在将首块赋给
`attempt.firstChunk` 并调用 `commitWinner` 前记录
`attempt.firstByteAt`，确保与门控分支一致，并让赢家的 `commitWinner` 流程能够调用 session
的首字节统计。仅在确实读到首块时设置该时间戳，保留现有空响应处理。
---
Nitpick comments:
In `@src/app/v1/_lib/proxy/response-handler.ts`:
- Around line 2756-2766: 在 GeminiAdapter.transformResponse 的响应处理流程中，直接校验
transformed 返回的 OpenAICompatibleResponse，而不要通过 transformedBody 序列化后再 JSON.parse
校验。将 undefined、非对象、null 和数组检查移到序列化之前，并在校验通过后仅序列化一次；移除针对 transformedBody 的重复解析校验。
In `@tests/unit/proxy/replay-spool.test.ts`:
- Around line 163-181: Update the beforeEach setup to use mockReset() for
storeControl.store.completeOwned and storeControl.store.writeOwned instead of
mockClear(), ensuring any unconsumed mockResolvedValueOnce or
mockImplementationOnce queue is removed; then re-register their required
persistent implementations in the setup while retaining vi.clearAllMocks() for
call-history isolation.
🪄 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: fcb082cc-8789-4737-9731-80d88bade523
📒 Files selected for processing (25)
messages/en/settings/config.jsonmessages/ja/settings/config.jsonmessages/ru/settings/config.jsonmessages/zh-CN/settings/config.jsonmessages/zh-TW/settings/config.jsonsrc/app/v1/_lib/proxy/forwarder.tssrc/app/v1/_lib/proxy/replay/replay-guard.tssrc/app/v1/_lib/proxy/replay/replay-headers.tssrc/app/v1/_lib/proxy/replay/replay-spool.tssrc/app/v1/_lib/proxy/replay/replay-store.tssrc/app/v1/_lib/proxy/response-content-type.tssrc/app/v1/_lib/proxy/response-handler.tssrc/app/v1/_lib/proxy/stream-gate/sse-frames.tssrc/app/v1/_lib/proxy/stream-gate/stream-protocol-observer.tssrc/lib/api-client/v1/openapi-types.gen.tssrc/lib/api/v1/schemas/system-config.tstests/unit/proxy/proxy-forwarder-fake-200-html.test.tstests/unit/proxy/replay-guard.test.tstests/unit/proxy/replay-spool.test.tstests/unit/proxy/replay-store.test.tstests/unit/proxy/response-handler-nonstream-terminal.test.tstests/unit/proxy/response-handler-stream-terminal.test.tstests/unit/proxy/stream-gate-forwarder-integration.test.tstests/unit/proxy/stream-gate-protocol-observer.test.tstests/unit/proxy/stream-gate-sse-frames.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- messages/zh-TW/settings/config.json
- src/lib/api/v1/schemas/system-config.ts
- messages/ja/settings/config.json
- messages/ru/settings/config.json
- messages/zh-CN/settings/config.json
- messages/en/settings/config.json
- src/lib/api-client/v1/openapi-types.gen.ts
| const mediaType = contentType.split(";", 1)[0]?.trim().toLowerCase() ?? ""; | ||
| return mediaType === "application/json" || mediaType.endsWith("+json"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
先校验合法的 type/subtype 媒体类型，再识别 +json。
当前逻辑只检查后缀，因此 Content-Type: vendor+json 这类缺少 / 的值也会被判定为 JSON。下游可能因此进入 JSON 缓冲与校验路径，并将普通响应误判为 malformed，触发 502 或 Replay abort。
请先拆分并校验 type/subtype，再判断 subtype 是否为 json 或以 +json 结尾。该判断基于提供的下游调用：forwarder.ts Line [1682] 和 response-handler.ts Line [2938]-[2941] 依赖此分类结果。
建议修改
const mediaType = contentType.split(";", 1)[0]?.trim().toLowerCase() ?? "";
- return mediaType === "application/json" || mediaType.endsWith("+json");
+ const [type, subtype, ...extra] = mediaType.split("/");
+ if (!type || !subtype || extra.length > 0) return false;
+ return subtype === "json" || subtype.endsWith("+json");📝 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.
| const mediaType = contentType.split(";", 1)[0]?.trim().toLowerCase() ?? ""; | |
| return mediaType === "application/json" || mediaType.endsWith("+json"); | |
| const mediaType = contentType.split(";", 1)[0]?.trim().toLowerCase() ?? ""; | |
| const [type, subtype, ...extra] = mediaType.split("/"); | |
| if (!type || !subtype || extra.length > 0) return false; | |
| return subtype === "json" || subtype.endsWith("+json"); |
🤖 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-content-type.ts` around lines 4 - 5, Update
the media-type classification logic around the existing mediaType check to first
require a valid type/subtype structure containing exactly one “/” with non-empty
components. Then inspect only the subtype, returning true when it is “json” or
ends with “+json”; otherwise return false so values such as “vendor+json” are
not classified as JSON.
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
1 participant
Summary
streamGateModeisofforshadowBehavior
offremains passthrough andshadowremains observation-onlyenforceSafety
Testing
Passed:
bun run buildbun run lint:fixbun run lintbun run typecheckbun run test:v1(91 files, 378 tests)bun run openapi:checkbun run openapi:lintgit diff --checkFull
bun run testreached 819 passed files, 2 skipped files, 7939 passed tests, and 13 skipped tests. It remains non-zero because the existing unrelated testLanguageSwitcher > keeps the pending refresh after remount when sessionStorage is blockedexpectsconsole.errorto be called. The failure reproduces independently, and neither the component nor its test is changed by this PR.Greptile Summary
Updates Replay handling to prevent failed upstream responses from becoming completed cache entries.
Confidence Score: 5/5
The PR appears safe to merge because no blocking failure remains within the follow-up review scope.
No blocking failure remains.
Important Files Changed
Sequence Diagram
sequenceDiagram participant Client participant Guard as Replay Guard participant Forwarder participant Upstream participant Handler as Response Handler participant Redis participant PG as PostgreSQL Client->>Guard: Replay-eligible request Guard->>Redis: Claim owner token Guard->>Forwarder: Continue as Replay owner Forwarder->>Upstream: Forward request Upstream-->>Forwarder: SSE or buffered response Forwarder->>Forwarder: Apply pre-content gate / JSON validation alt Pre-content or buffered protocol failure Forwarder->>Redis: Abort owned Replay entry Forwarder->>Upstream: Retry or provider fallback else Deliverable response Forwarder->>Handler: Client-visible response Handler-->>Client: Stream or buffered body Handler->>Redis: Fenced chunk and metadata writes Handler->>Handler: Observe terminal protocol state alt Successful terminal state and durable side effects Handler->>PG: Persist completed payload Handler->>Redis: Fenced completion else Late protocol or persistence failure Handler->>Redis: Fenced abort and chunk cleanup end endReviews (2): Last reviewed commit: "fix(proxy): validate replay JSON before ..." | Re-trigger Greptile
Context used: