fix: correct HTTP statusText and Gemini GET/HEAD handling - #481
Conversation
- Fix statusText bug: was `HTTP/1.1 200 200`, now `HTTP/1.1 200 OK` (use STATUS_CODES from node:http instead of String(statusCode)) - Fix Gemini request body handling: skip JSON.stringify for GET/HEAD requests which should not have a body
Summary of ChangesHello @NieiR, 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! This pull request addresses two critical issues related to API request and response handling. It standardizes the HTTP Highlights
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
|
There was a problem hiding this comment.
Code Review
This pull request introduces two important fixes. First, it corrects the HTTP response statusText by using standard status codes from node:http, which is a great improvement over using the status code number. Second, it properly handles Gemini GET/HEAD requests by ensuring no body is sent, which resolves potential errors. The changes are well-implemented, though I have a small suggestion to make the statusText fallback more robust for non-standard error codes. Additionally, please note that the update to CHANGELOG.md appears to contain entries for a different set of changes and does not reflect the fixes in this PR, which might cause confusion.
| ### 新增 | ||
|
|
||
| - Session 详情页新增请求/响应头日志展示,支持 Tab 切换查看 (#469) | ||
| - 排行榜新增排序和供应商类型筛选功能 (#448) [@YewFence](https://github.com/YewFence) | ||
| - 虚拟化表格组件 (use-virtualizer hook) 用于大数据量列表性能优化 (#467) [@NightYuYyy](https://github.com/NightYuYyy) | ||
| - 新增 `FETCH_CONNECT_TIMEOUT` 环境变量,统一配置 Undici 连接超时(默认 30 秒)(#479, #480) | ||
|
|
||
| ### 优化 | ||
|
|
||
| - 供应商管理页面 UX 改进,优化交互体验 (#446) [@miraserver](https://github.com/miraserver) | ||
| - 用户筛选与排序体验优化,移除使用日志用户筛选限制 (#462, #449) [@NightYu](https://github.com/NightYuYyy) | ||
| - 缓存 tooltip 显示改进,当 5m/1h breakdown 不可用时提供友好提示 (#445) [@Hwwwww](https://github.com/Hwwwww-dev) | ||
| - TagInput 组件和虚拟化表格稳定性增强 (#467) [@NightYuYyy](https://github.com/NightYuYyy) | ||
| - SSE 解析工具增强,添加错误处理和测试 (#469) | ||
| - Session 消息客户端 SSE 性能和 matchMedia 回退优化 (#469) | ||
|
|
||
| ### 修复 | ||
|
|
||
| - 修复计费模型来源配置不生效问题 (#464) | ||
| - Codex instructions 一律透传,移除缓存与策略 (#475) | ||
| - 修复 Session 详情页中的 tool_use_id 验证问题 (#473, #472) | ||
| - 修复日志表格中供应商名称溢出问题 (#478) [@YangQing-Lin](https://github.com/YangQing-Lin) | ||
| - 请求过滤器 header 修改追踪修复,确保在 Session 详情中正确显示 (#465) | ||
| - 数据导入组件优化,移除重复描述文本 (#458) [@Abner](https://github.com) | ||
|
|
||
| ### 其他 | ||
|
|
||
| - 新增多项单元测试:undici 超时、proxy forwarder、session 等 (#469, #479) | ||
| - 移除 codex-instructions-cache.ts 模块,简化代码结构 (#475) | ||
| --- |
There was a problem hiding this comment.
The content added to the changelog for v0.3.38 seems to be unrelated to the changes in this pull request, which focuses on fixing statusText and Gemini GET/HEAD request handling. The changelog entries describe new features and optimizations that are not part of this PR. This could lead to confusion about what is included in the release. Please verify if this changelog update is intended for this PR.
| return new Response(bodyStream, { | ||
| status: undiciRes.statusCode, | ||
| statusText: String(undiciRes.statusCode), | ||
| statusText: STATUS_CODES[undiciRes.statusCode] ?? "OK", |
There was a problem hiding this comment.
Using "OK" as a fallback for statusText can be misleading for non-standard error status codes. For example, if undiciRes.statusCode is a custom error code like 599, STATUS_CODES[599] will be undefined, and the response status line would become HTTP/1.1 599 OK, which is incorrect. A safer approach is to use an empty string "" as the fallback. This allows the Response constructor to use its default status text for known codes or leave it empty for unknown ones, which is more accurate.
| statusText: STATUS_CODES[undiciRes.statusCode] ?? "OK", | |
| statusText: STATUS_CODES[undiciRes.statusCode] ?? "", |
There was a problem hiding this comment.
Code Review Summary
This PR fixes two specific bugs in the proxy forwarder: HTTP statusText formatting and Gemini GET/HEAD request handling. Both changes are well-targeted and follow existing patterns in the codebase.
PR Size: S
- Lines changed: 37 additions, 4 deletions
- Files changed: 2 (CHANGELOG.md, src/app/v1/_lib/proxy/forwarder.ts)
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 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 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Analysis Summary
Change 1: HTTP statusText fix (line 1964)
- Replaces
String(undiciRes.statusCode)withSTATUS_CODES[undiciRes.statusCode] ?? "OK" - Corrects the status line from
HTTP/1.1 200 200toHTTP/1.1 200 OK - Uses Node.js
http.STATUS_CODESmapping for proper reason phrases - Provides sensible fallback for undefined status codes
- Confidence: This is a correct fix that addresses the reported bug
Change 2: Gemini GET/HEAD body handling (lines 826-831)
- Adds conditional check:
hasBody = session.method !== "GET" && session.method !== "HEAD" - Only calls
JSON.stringify()when the HTTP method allows a request body - Follows the exact same pattern used elsewhere in the file (line 1099)
- The
session.methodproperty is defined asreadonly stringinProxySessionclass and set viac.req.method.toUpperCase() - Confidence: This is a correct fix that prevents JSON serialization of GET/HEAD requests
Review Coverage
- Logic and correctness - Changes are bug fixes that align with HTTP specs
- Security (OWASP Top 10) - No security issues introduced
- Error handling - No error handling changes
- Type safety - Types are consistent with existing code
- Documentation accuracy - No comment changes needed
- Test coverage - No test file changes (fixes are straightforward)
- Code clarity - Changes improve code correctness
Notes
-
STATUS_CODES import: The import
import { STATUS_CODES } from "node:http";is added at line 1, which is the correct location for module imports. -
Consistent pattern: The
hasBodycheck at line 828 uses the exact same logic as line 1099, maintaining consistency with existing code patterns in the same file. -
CHANGELOG.md update: The PR includes a changelog entry (lines 9-37 in CHANGELOG.md) documenting the fixes, which follows project conventions.
-
No tests added: Given the straightforward nature of these bug fixes (using standard library constant and applying HTTP method semantics), the lack of test coverage is acceptable. The changes are well-scoped and follow existing patterns.
Automated review by Claude AI
Greptile found no issues!From now on, if a review finishes and we haven't found any issues, we will not post anything, but you can confirm that we reviewed your changes in the status check section. This feature can be toggled off in your Code Review Settings by deselecting "Create a status check for each PR". |
|
Closing: will resubmit after further testing |
Summary
HTTP/1.1 200 200, nowHTTP/1.1 200 OK(use STATUS_CODES from node:http instead of String(statusCode))Problem
This PR fixes two separate bugs in the proxy forwarder:
Incorrect HTTP statusText: Responses were being constructed with numeric status codes repeated in the status line (e.g.,
HTTP/1.1 200 200instead ofHTTP/1.1 200 OK), which violates HTTP/1.1 specification and could cause issues with strict HTTP clients.Gemini GET/HEAD request serialization: Gemini API providers were incorrectly attempting to serialize request bodies for GET/HEAD requests, which should not have request bodies according to HTTP specification. This caused errors or malformed requests.
Solution
StatusText Fix
Changed from
String(undiciRes.statusCode)toSTATUS_CODES[undiciRes.statusCode] ?? "OK"using Node.js's built-inSTATUS_CODESmap fromnode:http.Gemini Request Body Fix
Added check for HTTP method before serializing request body:
Changes
Core Changes
src/app/v1/_lib/proxy/forwarder.ts:827-831- Added method check before JSON.stringify for Gemini requestssrc/app/v1/_lib/proxy/forwarder.ts:1963- Use STATUS_CODES from node:http for proper status textDocumentation Changes
CHANGELOG.md- Entry added for v0.3.38Test plan
HTTP/1.1 200 OK)Description enhanced by Claude AI
Greptile Summary
Fixed two bugs in HTTP response handling: corrected status line format from
HTTP/1.1 200 200toHTTP/1.1 200 OKusing Node.js'sSTATUS_CODESmap, and prevented Gemini GET/HEAD requests from incorrectly serializing request bodies.Key Changes:
STATUS_CODESfromnode:httpand use it for proper HTTP status text mapping (forwarder.ts:1967)forwarder.ts:830-834)Confidence Score: 5/5
Important Files Changed
Sequence Diagram
sequenceDiagram participant Client participant ProxyForwarder participant Provider as Provider API Client->>ProxyForwarder: HTTP Request (GET/POST/etc) alt Gemini Provider ProxyForwarder->>ProxyForwarder: Check method type alt GET or HEAD request ProxyForwarder->>ProxyForwarder: Skip body serialization else POST or other methods ProxyForwarder->>ProxyForwarder: JSON.stringify(request body) end else Other Providers ProxyForwarder->>ProxyForwarder: Standard body handling end ProxyForwarder->>Provider: Forward request with proper body Provider->>ProxyForwarder: Response (statusCode) ProxyForwarder->>ProxyForwarder: Map statusCode to STATUS_CODES Note over ProxyForwarder: Was: String(200) → "200"<br/>Now: STATUS_CODES[200] → "OK" ProxyForwarder->>Client: Response with correct statusText