Skip to content

perf(mcp-proxy): reduce large tool response allocations - #2856

Merged
wklken merged 16 commits into
TencentBlueKing:masterfrom
wklken:refactor_mcp_proxy_resolve_oom
Jun 30, 2026
Merged

perf(mcp-proxy): reduce large tool response allocations#2856
wklken merged 16 commits into
TencentBlueKing:masterfrom
wklken:refactor_mcp_proxy_resolve_oom

Conversation

@wklken

@wklken wklken commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Route tools/call upstream responses through raw JSON bytes instead of decoding large JSON into map[string]any.
  • Reuse raw payload metadata for audit logs, access logs, and response-size metrics.
  • Add focused handler characterization tests and a large-response benchmark for before/after verification.

Test plan

  • cd src/mcp-proxy && go test -mod=mod ./pkg/infra/proxy -run TestProxy
  • cd src/mcp-proxy && go test -mod=mod ./pkg/infra/proxy -run '^$' -bench BenchmarkGenToolHandlerLargeJSONResponse -benchmem -benchtime=3x -count=5
  • cd src/mcp-proxy && make dep
  • cd src/mcp-proxy && ./bin/ginkgo -r -mod=vendor ./pkg/infra/proxy/...
  • cd src/mcp-proxy && make lint
  • cd src/mcp-proxy && make test

Made with Cursor

wklken and others added 5 commits June 10, 2026 15:28
Why this change was needed:
The raw payload refactor needs behavior protection and measurable baseline data around the tools/call entry point before changing response materialization.

What changed:
- Added handler-level characterization tests for JSON envelope mode
- Added raw response mode coverage
- Added non-JSON response coverage
- Added a large-response benchmark for envelope and raw-response modes

Problem solved:
Future refactor steps can prove they preserve the MCP response contract while reducing allocations.

Co-authored-by: Cursor <cursoragent@cursor.com>
Why this change was needed:
The tools/call response path needs a single owner for upstream response metadata and raw body bytes before replacing generic JSON decoding.

What changed:
- Added toolResponsePayload for response metadata, raw body, JSON detection, and log preview
- Added envelope and raw-response serialization helpers
- Covered JSON, non-JSON, and invalid JSON behavior

Problem solved:
Response handling can move away from map[string]any without changing the public MCP response shape.

Co-authored-by: Cursor <cursoragent@cursor.com>
Why this change was needed:
Large tools/call responses were decoded into generic Go objects before being serialized back to JSON, multiplying memory usage.

What changed:
- Read upstream response bodies as raw bytes
- Created tool results from serialized JSON bytes
- Preserved envelope and raw-response behavior with characterization tests
- Removed the now-obsolete production envelope map helper

Problem solved:
The main success path no longer needs to materialize upstream JSON as map[string]any.

Co-authored-by: Cursor <cursoragent@cursor.com>
Why this change was needed:
Audit logging was rebuilding and reparsing large tool responses only to record a preview and upstream request id.

What changed:
- Reused toolResponsePayload preview for audit response logs
- Reused raw body length for audit response size
- Reused upstream response header for upstream_request_id

Problem solved:
The audit path no longer performs full response marshal/unmarshal after a successful tools/call.

Co-authored-by: Cursor <cursoragent@cursor.com>
Why this change was needed:
Access logging and metrics were serializing the full MCP tool result to compute previews and sizes.

What changed:
- Threaded toolResponsePayload into tool-call logging and metrics
- Used payload preview for response logs
- Used raw body length for response size metrics
- Kept the old marshal path as a fallback when payload metadata is unavailable

Problem solved:
Large successful tool responses avoid another round of full result serialization in observability code.

Co-authored-by: Cursor <cursoragent@cursor.com>
@wklken

wklken commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator Author

Newest benchmark comparison (count=5, average, benchtime=3x):

Case Before After Improvement
64KB envelope time 13.88 ms/op 2.68 ms/op 80.7%
64KB envelope memory 4.66 MB/op 0.53 MB/op 88.6%
64KB envelope allocs 65,376/op 393/op 99.40%
64KB raw time 13.85 ms/op 2.38 ms/op 82.8%
64KB raw memory 4.65 MB/op 0.43 MB/op 90.8%
64KB raw allocs 65,285/op 395/op 99.40%
1MB envelope time 191.80 ms/op 11.95 ms/op 93.8%
1MB envelope memory 86.51 MB/op 7.73 MB/op 91.1%
1MB envelope allocs 1,036,563/op 420/op 99.96%
1MB raw time 192.69 ms/op 9.23 ms/op 95.2%
1MB raw memory 83.84 MB/op 6.12 MB/op 92.7%
1MB raw allocs 1,036,448/op 416/op 99.96%

Why this change was needed:
The raw payload refactor made the old response-envelope constants and test helper production-dead, leaving confusing code that looked like part of the runtime path.

What changed:
- Removed obsolete response field constants from proxy.go
- Inlined the fallback request_id key in upstream request ID extraction
- Replaced test-only envelope helper usage with explicit fixtures
- Dropped tests that only validated the removed test helper

Problem solved:
The proxy package no longer keeps dead response-envelope scaffolding after migrating response construction to toolResponsePayload.

Co-authored-by: Cursor <cursoragent@cursor.com>
@wklken

This comment was marked as outdated.

wklken

This comment was marked as outdated.

wklken and others added 2 commits June 10, 2026 17:56
Why this change was needed:
The preceding perf refactor (toolResponsePayload) eagerly materialized a
single truncated preview using only AuditLogMaxResponseSize, which had two
side effects: APILogResponseSize/APILogErrorResponseSize knobs were silently
bypassed for tools/call, and the audit log response field for envelope mode
no longer contained the envelope wrapper. The error branch was worse — for
non-2xx upstream responses it overwrote auditResponse with a bare error
string, dropping all upstream body context. For non-JSON error bodies (e.g.
HTML 500 pages) this meant troubleshooting evidence was effectively lost.

