fix(amp): 修复Amp CLI 集成 缺失/无效 signature 导致的 TUI 崩溃与上游 400 问题#2403
fix(amp): 修复Amp CLI 集成 缺失/无效 signature 导致的 TUI 崩溃与上游 400 问题#2403luispater merged 2 commits intorouter-for-me:devfrom
Conversation
![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 enhances the AMP module by implementing request sanitization and improving response rewriting logic. Key changes include the addition of SanitizeAmpRequestBody to remove thinking blocks with invalid signatures from upstream requests and updates to ResponseRewriter to inject required signatures and suppress thinking blocks for TUI compatibility. The rewriter now also supports dynamic transitions to streaming mode based on content type or buffer limits. Review feedback identifies that the streaming response path is currently missing the logic to suppress thinking blocks and provides a suggestion for more robust test initialization.
| func (rw *ResponseRewriter) rewriteStreamEvent(data []byte) []byte { | ||
| // Inject empty signature where needed | ||
| data = ensureAmpSignature(data) | ||
|
|
||
| // Rewrite model name | ||
| if rw.originalModel != "" { | ||
| for _, path := range modelFieldPaths { | ||
| if gjson.GetBytes(data, path).Exists() { | ||
| data, _ = sjson.SetBytes(data, path, rw.originalModel) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return bytes.Join(lines, []byte("\n")) | ||
| return data | ||
| } |
There was a problem hiding this comment.
The streaming response path seems to be missing the logic to suppress thinking blocks. The function suppressAmpThinking has been implemented to handle this for streaming events (like content_block_start, thinking_delta, etc.) but it's not being called from rewriteStreamEvent.
This is likely an oversight from the refactoring where rewriteStreamChunk was changed to call rewriteStreamEvent instead of rewriteModelInResponse. Without this, streaming thinking blocks won't be suppressed, which is one of the main goals of this PR.
You should call suppressAmpThinking here. Note that suppressAmpThinking can return nil for suppressed events, so the calling function rewriteStreamChunk will also need a small change to handle this and avoid writing an empty data: line.
func (rw *ResponseRewriter) rewriteStreamEvent(data []byte) []byte {
// Suppress thinking blocks before any other processing.
data = rw.suppressAmpThinking(data)
if len(data) == 0 {
return nil
}
// Inject empty signature where needed
data = ensureAmpSignature(data)
// Rewrite model name
if rw.originalModel != "" {
for _, path := range modelFieldPaths {
if gjson.GetBytes(data, path).Exists() {
data, _ = sjson.SetBytes(data, path, rw.originalModel)
}
}
}
return data
}| } | ||
|
|
||
| func TestRewriteStreamChunk_SuppressesThinkingContentBlockFrames(t *testing.T) { | ||
| rw := &ResponseRewriter{} |
There was a problem hiding this comment.
It's good practice to initialize the ResponseRewriter fully for tests, similar to how NewResponseRewriter does it. While the implementation currently handles a nil map for suppressedContentBlock with a nil check, explicitly initializing it makes the test setup more robust and less dependent on that implementation detail.
| rw := &ResponseRewriter{} | |
| rw := &ResponseRewriter{suppressedContentBlock: make(map[int]struct{})} |
![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: 04ba8c8bc3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| func (rw *ResponseRewriter) rewriteStreamEvent(data []byte) []byte { | ||
| // Inject empty signature where needed | ||
| data = ensureAmpSignature(data) | ||
|
|
There was a problem hiding this comment.
suppressAmpThinking now contains the logic that drops streamed content_block_start/thinking_delta/content_block_stop frames, but rewriteStreamChunk sends SSE JSON to rewriteStreamEvent, and this function never calls that suppression path. In practice, any streamed response that includes thinking frames will still emit them, so the Amp client compatibility issue this commit targets remains in the streaming path.
Useful? React with 👍 / 👎.
| return bytes.Contains(data, []byte("data:")) || | ||
| bytes.Contains(data, []byte("event:")) || |
There was a problem hiding this comment.
The SSE heuristic treats any chunk containing substrings like "data:" or "event:" as streaming, even for normal JSON bodies. When that happens, Write switches to streaming mode and bypasses rewriteModelInResponse, so model/signature rewrites are skipped unless lines literally start with data: . A non-streaming JSON response containing text such as "...data: ..." is enough to trigger this regression.
Useful? React with 👍 / 👎.
…ion, test init - Call suppressAmpThinking in rewriteStreamEvent for streaming path - Handle nil return from suppressAmpThinking to skip suppressed events - Narrow looksLikeSSEChunk to line-prefix detection (HasPrefix vs Contains) - Initialize suppressedContentBlock map in test
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b15453c369
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| bodyBytes = SanitizeAmpRequestBody(bodyBytes) | ||
|
|
||
| // Restore the body for the handler to read | ||
| c.Request.Body = io.NopCloser(bytes.NewReader(bodyBytes)) |
There was a problem hiding this comment.
When SanitizeAmpRequestBody removes invalid thinking blocks, bodyBytes can become shorter, but the request length metadata is never updated before the proxy path runs. In the len(providers)==0 branch, proxy.ServeHTTP forwards c.Request with the stale Content-Length/ContentLength, which can cause outbound write failures (body shorter than declared length) and return gateway errors instead of reaching Amp upstream. After replacing the body, also update or clear the length fields.
Useful? React with 👍 / 👎.
2 participants
变更说明
本 PR 修复了 Amp 相关请求/响应里
signature缺失或无效时导致的两个问题：Amp TUI 渲染崩溃
tool_use/thinking/ streamingcontent_block缺少signature字段时，Amp TUI 可能触发P.signature.length异常，表现为消息消失或界面无法正常渲染。上游 API 400
thinkingblock 带有空、空白或非字符串的signature时，请求转发到上游后可能被拒绝，返回400 invalid_request_error。具体修改
1. 响应侧兼容处理
tool_use/thinkingblock 补齐缺失的signaturecontent_block补齐缺失的signaturetool_use时，抑制普通响应中的thinkingcontent_block形态 的处理：thinking的content_block_startthinking_deltacontent_block_stoptool_useblock，避免 Amp TUI 在流式场景下继续触发兼容性问题2. 请求侧清理
thinkingblocksignature缺失signature为nullsignature为空字符串signature仅包含空白字符signature不是字符串类型影响范围
仅影响 Amp 模块中的请求/响应改写逻辑，不改变其他 provider 的正常行为。
目标是增强对 Amp 客户端的兼容性，避免因历史脏数据或流式响应结构导致 UI 崩溃和上游拒绝请求。
验证情况
已验证以下定向测试通过：
go test ./internal/api/modules/amp -run "TestRewriteStreamChunk_SuppressesThinkingContentBlockFrames|TestSanitizeAmpRequestBody_RemovesWhitespaceAndNonStringSignatures" -count=1go test ./internal/api/modules/amp -run TestRewriteStreamChunk_MultipleEvents -count=1另外，先前已验证：
go build -o /tmp/cliproxyapi-plus-test ./cmd/server背景
该修复主要针对以下现象：
P.signature.length400 invalid_request_errorthinking与tool_use时，Amp 客户端兼容性较差