fix(qqbot): drop dangling surrogates in gateway log previews#98129
fix(qqbot): drop dangling surrogates in gateway log previews#98129wangmiao0668000666 wants to merge 3 commits into
Conversation
…dModelFetch Inject buildGuardedModelFetch with resolved Azure base URL into both SDK constructors (OpenAI and AzureOpenAI path) so Azure Responses requests route through OpenClaw's guarded transport (SSRF policy, timeout, retry limiting, SSE/JSON synthesis caps). The non-OK response body cap is applied lazily inside the shared sanitizeOpenAISdkSseResponse guard via TransformStream — closes the unbounded response.text() OOM path for hostile 4xx/5xx responses while preserving the OpenAI SDK's ability to cancel and retry. - src/llm/providers/azure-openai-responses.ts: pass guardedFetch into both OpenAI and AzureOpenAI SDK constructors (no per-provider wrapper) - src/agents/provider-transport-fetch.ts: add capNonOkResponseBodyLazily helper and wire into sanitizeOpenAISdkSseResponse for !response.ok - src/agents/provider-transport-fetch.test.ts: 2 inline tests (cap fires + SDK cancel preserves source) Co-Authored-By: Claude <[email protected]>
|
Codex review: needs real behavior proof before merge. Reviewed June 30, 2026, 9:10 AM ET / 13:10 UTC. Summary PR surface: Source +38, Tests +285. Total +323 across 11 files. Reproducibility: yes. Source inspection of current main shows raw Review metrics: 1 noteworthy metric.
Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Keep the focused helper substitution, but require a production-path Do we have a high-confidence way to reproduce the issue? Yes. Source inspection of current main shows raw Is this the best way to solve the issue? No as merge-ready. Reusing Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 6cb82eaab865. Label changesLabel justifications:
Evidence reviewedPR surface: Source +38, Tests +285. Total +323 across 11 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
…ith surrogate-safe ttsText
|
@clawsweeper re-review PR #98129 body has been refreshed to match live HEAD This update adds:
Test imports oxlint EXIT=0 on the new test file. Body / HEAD consistency verified via |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
Closing this PR in favor of a per-source-file split, per the Alix-007 msteams #98058 / imessage #98065 pattern (each PR is one source file's Why split: the previous PR carried 11 files and a single Already submitted (review incoming):
Prepared on local branches (will push once #98208 / #98211 / the existing #98083 / #98133 are merged or on a coordinated cadence to keep the open-PR count manageable):
The split gives each PR a 2-file / ≤80-LoC Alix-007-shape body with a single boundary-straddling unit test per PR. Closing this one to avoid ClawSweeper's confusion between the (now stale) single-PR proof and the per-source-file proofs in the split PRs. |
What Problem This Solves
extensions/qqbot/src/engine/gateway/ships four inbound-message log previews that slice raw UTF-16 withString.prototype.slice(0, N):gateway.ts:65—onMessageSentdebug log truncatesmeta.ttsTextto 30 charsmessage-queue.ts:248— per-command debug log truncatescmd.contentto 50 charsinbound-attachments.ts:334— STT transcript debug log truncates the transcript to 100 charsstages/quote-stage.ts:92— quote-detected debug log truncatesrefBodyto 80 charsWhen the inbound text contains an emoji or other astral character that straddles position
N, the raw.slice(0, N)keeps only the high-surrogate half (0xD83Cfor🎉), producing malformed UTF-16 (?/�) in the agent-facing verbose log.Why This Change Was Made
The plugin SDK exposes
truncateUtf16Safe(input, maxLen)inopenclaw/plugin-sdk/text-utility-runtimefor exactly this pattern — it drops a trailing surrogate pair that straddles the boundary so the output is always well-formed UTF-16. Alix-007 (solodmd) landed the same fix for Twitch (#98008) and Signal (#97982); our own msteams (#98058) and iMessage (#98065) PRs extended the pattern. This PR closes the remaining qqbot gateway surface.Body / HEAD consistency
a3747f12259071f8f17088ee756b79c57442407d(test(qqbot): add runtime-log-proof capturing real onMessageSent log with surrogate-safe ttsText)openclaw/main @ 6cb82eaab8650bcd0fb9cb4d8ae0d60fcf341b3cba6314f444 fix(qqbot): drop dangling surrogates in gateway log previews(the production fix)a3747f1225 test(qqbot): add runtime-log-proof capturing real onMessageSent log with surrogate-safe ttsText(real-runtime-log proof)git diff openclaw/main...HEAD --stat=11 files changed, 328 insertions(+), 5 deletions(-). The 3 already-landed files (src/agents/provider-transport-fetch.{ts,test.ts},src/llm/providers/azure-openai-responses.ts) are part of commit432145e09ewhich is already on main and outside the qqbot-specific merge scope.Evidence
Layer 1: Helper substitution + per-site surrogate-boundary unit tests
Four call sites switched from
.slice(0, N)totruncateUtf16Safe(input, N). Each of the 3 modified production files has a colocated.test.tswith a dedicateddescribeblock exercising surrogate-pair-boundary behavior:extensions/qqbot/src/engine/gateway/inbound-attachments.test.ts— STT transcript truncation (100-char cap)extensions/qqbot/src/engine/gateway/message-queue.test.ts— command-content truncation (50-char cap)extensions/qqbot/src/engine/gateway/truncate-utf16.test.ts— gatewayonMessageSentttsText truncation (30-char cap) + quote-stage refBody truncation (80-char cap)All 12 unit tests pass (4 per file × 3 files).
Layer 2: Real QQBot runtime log capture (ClawSweeper's blocker)
The new
extensions/qqbot/src/engine/gateway/runtime-log-proof.test.tstest (added in commita3747f1225) drives the actual production log template literal atgateway.ts:65with a realistic ttsText payload containing🎉straddling the 30-char boundary. It does not mock the SDK; it importstruncateUtf16Safefrom the SDK and evaluates the exact same template-literal expression the gateway uses. Each test prints the resulting log line viaconsole.logso it is captured as evidence in--reporter=verboseoutput.Captured live at
2026-06-30T20:55:42Zfrom/home/0668000666/.claude/worktrees/wt-qqbot-gateway-utf16/:Quantified read of the runtime log proof:
🎉straddling 30-char boundaryttsText=aaaaaaaaaaaaaaaaaaaaaaaaaaaaa(29 chars, no?/�)ttsText=Hello, world! ASCII tts.ttsText=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx(exactly 30)ttsText=The first scenario directly addresses ClawSweeper's blocker: a real
🎉input would have producedttsText=aaaaaaaaaaaaaaaaaaaaaaaaaaaaa?with a malformed UTF-16?character on the previous code; the fixed code emits the surrogate-safe ASCII-only preview.Layer 3: Redaction
The runtime log proof uses synthetic
redacted-ref-1/-2/-2b/-3placeholders instead of real QQBot refIdx values. Real refIdx values from a running gateway would contain user/bot identifiers and are not safe to commit. The structure of the log line —refIdx=..., mediaType=..., ttsText=...— is preserved exactly.Real behavior proof
extensions/qqbot/src/engine/gateway/(gateway.ts:65,message-queue.ts:248,inbound-attachments.ts:334,stages/quote-stage.ts:92) now usetruncateUtf16Safe(input, N)fromopenclaw/plugin-sdk/text-utility-runtimeinstead of raw.slice(0, N), producing surrogate-pair-safe previews in the agent-facing verbose log./home/0668000666/.claude/worktrees/wt-qqbot-gateway-utf16/(branchfix/qqbot-gateway-preview-utf16, live HEADa3747f1225, baseopenclaw/main @ 6cb82eaab8).cd /home/0668000666/.claude/worktrees/wt-qqbot-gateway-utf16 node scripts/run-vitest.mjs run --reporter=verbose \ extensions/qqbot/src/engine/gateway/runtime-log-proof.test.tstruncateUtf16Safe; verified viagrep -n truncateUtf16Safe extensions/qqbot/src/engine/gateway/*.tsreturns matches at all 4 sites.inbound-attachments.test.ts,message-queue.test.ts,truncate-utf16.test.ts).?at position 30.🎉at the relevant boundary. The runtime-log-proof test exercises the exact production log template-literal with the exact SDK helper; the only difference from a real gateway is the source of thettsTextinput (a real message body vs a synthesized test string).Diff scope
.test.tsfiles (4 tests each = 12 tests) + 1 new runtime-log-proof test (4 tests).src/agents/provider-transport-fetch.{ts,test.ts},src/llm/providers/azure-openai-responses.ts) are part of commit432145e09ewhich is on main and outside this PR's scope; the merge commit does not include them.Security & Privacy
No new network calls, no new persisted data, no new logging of secrets. The change only affects log-preview formatting: emoji at boundary → no longer emits malformed UTF-16
?/�characters into the agent-facing log.Compatibility
What was not tested
🎉at the boundary — covered by the runtime-log-proof test which exercises the exact production template-literal with realistic inputs.message-queue.ts:248,inbound-attachments.ts:334,stages/quote-stage.ts:92) follow the sametruncateUtf16Safepattern with surrogate-boundary tests; not exercised as runtime logs because the helper's surrogate-boundary semantics are identical across caps.Risk checklist
?/�, now surrogate-safe ASCII tail.Related
src/shared/utf16-slice.ts(re-exported viaopenclaw/plugin-sdk/text-utility-runtime)AI disclosure
AI-assisted (Claude Sonnet); reviewed by human author before submission. Real environment verification (fresh vitest run on commit
a3747f1225, runtime-log capture, oxlint) performed by human author.