What changed:
- config: added AuditLogMaxErrorResponseSize knob (default 16KB) so failed
  calls get a larger preview budget than successful ones; existing audit
  and API log size knobs keep their independent meaning.
- toolResponsePayload: dropped the eagerly-cached truncatedPreview field.
  Added IsSuccess, PickLimit, and an on-demand EnvelopePreview that always
  emits valid JSON, embedding the body either raw (when it is JSON and fits
  the limit) or as a JSON-encoded string (for non-JSON content, truncated
  JSON, malformed JSON, or even binary with invalid UTF-8 substituted as
  U+FFFD). This guarantees non-JSON error bodies are preserved verbatim in
  audit logs instead of being lost as null.
- proxy.go: audit and API log call sites now build the envelope preview
  on demand, picking the per-status limit. Submit-error branch reuses the
  populated payload to surface the upstream envelope on non-2xx instead of
  overwriting with the error message; transport errors still fall back to
  the error message because no upstream response exists.
- Removed dead fallback paths that the previous refactor left behind: the
  unreachable else branch after Submit success, buildToolResult plus its
  four envelope-shape tests, extractUpstreamRequestID, getMapKeys, and the
  associated noisy debug logs. The remaining payload-nil branch in
  serializeToolCallResponse is genuinely only reachable on transport
  errors or panic-before-reader and now emits a warning if it ever fires.
- tests: added EnvelopePreview suite covering empty/raw-JSON/truncated/
  non-JSON HTML/plain-text/malformed-JSON bodies; rewrote
  serializeToolCallResponse tests for the new signature with 2xx vs 5xx
  budget selection, non-JSON envelope preservation, and panic-path
  fallback; added two non-2xx genToolHandler integration specs covering
  JSON and HTML error envelopes; added BenchmarkEnvelopePreview to
  confirm preview cost stays O(min(body, limit)) at 3-6 allocs/op.

Problem solved:
Operators get back the per-call-site log truncation knobs they configured,
non-2xx audit logs retain full upstream envelope detail for troubleshooting
(including HTML error pages and other non-JSON bodies), and the dead-code
removal goal of the preceding refactor commit is now complete.

Co-authored-by: Cursor <cursoragent@cursor.com>
Why this change was needed:
The raw payload refactor accidentally treated non-empty invalid bodies declared as application/json as successful string responses, which hid upstream protocol errors from MCP callers.

What changed:
- Track whether the upstream response explicitly declared a JSON content type separately from whether the body is valid JSON
- Return an error when envelope or raw-response serialization sees invalid non-empty declared JSON
- Add regression coverage at both payload-helper and tool-handler levels while preserving explicit non-JSON string wrapping

Problem solved:
MCP tool calls again fail when an upstream declares JSON but returns malformed JSON, matching the pre-refactor contract instead of silently converting the body to a string.

Co-authored-by: Cursor <cursoragent@cursor.com>
@wklken

wklken commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator Author

major

  • M3. 声明为 JSON 的非法响应体行为从"报错"变成"静默成功"【两方一致,codex 标 major】

位置:proxy.go:1038-1083,response_payload.go:43-49,response_payload.go:69-80,response_payload.go:100-107
问题:旧实现 Content-Type: application/json + 非法 JSON body → 直接报错;新实现 json.Valid 失败时把 body quote 成字符串继续成功返回,将上游协议错误静默吞掉,让下游误以为调用成功。
建议:保持原契约——声明为 JSON 且 body 非空但不合法时应返回 error;只有明确的非 JSON 响应才走字符串包装。

  • M4. raw_response 模式下非 JSON body 被悄悄 JSON 字符串化,违背"原样透传"语义【claude 独立发现】

位置:response_payload.go:101-108(marshalRawResponse)
问题:raw_response 模式 + 非 JSON Content-Type 时,旧实现透传原始文本,新实现通过 responseBodyRawMessage() → json.Marshal(string(rawBody)) 对 body 做了 JSON 字符串化(加引号、特殊字符转义),破坏了"原样透传"语义。
建议:raw 模式 + 非 JSON 时直接 return p.rawBody, nil 透传;或明确文档说明"raw 模式仅对 JSON 上游有效"。

  • M5. json.Valid 对大响应是 O(n) 额外遍历,抵消部分 perf 收益【claude 独立发现】

位置:response_payload.go:60
问题:isJSON: isJSONContentType(contentType) && json.Valid(rawBody) 在 1MB 响应上是一次完整字节扫描,与本 PR 的性能目标相矛盾。
建议:仅以 Content-Type 为判定依据;json.Valid 校验移到 marshal 阶段(marshal 失败时回退);或仅在 envelope 路径校验(raw 模式即使非法 JSON 透传也能 work)。

Minor

  • m2. submit.(json.RawMessage) 类型断言的 !ok 兜底分支不可达【claude 独立发现】

位置:proxy.go:1127-1130
问题:Reader 回调成功路径明确返回 json.RawMessage,!ok 不可能触发,属于过度防御 + 死代码,误导维护者。
建议:删除 !ok 分支,或加注释说明此分支不可达;audit 侧直接信任 responsePayload != nil。

  • m3. truncateBytesForLog 与 util.TruncateJSON 截断逻辑重复维护

位置:response_payload.go(新增函数)vs util/mask.go
问题:两者都用 "...(truncated)" 后缀,但分散在两处,未来改动需要同步修改,且存在潜在的 UTF-8 截断问题。
建议:将截断逻辑统一到 util 包,或直接复用 util.TruncateJSON;顺手改为按 rune 安全截断。

  • m4. Body 为空时 envelope 语义细微变化

