fix: suppress tool-call planning content#161
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds an optional Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Proxy
participant Upstream
Client->>Proxy: request
Proxy->>Upstream: forward request
Upstream-->>Proxy: response (choices with/without tool_calls)
alt tool_calls present
Proxy->>Proxy: for each choice, detect tool_calls → set content = ""
Proxy-->>Client: stream tool_calls chunks + empty assistant content
else no tool_calls
Proxy->>Proxy: strip "thinking" tokens from content
Proxy-->>Client: stream assistant content
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/proxy.tool-forwarding.test.ts (1)
118-179: Add a streaming-path companion regression test.Line [118]-[179] correctly covers non-streaming behavior. Add a
stream: truecase that asserts emptydelta.contentwith preservedtool_callsso SSE behavior is also locked in.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/proxy.tool-forwarding.test.ts` around lines 118 - 179, Add a companion streaming test that mirrors the non-streaming case: create a new it(...) e.g. "suppresses assistant content when upstream returns tool_calls (streaming)", set upstreamResponse the same way, POST to `${proxy.baseUrl}/v1/chat/completions` with stream: true and same messages/tools, consume the SSE response and assert that the streamed assistant delta events contain an empty delta.content and include the tool_calls payload (preserved) — i.e. verify each relevant SSE chunk's parsed delta has content === "" and tool_calls length === 1 (and that the final finish event is handled); reuse the same identifiers (upstreamResponse, proxy.baseUrl, endpoint /v1/chat/completions) so the test structure matches the existing non-streaming test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/proxy.tool-forwarding.test.ts`:
- Around line 118-179: Add a companion streaming test that mirrors the
non-streaming case: create a new it(...) e.g. "suppresses assistant content when
upstream returns tool_calls (streaming)", set upstreamResponse the same way,
POST to `${proxy.baseUrl}/v1/chat/completions` with stream: true and same
messages/tools, consume the SSE response and assert that the streamed assistant
delta events contain an empty delta.content and include the tool_calls payload
(preserved) — i.e. verify each relevant SSE chunk's parsed delta has content ===
"" and tool_calls length === 1 (and that the final finish event is handled);
reuse the same identifiers (upstreamResponse, proxy.baseUrl, endpoint
/v1/chat/completions) so the test structure matches the existing non-streaming
test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 43046a9b-b7f2-4132-b6f7-779a7cebc116
📒 Files selected for processing (4)
src/index.tssrc/proxy.tool-forwarding.test.tssrc/proxy.tssrc/types.ts
4a4c9ee to
6488348
Compare
…sion PR #161 fixed Kimi planning-prose leakage in both non-streaming and SSE paths, but only the non-streaming path had a test. The SSE emission path is what OpenClaw Telegram actually uses in production. Asserts no delta.content chunk leaks the planning text, a tool_calls delta chunk is emitted, and finish_reason is "tool_calls". Uses a distinct user message to bypass the proxy dedup cache, which hashes the request body without accounting for the stream flag — a cached non-streaming response would otherwise be returned as application/json.
…ream cache isolation - fix: suppress tool-call planning prose when upstream returns content alongside tool_calls (thanks @0xCheetah1, #161) - fix: declare mcp.servers.blockrun as noop reload prefix to stop OpenClaw gateway restart loop - fix: dedup + response cache now keyed on original stream intent; SSE and JSON responses no longer collide - test: SSE streaming regression test + dedup isolation regression test
Thanks @0xCheetah1 — follow-up to #161. Some providers (observed on Moonshot Kimi K2.6 via OpenClaw Telegram) mark a turn with finish_reason: "tool_calls" without exposing the tool_calls array at the same inspection point, so the #161 gate (tool_calls.length > 0) let planning prose slip through. Broadens the gate to `endsWithToolCalls || toolCalls.length > 0` in both the non-streaming JSON path and the SSE emission path. Rebased onto v0.12.165's dedup/cache-isolation fix — changes auto-merged; the second "style: format" commit was dropped (prettier already applied upstream). The original PR branch's dedup-test conflict was resolved by keeping both the dedup-isolation test and the new finish_reason test.
Summary
Testing
Summary by CodeRabbit
Improvements
Tests
Documentation
Style