位置:response_payload.go:71-79(responseBodyRawMessage)
问题:len(rawBody)==0 时一律返回 null,而旧实现 text/plain + 空 body 时 envelope 中是 "response_body": ""(空字符串)。需确认下游是否有依赖此语义的调用方;建议补充空 body 文本响应的测试 case。

wklken and others added 2 commits June 10, 2026 19:53
Why this change was needed:
Raw response mode should preserve upstream bodies for MCP clients even when an upstream service returns text or another non-JSON payload.

What changed:
- Treat declared JSON as raw JSON only after validation at marshal time
- Encode non-JSON raw response bodies as JSON strings for MCP tool results
- Cover raw text/plain success and error responses in proxy tests
- Keep the tool response reader type contract explicit without violating lint

Problem solved:
MCP raw response mode now returns valid JSON content for non-JSON upstream bodies while still rejecting invalid bodies declared as JSON.

Co-authored-by: Cursor <cursoragent@cursor.com>
Why this change was needed:
A raw-payload refactor changed empty text/plain tool responses in the envelope from an empty string to null, which could break clients that distinguish empty text bodies from absent JSON bodies.

What changed:
- Preserve explicitly read empty non-JSON response bodies as JSON empty strings in client envelopes and log previews
- Add a regression test for text/plain empty body envelope output

Problem solved:
MCP tool envelopes now retain the pre-refactor empty text body semantics while keeping absent or JSON empty bodies as null.

Co-authored-by: Cursor <cursoragent@cursor.com>
wklken

This comment was marked as outdated.

@wklken

wklken commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator Author

review again

wklken

This comment was marked as outdated.

Keep audit status aligned with the tool-call outcome when the go-openapi submit result violates the expected json.RawMessage reader contract.

Also remove the unused status-based payload limit helpers and document the larger audit error response budget in the config template.

Co-authored-by: Cursor <cursoragent@cursor.com>
@wklken
wklken marked this pull request as draft June 11, 2026 08:45
@wklken

wklken commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator Author

PR #2856 Code Review Report

Review 模式: Claude Opus 4.7(codex-internal 不可用)
触发原因: review again 指令
审查时间: 2026-06-12

变更概述

本 PR 解决 mcp-proxy 处理大体积 tools/call 上游响应时的内存放大问题。核心改动:

  1. 响应处理由 decode→re-encode 改为 raw bytes — 避免大 JSON 两次序列化及 float64 精度丢失
  2. 新增 toolResponsePayload 类型 — 承载 raw bytes + 元数据,提供 marshalEnvelope/marshalRawResponse/EnvelopePreview 三个呈现方法
  3. 审计日志/metrics 复用同一 payload — 删除 extractUpstreamRequestID 的"再序列化再反序列化"链路
  4. 失败响应使用更大的日志预算 — 新增 auditLogMaxErrorResponseSize(默认 16384)
  5. 测试覆盖充分 — 新增 response_payload_test.goproxy_refactor_compat_test.goproxy_benchmark_test.go

问题列表

🔴 Blocking

无。

🟠 Major

1. 空 JSON body 行为变更:现返回 null 而不是报错
response_payload.go:91-103

  • 旧实现通过 consumer.Consume 解析空 body 会报 EOF 错误,走 handleToolCallError
  • 新实现对 len(rawBody)==0 统一返回 json.RawMessage("null"),envelope 变成 {"response_body":null}
  • 建议:确认这是预期行为,并在 proxy_refactor_compat_test.go 中增加 "empty body declared as application/json" 的 characterization 测试锁定行为

2. auditResponseSize 语义静默变更

  • 旧:auditResponseSize = int64(len(responseBody))(truncated envelope 长度)
  • 新:auditResponseSize = int64(len(responsePayload.rawBody))(原始上游 body 长度,不截断)
  • 建议:在 PR 描述和 release notes 中注明此变更,依赖 response_body_size 的日志聚合/dashboard 需要知道语义变化

🟡 Minor

  1. marshalEnvelope 不做前置 JSON 校验,marshalRawResponse 做了 — 两方法校验不一致,建议统一
  2. previewBodyAsRawMessage 未做 nil-check,但 responseBodyRawMessagemarshalRawResponse 做了 — 建议统一 nil-check 风格
  3. handleUnexpectedSubmitResultauditStatus/auditLatency 指针传递属于过度防御,建议改为值类型
  4. pickToolCallLogLimit(hasError, success, error) bool flag 调用点可读性差,建议改为命名函数
  5. EnvelopePreviewmarshalEnvelope 中的匿名 struct 定义重复,建议提取为命名类型
  6. Reader 中 io.ReadAll(response.Body()) 没有 max-size 限制(非回归,但建议后续跟进 http.MaxBytesReader

💬 Nit

  1. proxy_benchmark_test.go / proxy_refactor_compat_test.go / proxy_test.go 中三份近乎相同的 fixture 构造代码,建议提取到共享 test helper 文件
  2. serializeToolCallResponse fallback 的 Warn 日志在 transport error 成片出现时会产生噪声,建议降级为 Debug 或加 sampling
  3. config.yaml.tplauditLogMaxErrorResponseSize 的注释建议明确 "failed = HTTP 非 2xx 或 transport error"
  4. truncateBytesForLogpreviewBodyAsRawMessagelimit <= 0 的语义相反,建议统一

优点

  1. 核心思路正确:消除 decode → object graph → re-encode 的重复分配,对 1MB body 场景能显著减少 GC 压力
  2. 抽象边界克制toolResponsePayload 只承载数据,没有引入不必要的 interface/strategy
  3. 测试覆盖全面:compat 测试锁定 envelope 形状,benchmark 支撑性能改进的前后对比
  4. 删除真正死代码extractUpstreamRequestID + getMapKeys 及 6 行 Debug 日志一并清理,净减约 60 行
  5. 失败日志加大预算的设计很务实,16KB 是合理的默认値

总结建议

整体是一次质量较高的性能重构。建议在解决 Major 问题 #1#2 后合并

  • 必修:空 JSON body 行为确认 + compat 测试;auditResponseSize 语义变更注明
  • 可选:合并重复 fixture、提取共享 envelope 类型、pickToolCallLogLimit 改名
  • 后续跟进:MaxBytesReader 限制 Reader 路径的内存上限

[from openclaw-internal]

@wklken
wklken marked this pull request as ready for review June 29, 2026 04:02
@wklken

This comment was marked as outdated.

@wklken

wklken commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator Author

@robot-wklken 你也 review 下这个 PR,然后把报告提交上来

@wklken

This comment was marked as outdated.

@robot-wklken

Copy link
Copy Markdown
Collaborator

PR Review

Review Scope

变更概览

  • tools/call 上游响应改成 raw bytes 贯穿,避免大 JSON decode → object graph → re-encode。
  • 新增 toolResponsePayload 统一处理 wire 输出、audit/API log 预览、以及 response size 统计。
  • 新增 compat / payload / benchmark 测试,覆盖 envelope、raw response、invalid JSON、empty body 和 large response 路径。

历史评论核对

  • 已修复/不再适用:
    • 之前提到的 “2xx 上游但最终 tool call 失败时,API log 仍走 success budget” 现在已经修好:serializeToolCallResponse(..., hasError, ...) 会按最终 hasError 选择 APILogErrorResponseSize,并且有回归测试覆盖 2xx + result.IsError 场景。
    • 之前提到 benchmark 缺少 alloc 统计的问题也已经修好,当前两个 benchmark 都调用了 b.ReportAllocs()

发现的问题

Suggestion

  • [【新】] [pkg/infra/proxy/response_payload.go:67]
    • truncateBytesForLog 这里是否应该和 LogTruncate 的配置语义保持一致?现在配置注释写的是“string length / number of characters”,但实现按 byte 长度截断,而且 body[:limit] 可能把 UTF-8 rune 切半,日志里会出现无效 UTF-8。
    • 如果这里打算继续按 byte 工作,是否至少把配置/注释改成 byte;如果想保持“字符数”语义,是否改成 rune-aware truncation 或直接复用统一 helper?

Suggestion

  • [【已提及】] [pkg/infra/proxy/proxy.go:1121]
    • response_body_size 现在记录的是 len(responsePayload.rawBody),不再是最终写进 audit log 的 preview 长度;这个字段的语义变化是有意的吗?
    • 如果这是有意的变化,是否值得在 PR 描述 / release note 里点明,避免现有日志聚合或 dashboard 继续把它当成“实际落盘响应片段长度”来解释?

优点

  • toolResponsePayload 的边界比较克制,客户端输出、log preview、metrics size 都沿着同一份 raw bytes 走,主路径没有再回到 map/object graph。
  • 回归测试补得比较到位:invalid declared JSON、empty text body、raw response、non-2xx、large JSON benchmark 都覆盖到了。
  • 我本地跑了 go test -mod=mod ./pkg/infra/proxy ./pkg/config,这两组测试当前是通过的。

测试与文档建议

  • 如果保留当前截断实现,建议至少补一个多字节 UTF-8 body 的 preview case,把“按 byte 还是按字符”锁死。
  • 如果 response_body_size 的新语义是预期行为,建议在 PR 描述里显式写出来。

结论

  • 这轮我没有看到新的 blocking 问题。
  • 我倾向于可以合并,但建议先确认上面两个语义点,尤其是 UTF-8/limit 的契约是否与配置注释一致。

[from-codex local repo]

@wklken

wklken commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator Author

review again

…y_resolve_oom

# Conflicts:
#	src/mcp-proxy/pkg/infra/proxy/proxy_test.go
wklken

This comment was marked as outdated.

@wklken

wklken commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator Author

PR #2856 Code Review 汇总报告

由 OpenClaw K 直接撰写(codex-internal 和 claude-internal 在 cron 环境下均不可用)| review again 触发
PR 标题:perf(mcp-proxy): reduce large tool response allocations
PR 作者:wklken
变更统计:10 files, +1580 -223

变更概述

本 PR 的核心目标是解决 mcp-proxy 在处理大 tool response 时的 OOM 问题。主要手段是引入 toolResponsePayload 类型,将上游响应体以 []byte 原始字节形式保存,避免在 Reader 中将其反序列化为 map[string]any,从而消除大 JSON 响应的双倍内存开销(一份在 map[string]any,一份在 mcp.CallToolResult 的 JSON 文本中)。

主要变更:

  1. 新增 response_payload.go + response_payload_test.go:核心 toolResponsePayload 类型及其测试
  2. 重构 proxy.go 中的 genToolHandlerReader 直接返回 json.RawMessage,移除 buildToolResponseEnvelopebuildToolResult
  3. 新增 proxy_benchmark_test.go:性能基准测试,验证重构前后的内存分配差异
  4. 新增 proxy_refactor_compat_test.go:兼容性测试,确保重构后 envelope 形状不变
  5. 配置层新增 AuditLogMaxErrorResponseSize:失败响应的审计日志保留更大空间(16384)
  6. 移除 helpers_test.go 中的 buildToolResponseEnvelope 测试(函数已删除)
  7. serializeToolCallResponse 重构:优先使用 payload.EnvelopePreview,fallback 路径增加 warn 日志

Report

Critical Issues


High Issues

H1: handleUnexpectedSubmitResult 的错误处理路径中 handleToolCallError 会被 defer 中的 logToolCall/recordToolCallMetrics 重复处理

genToolHandler 的 Submit 成功路径中,新增了对 submit 类型的断言检查:

responseBytes, ok := submit.(json.RawMessage)
if !ok {
    return handleUnexpectedSubmitResult(...), nil
}

handleUnexpectedSubmitResult 内部调用 handleToolCallError,设置 auditStatus = "failed" 并返回 *mcp.CallToolResult(error 结果)。但注意 handleUnexpectedSubmitResult 返回后,外层 return ..., nilnil err 会导致 defer 中的 logToolCall 认为 err == nil,从而 hasError = false,打出 success 日志——这与实际语义不符。

不过,这个场景本身极难触发(openAPIClient.Submit 正常返回时 submit 必然是 json.RawMessage),属于防御性代码。严重性在于日志语义不一致,不影响正确性。建议:在 handleUnexpectedSubmitResult 返回的结果中设置 IsError = true,或在 defer 中增加判断逻辑。

H2: response_payload.gomarshalEnvelope 对无效 JSON 的错误包装可能掩盖根本原因

if p.isDeclaredJSON {
    return nil, p.invalidDeclaredJSONBodyError(err)
}

json.Marshal(envelope) 失败且 p.isDeclaredJSON 时,错误被包装为 invalid JSON response body for Content-Type "application/json": <marshal err>。但此时 json.Marshal 失败的原因几乎不可能是 upstream body 导致的(envelope 结构是固定的),而是系统级问题(OOM、stack overflow 等)。这个错误包装可能误导排查方向。建议:对 json.Marshal(envelope) 的错误不做特殊包装,直接返回原始 err。


Medium Issues

M1: proxy_refactor_compat_test.gocallCompatToolHandler 辅助函数没有在 diff 中完整展示,需确认其正确处理了 rawResponseEnabled 参数

diff 中 proxy_refactor_compat_test.go 被截断,无法看到 callCompatToolHandler 的完整实现。从测试用例的使用方式推断,它应该正确传递了 rawResponseEnabledGetter建议:确认 callCompatToolHandler 内部使用的 rawResponseEnabledGetter 闭包确实捕获了传入的 rawResponseEnabled 参数,而非始终返回 false

M2: pickToolCallLogLimit 函数命名和语义

pickToolCallLogLimit 根据 hasError 选择 errorLimitsuccessLimit。函数名中的 ToolCall 前缀与包内其他公共函数风格一致,但该函数仅被 serializeToolCallResponsegenToolHandler 中的 audit log 部分使用,且逻辑极其简单(一个 if-else),提取为独立函数是否过度抽象值得讨论。当前代码中 logTruncate.GetAuditLogMaxResponseSize()logTruncate.GetAuditLogMaxErrorResponseSize() 的调用处已经有足够的上下文说明语义,提取函数后反而增加了跳转成本。

结论:当前可以接受,但如果后续有更多调用点选择日志限制,再提取不迟。不属于必须修改的问题。

M3: AuditLogMaxErrorResponseSize 默认值为 16384,是 AuditLogMaxResponseSize(4096)的 4 倍,配置文件中已添加但未在文档中说明

新增配置项 auditLogMaxErrorResponseSize: 16384 出现在 config.yaml.tpl 中,但 PR 描述(body)未提及这个配置项的用途。建议:在 PR 描述中补充这个配置项的说明,或在 config.go 的字段注释中已说明,可忽略此条。


Low Issues

L1: response_payload.gopreviewBodyAsRawMessage 对 binary body 的处理依赖 json.Marshal 的 stdlib 行为(替换无效 UTF-8 为 U+FFFD)

对于二进制响应体(如 protobuf、gzip),json.Marshal(truncated) 会产生大量 U+FFFD 替换字符,导致预览内容可读性极差且体积膨胀(每个非法字节变成 3 字节的 \ufffd)。虽然 truncateBytesForLog 限制了大小,但截断后的字符串经 json.Marshal 后可能进一步膨胀。建议:对于 !isDeclaredJSON && len(p.rawBody) > limit 的情况,考虑使用 base64 编码或明确标注 [binary body, N bytes]

L2: benchmark 测试中 buildBenchmarkJSONBody 使用 crypto/rand 生成随机 JSON body,每次调用都重新生成,可能导致 benchmark 结果有噪声

BenchmarkGenToolHandlerLargeJSONResponsebuildBenchmarkJSONBody(size) 每次构造随机 JSON,不同 iteration 之间的内存分配模式可能有差异。不过 b.ReportAllocs() 报告的是每次 iteration 的分配次数,随机性影响不大。建议:可以使用固定内容的 JSON body(如预生成的 []byte 重复拼接),减少 benchmark 噪音。

L3: proxy_benchmark_test.goinitBenchmarkRuntime 函数未在 diff 中展示

该函数在 proxy_test.go 中定义(不在本 PR 变更范围内),benchmark 测试依赖它。需确认 initBenchmarkRuntime 正确初始化了 config.G 等全局状态,否则 benchmark 可能 panic 或使用默认配置导致结果不准确。


Dismissed Findings

DF1: extractUpstreamRequestID 及相关辅助函数被整块删除,是否破坏了对旧版 upstream 响应的兼容?

初看会担心删除 extractUpstreamRequestID 后,从 mcp.CallToolResult 中提取 request_id 的能力丢失。但实际上,本 PR 引入的 toolResponsePayloadReader 中直接从 response header 读取 upstreamRequestIDresponse.GetHeader(constant.BkGatewayRequestIDKey)),不再需要从序列化后的 envelope 中反向解析。这是更可靠的做法,因为 header 是 upstream 响应的权威来源,而 envelope 是 proxy 自己构造的。

DF2: 移除 buildToolResult 后,buildToolResultFromJSONBytes 直接把 []byte 当字符串放入 mcp.TextContent,是否会有编码问题?

json.RawMessage 本质是 []byte,转为 string 是零拷贝语义(Go 中 string([]byte) 会复制,但 TextContent.Text 字段是 string 类型,赋值时必然复制)。对于合法的 UTF-8 JSON(isDeclaredJSON 路径),这没有问题。对于非 JSON 响应体,marshalRawResponse 通过 json.Marshal(string(p.rawBody)) 将其编码为 JSON 字符串,也不会有编码问题。


Agent Agreement Summary

Issue Codex Claude Verdict
H1: defer 中 err 语义不一致 待汇总 真实问题(Low优先级,因极难触发)
H2: marshalEnvelope 错误包装误导 待汇总 真实问题(Medium优先级)
M1: callCompatToolHandler 正确性 待汇总 是(需确认) 需人工确认
L1: binary body 预览膨胀 待汇总 真实问题(Low优先级,可后续优化)

合并建议

merge after fixes

本 PR 设计思路清晰,核心目标(避免大 JSON 响应的双倍内存分配)明确,实现路径合理。toolResponsePayload 的引入将原始字节与展示逻辑分离,是正确的抽象方向。

主要需要修复的问题:

  1. H2(Medium):修正 marshalEnvelope 中对 json.Marshal 错误的包装方式,避免误导排查
  2. H1(High 降级为 Low):确认 handleUnexpectedSubmitResult 返回后的 defer 日志语义是否正确,如不正确则修复
  3. M1(Medium):确认 proxy_refactor_compat_test.gocallCompatToolHandler 实现正确性

L1/L2/L3 可在后续 PR 中优化,不阻塞当前合并。


新增测试覆盖评价

  • response_payload_test.go:214 行,覆盖了 newToolResponsePayloadpreviewBodyAsRawMessagemarshalEnvelopemarshalRawResponse 的主要路径,包括 JSON/非 JSON、空 body、截断等场景,覆盖充分
  • proxy_refactor_compat_test.go:277 行,覆盖了 envelope 形状兼容性、raw response 模式、无效 JSON 错误路径,覆盖充分
  • proxy_benchmark_test.go:261 行,覆盖了 64KB/1MB 响应体的 envelope/raw 两种模式,benchmark 设计合理
  • config_test.go:新增 GetAuditLogMaxErrorResponseSize 测试,覆盖充分

由 OpenClaw K 直接撰写 | [from openclaw-internal]


@wklken 您的 PR 已完成 review,请查看以上意见。

@wklken

wklken commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator Author

对原 review report 中 review points 的逐项复核如下,复核对象为当前 PR head 51977af01a50553c357d54f8f1f6e599eb5db122

Review point 结论 证据 建议
H1:handleUnexpectedSubmitResult 返回 (error result, nil) 后,defer 中 logToolCall / recordToolCallMetrics 会按 err == nil 误记 success 不成立 BKAIDev defer、metrics、API log 都检查 `err != nil
H2:marshalEnvelope 对无效 JSON 的错误包装会误导,因为 json.Marshal(envelope) 失败几乎不可能由 upstream body 导致 不成立 envelope 的 response_bodyjson.RawMessage,如果 declared JSON body 非法,json.Marshal(envelope) 会因该 raw message 失败;这里包装为 invalid JSON response body 是指向真实原因。见 response_payload.go:184-208,测试见 response_payload_test.go:173-180 不需要修复
M1:callCompatToolHandler 可能没有正确处理 rawResponseEnabled 参数 不成立 / 已确认 完整文件中 callCompatToolHandler(upstream, rawResponseEnabled) 在闭包里直接 return rawResponseEnabled。见 proxy_refactor_compat_test.go:187-208 不需要修复
M2:pickToolCallLogLimit 可能过度抽象 部分正确,但只是风格讨论 函数确实只是简单 if-else,当前用于 audit log 和 API log 两处,语义明确。见 proxy.go:1119-1123, proxy.go:1377-1383, proxy.go:1411-1416 不阻塞;可保留
M3:新增 AuditLogMaxErrorResponseSize=16384,PR 描述未说明 部分正确 PR body 未提及该配置;但代码字段注释和 config.yaml.tpl 已说明用途。见 config.go:197-200, config.yaml.tpl:88-89 仅 PR 描述补一句即可;不属于代码 blocker
L1:binary body 预览依赖 json.Marshal(string(...)),无效 UTF-8 会替换为 U+FFFD,可能膨胀且可读性差 正确,但低优先级 代码注释已明确记录该行为。见 response_payload.go:121-153 可后续优化,例如 binary preview 用占位说明或 base64;不阻塞本 PR
L2:benchmark 使用 crypto/rand 生成随机 JSON body,导致结果噪声 不成立 buildBenchmarkJSONBody 是固定 item 拼接,确定性生成;crypto/rand 只用于生成 RSA key 初始化测试上下文。见 proxy_benchmark_test.go:247-260, proxy_benchmark_test.go:215-222 不需要修复
L3:initBenchmarkRuntime 未展示,需确认 benchmark 初始化 不成立 / 已确认 initBenchmarkRuntime 在当前 proxy_benchmark_test.go 中,初始化了 config.G、logger、shared transport。见 proxy_benchmark_test.go:183-210 不需要修复
DF1:删除 extractUpstreamRequestID 是否破坏旧版 upstream 响应兼容 不成立,原 report dismiss 正确 Reader 中通过 response.GetHeader(constant.BkGatewayRequestIDKey) 填充 toolResponsePayload.upstreamRequestID。见 proxy.go:1065-1070 保持现状
DF2:移除 buildToolResult 后,buildToolResultFromJSONBytes[]byte 转 string 放入 TextContent 是否有编码问题 不成立,原 report dismiss 基本正确 新路径传入的是 marshalEnvelope / marshalRawResponse 产出的 JSON bytes;非 JSON body 会先被 JSON string 编码。见 proxy.go:169-175, response_payload.go:184-240 保持现状

验证命令:

  • cd src/mcp-proxy && ./bin/ginkgo -r -mod=vendor ./pkg/infra/proxy/...:176 passed, 0 failed
  • cd src/mcp-proxy && ./bin/ginkgo -r -mod=vendor ./pkg/config/...:67 passed, 0 failed

结论:阻塞项 H1/H2/M1 不成立;本 PR 不需要因为这些点继续改代码。

wklken added 2 commits June 30, 2026 14:25
Why this change was needed:
Review of the MCP proxy raw-response refactor found several audit and JSON edge cases where the new payload path could diverge from the previous behavior or log misleading success details.

What changed:
- Treat structured +json media types as JSON while avoiding substring matches
- Reject empty declared JSON bodies on content responses and validate raw/envelope paths consistently
- Keep audit failure state for early decode errors and rethrow panics after audit logging
- Re-render unexpected submit audit previews with the error response budget
- Document byte-oriented log preview limits in AGENTS.md

Problem solved:
MCP tool calls now keep client-visible JSON handling and audit logs aligned across success, upstream error, and defensive failure paths.
Why this change was needed:
The raw-response and envelope response paths had duplicated body-shaping logic for nil payloads, empty bodies, declared JSON, and non-JSON responses.

What changed:
- Extracted bodyAsJSONValue as the shared response-body conversion helper
- Updated marshalEnvelope to embed the shared JSON value as response_body
- Updated marshalRawResponse to return the same JSON value as the full MCP response

Problem solved:
Future changes to upstream response body semantics now happen in one place while preserving the existing raw_response_enabled and envelope output contracts.
@wklken
wklken merged commit 5f4594f into TencentBlueKing:master Jun 30, 2026
3 checks passed
Carlmac pushed a commit to Carlmac/blueking-apigateway that referenced this pull request Jul 1, 2026
…ing#2856)

* test(mcp-proxy): lock large response tool handler behavior

Why this change was needed:
The raw payload refactor needs behavior protection and measurable baseline data around the tools/call entry point before changing response materialization.

What changed:
- Added handler-level characterization tests for JSON envelope mode
- Added raw response mode coverage
- Added non-JSON response coverage
- Added a large-response benchmark for envelope and raw-response modes

Problem solved:
Future refactor steps can prove they preserve the MCP response contract while reducing allocations.

Co-authored-by: Cursor <cursoragent@cursor.com>

* refactor(mcp-proxy): add raw tool response payload helper

Why this change was needed:
The tools/call response path needs a single owner for upstream response metadata and raw body bytes before replacing generic JSON decoding.

What changed:
- Added toolResponsePayload for response metadata, raw body, JSON detection, and log preview
- Added envelope and raw-response serialization helpers
- Covered JSON, non-JSON, and invalid JSON behavior

Problem solved:
Response handling can move away from map[string]any without changing the public MCP response shape.

Co-authored-by: Cursor <cursoragent@cursor.com>

* refactor(mcp-proxy): route tool responses through raw payload bytes

Why this change was needed:
Large tools/call responses were decoded into generic Go objects before being serialized back to JSON, multiplying memory usage.

What changed:
- Read upstream response bodies as raw bytes
- Created tool results from serialized JSON bytes
- Preserved envelope and raw-response behavior with characterization tests
- Removed the now-obsolete production envelope map helper

Problem solved:
The main success path no longer needs to materialize upstream JSON as map[string]any.

Co-authored-by: Cursor <cursoragent@cursor.com>

* perf(mcp-proxy): reuse raw payload metadata for audit logs

Why this change was needed:
Audit logging was rebuilding and reparsing large tool responses only to record a preview and upstream request id.

What changed:
- Reused toolResponsePayload preview for audit response logs
- Reused raw body length for audit response size
- Reused upstream response header for upstream_request_id

Problem solved:
The audit path no longer performs full response marshal/unmarshal after a successful tools/call.

Co-authored-by: Cursor <cursoragent@cursor.com>

* perf(mcp-proxy): reuse raw payload metadata for logs and metrics

Why this change was needed:
Access logging and metrics were serializing the full MCP tool result to compute previews and sizes.

What changed:
- Threaded toolResponsePayload into tool-call logging and metrics
- Used payload preview for response logs
- Used raw body length for response size metrics
- Kept the old marshal path as a fallback when payload metadata is unavailable

Problem solved:
Large successful tool responses avoid another round of full result serialization in observability code.

Co-authored-by: Cursor <cursoragent@cursor.com>

* refactor(mcp-proxy): remove dead response envelope helpers

Why this change was needed:
The raw payload refactor made the old response-envelope constants and test helper production-dead, leaving confusing code that looked like part of the runtime path.

What changed:
- Removed obsolete response field constants from proxy.go
- Inlined the fallback request_id key in upstream request ID extraction
- Replaced test-only envelope helper usage with explicit fixtures
- Dropped tests that only validated the removed test helper

Problem solved:
The proxy package no longer keeps dead response-envelope scaffolding after migrating response construction to toolResponsePayload.

Co-authored-by: Cursor <cursoragent@cursor.com>

* fix(mcp-proxy): preserve upstream envelope in tool call audit/API logs

Why this change was needed:
The preceding perf refactor (toolResponsePayload) eagerly materialized a
single truncated preview using only AuditLogMaxResponseSize, which had two
side effects: APILogResponseSize/APILogErrorResponseSize knobs were silently
bypassed for tools/call, and the audit log response field for envelope mode
no longer contained the envelope wrapper. The error branch was worse — for
non-2xx upstream responses it overwrote auditResponse with a bare error
string, dropping all upstream body context. For non-JSON error bodies (e.g.
HTML 500 pages) this meant troubleshooting evidence was effectively lost.

What changed:
- config: added AuditLogMaxErrorResponseSize knob (default 16KB) so failed
  calls get a larger preview budget than successful ones; existing audit
  and API log size knobs keep their independent meaning.
- toolResponsePayload: dropped the eagerly-cached truncatedPreview field.
  Added IsSuccess, PickLimit, and an on-demand EnvelopePreview that always
  emits valid JSON, embedding the body either raw (when it is JSON and fits
  the limit) or as a JSON-encoded string (for non-JSON content, truncated
  JSON, malformed JSON, or even binary with invalid UTF-8 substituted as
  U+FFFD). This guarantees non-JSON error bodies are preserved verbatim in
  audit logs instead of being lost as null.
- proxy.go: audit and API log call sites now build the envelope preview
  on demand, picking the per-status limit. Submit-error branch reuses the
  populated payload to surface the upstream envelope on non-2xx instead of
  overwriting with the error message; transport errors still fall back to
  the error message because no upstream response exists.
- Removed dead fallback paths that the previous refactor left behind: the
  unreachable else branch after Submit success, buildToolResult plus its
  four envelope-shape tests, extractUpstreamRequestID, getMapKeys, and the
  associated noisy debug logs. The remaining payload-nil branch in
  serializeToolCallResponse is genuinely only reachable on transport
  errors or panic-before-reader and now emits a warning if it ever fires.
- tests: added EnvelopePreview suite covering empty/raw-JSON/truncated/
  non-JSON HTML/plain-text/malformed-JSON bodies; rewrote
  serializeToolCallResponse tests for the new signature with 2xx vs 5xx
  budget selection, non-JSON envelope preservation, and panic-path
  fallback; added two non-2xx genToolHandler integration specs covering
  JSON and HTML error envelopes; added BenchmarkEnvelopePreview to
  confirm preview cost stays O(min(body, limit)) at 3-6 allocs/op.

Problem solved:
Operators get back the per-call-site log truncation knobs they configured,
non-2xx audit logs retain full upstream envelope detail for troubleshooting
(including HTML error pages and other non-JSON bodies), and the dead-code
removal goal of the preceding refactor commit is now complete.

Co-authored-by: Cursor <cursoragent@cursor.com>

* fix(mcp-proxy): reject invalid declared JSON responses

Why this change was needed:
The raw payload refactor accidentally treated non-empty invalid bodies declared as application/json as successful string responses, which hid upstream protocol errors from MCP callers.

What changed:
- Track whether the upstream response explicitly declared a JSON content type separately from whether the body is valid JSON
- Return an error when envelope or raw-response serialization sees invalid non-empty declared JSON
- Add regression coverage at both payload-helper and tool-handler levels while preserving explicit non-JSON string wrapping

Problem solved:
MCP tool calls again fail when an upstream declares JSON but returns malformed JSON, matching the pre-refactor contract instead of silently converting the body to a string.

Co-authored-by: Cursor <cursoragent@cursor.com>

* fix(mcp-proxy): encode raw non-json tool responses

Why this change was needed:
Raw response mode should preserve upstream bodies for MCP clients even when an upstream service returns text or another non-JSON payload.

What changed:
- Treat declared JSON as raw JSON only after validation at marshal time
- Encode non-JSON raw response bodies as JSON strings for MCP tool results
- Cover raw text/plain success and error responses in proxy tests
- Keep the tool response reader type contract explicit without violating lint

Problem solved:
MCP raw response mode now returns valid JSON content for non-JSON upstream bodies while still rejecting invalid bodies declared as JSON.

Co-authored-by: Cursor <cursoragent@cursor.com>

* fix(mcp-proxy): preserve empty text response envelopes

Why this change was needed:
A raw-payload refactor changed empty text/plain tool responses in the envelope from an empty string to null, which could break clients that distinguish empty text bodies from absent JSON bodies.

What changed:
- Preserve explicitly read empty non-JSON response bodies as JSON empty strings in client envelopes and log previews
- Add a regression test for text/plain empty body envelope output

Problem solved:
MCP tool envelopes now retain the pre-refactor empty text body semantics while keeping absent or JSON empty bodies as null.

Co-authored-by: Cursor <cursoragent@cursor.com>

* fix(review/comments): fix cr comments

* fix(mcp-proxy): handle unexpected submit result as tool error

Keep audit status aligned with the tool-call outcome when the go-openapi submit result violates the expected json.RawMessage reader contract.

Also remove the unused status-based payload limit helpers and document the larger audit error response budget in the config template.

Co-authored-by: Cursor <cursoragent@cursor.com>

* test(mcp-proxy/refactor): add tests

* fix(mcp-proxy): preserve audit semantics for raw responses

Why this change was needed:
Review of the MCP proxy raw-response refactor found several audit and JSON edge cases where the new payload path could diverge from the previous behavior or log misleading success details.

What changed:
- Treat structured +json media types as JSON while avoiding substring matches
- Reject empty declared JSON bodies on content responses and validate raw/envelope paths consistently
- Keep audit failure state for early decode errors and rethrow panics after audit logging
- Re-render unexpected submit audit previews with the error response budget
- Document byte-oriented log preview limits in AGENTS.md

Problem solved:
MCP tool calls now keep client-visible JSON handling and audit logs aligned across success, upstream error, and defensive failure paths.

* refactor(mcp-proxy): share response payload JSON shaping

Why this change was needed:
The raw-response and envelope response paths had duplicated body-shaping logic for nil payloads, empty bodies, declared JSON, and non-JSON responses.

What changed:
- Extracted bodyAsJSONValue as the shared response-body conversion helper
- Updated marshalEnvelope to embed the shared JSON value as response_body
- Updated marshalRawResponse to return the same JSON value as the full MCP response

Problem solved:
Future changes to upstream response body semantics now happen in one place while preserving the existing raw_response_enabled and envelope output contracts.

---------

Co-authored-by: Cursor <cursoragent@cursor.com>
@wklken

wklken commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator Author
image image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants