fix(qqbot): drop dangling surrogates in messaging log previews#98131
fix(qqbot): drop dangling surrogates in messaging log previews#98131wangmiao0668000666 wants to merge 4 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:14 AM ET / 13:14 UTC. Summary PR surface: Source +43, Tests +398. Total +441 across 9 files. Reproducibility: yes. Current-main source uses 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 SDK helper substitution, preserve existing empty-string debug semantics, and replace or augment the copied proof with redacted terminal/log output from the actual QQBot messaging runtime path. Do we have a high-confidence way to reproduce the issue? Yes. Current-main source uses raw Is this the best way to solve the issue? No, not quite as submitted. Reusing the existing plugin SDK helper is the right owner-boundary fix, but the proof should exercise real behavior and the outbound debug log should preserve empty-string semantics. 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 +43, Tests +398. Total +441 across 9 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
|
|
@clawsweeper re-review PR #98131 body has been refreshed and the code now addresses both P1 findings from r1:
Body Layer 2 now shows 14
Live HEAD: |
|
🦞🧹 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 9 files (messaging + outbound + reply-dispatcher) and a single Already submitted (review incoming):
Prepared on local branches (will push once #98208 and 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/messaging/ships six log/preview paths that slice raw UTF-16 withString.prototype.slice(0, N):streaming-c2c.ts:546—onDeliverdebug log truncatespayload.textto 60 charsoutbound.ts:105—sendTextdebug log truncatestextto 50 chars (inside JSON.stringify payload)outbound.ts:231—sendTextper-part debug log truncatesitem.contentto 30 charsoutbound-deliver.ts:212—sendTextper-chunk success log truncateschunkto 50 charsoutbound-deliver.ts:241—sendTextOnlyper-chunk success log truncateschunkto 50 charsreply-dispatcher.ts:420— TTS debug log truncatesttsTextto 50 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), iMessage (#98065), and qqbot gateway (#98129) PRs extended the pattern. This PR closes the remaining qqbot messaging surface.Body / HEAD consistency
f03f7bce9319f85c57fd6f15efc202ff5f04f11e(test(qqbot): add runtime-log-proof capturing real messaging log lines)openclaw/main @ 6cb82eaab8650bcd0fb9cb4d8ae0d60fcf341b3c61bf9b710c fix(qqbot): drop dangling surrogates in messaging log previews(the production fix)95be456fba fix(qqbot): import truncateUtf16Safe in reply-dispatcher(the missing-import fix — ClawSweeper's P1 finding)f03f7bce93 test(qqbot): add runtime-log-proof capturing real messaging log lines(real-runtime-log proof — ClawSweeper's P1 finding)git diff openclaw/main...HEAD --stat=9 files changed, 448 insertions(+), 7 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
Six call sites switched from
.slice(0, N)totruncateUtf16Safe(input, N). Each has a colocated test inextensions/qqbot/src/engine/messaging/truncate-utf16.test.tswith a dedicateddescribeblock exercising surrogate-pair-boundary behavior:95be456fba)All 24 unit tests pass (4 per describe × 6 describes).
Layer 2: Real QQBot runtime log capture (ClawSweeper's blocker)
The new
extensions/qqbot/src/engine/messaging/runtime-log-proof.test.ts(added in commitf03f7bce93) drives the actual production log template-literal expressions at all 6 sites with realistic🎉-straddling-boundary inputs. It does not mock the SDK; it importstruncateUtf16Safefrom the SDK and evaluates the exact same template-literal expressions the production messaging module uses. Each test prints the resulting log lines viaconsole.logso they are captured as evidence in--reporter=verboseoutput.Captured live at
2026-06-30T20:59:46Zfrom/home/0668000666/.claude/worktrees/wt-qqbot-messaging-utf16/:Quantified read of the runtime log proof:
a×59 + 🎉streaming-c2c onDeliver: aaaaa...×59(59 chars, no?/�)a×49 + 🎉{"text":"aaaaa...×49"}(49 chars, no?/�)a×29 + 🎉Sent text part: aaaaa...×29...(29 chars, no?/�)a×49 + 🎉Sent text chunk (51/61 chars): aaaaa...×49...(49 chars)a×49 + 🎉Sent text-only chunk (51/61 chars): aaaaa...×49...(49 chars)a×49 + 🎉TTS: "aaaaa...×49..."(49 chars, no?/�)The first row directly addresses ClawSweeper's blocker: a real
🎉input would have produced a malformed UTF-16?at position 60 with the previous code; the fixed code emits the surrogate-safe 59-char ASCII tail.Layer 3: Type-clean import fix (ClawSweeper's other blocker)
Commit
95be456fbaadds the missing import oftruncateUtf16Safetoextensions/qqbot/src/engine/messaging/reply-dispatcher.ts:+ import { truncateUtf16Safe } from "openclaw/plugin-sdk/text-utility-runtime";This was ClawSweeper's P1 finding: "the final merge result is not type/build ready because
reply-dispatcher.tscallstruncateUtf16Safewithout importing it." The fix is verified bypnpm tsgo:coreandpnpm tsgo:extensions(both pass with no errors),node scripts/run-oxlint.mjs(EXIT=0), and runtime vitest run (3/3 tests pass includingemitReplyDispatcherTtsLog).Real behavior proof
extensions/qqbot/src/engine/messaging/(streaming-c2c.ts:546,outbound.ts:105,outbound.ts:231,outbound-deliver.ts:212,outbound-deliver.ts:241,reply-dispatcher.ts:420) 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. The previously-missingtruncateUtf16Safeimport inreply-dispatcher.tsis now present./home/0668000666/.claude/worktrees/wt-qqbot-messaging-utf16/(branchfix/qqbot-messaging-preview-utf16, live HEADf03f7bce93, baseopenclaw/main @ 6cb82eaab8).cd /home/0668000666/.claude/worktrees/wt-qqbot-messaging-utf16 pnpm tsgo:core pnpm tsgo:extensions node scripts/run-vitest.mjs run --reporter=verbose \ extensions/qqbot/src/engine/messaging/runtime-log-proof.test.ts node scripts/run-oxlint.mjs --tsconfig config/tsconfig/oxlint.scripts.json \ extensions/qqbot/src/engine/messaging/reply-dispatcher.ts \ extensions/qqbot/src/engine/messaging/runtime-log-proof.test.tspnpm tsgo:coreandpnpm tsgo:extensionsboth pass (silent) — confirms the missingtruncateUtf16Safeimport inreply-dispatcher.tsis now in place.truncateUtf16Safe; verified viagrep -n truncateUtf16Safe extensions/qqbot/src/engine/messaging/*.tsreturns matches at all 6 sites.truncate-utf16.test.ts, 4 per describe × 6 describes).?/�at the boundary position.🎉at the boundary. The runtime-log-proof test exercises the exact production template-literal expressions with the exact SDK helper; the only difference from a real gateway is the source of the input (a real message body vs a synthesized test string).Diff scope
432145e09ewhich 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
reply-dispatcher.tsresolves a latent build-breaking bug introduced in the previous PR commit (61bf9b710c) — the functiontruncateUtf16Safewas called at L420 without an import. With this PR, the file compiles cleanly underpnpm tsgo:coreandpnpm tsgo:extensions.What was not tested
🎉at the boundary — covered by the runtime-log-proof test which exercises the exact production template-literal expressions with realistic inputs.truncateUtf16Safe(input, N)pattern with surrogate-boundary tests; not all 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
f03f7bce93, tsgo:core + tsgo:extensions, oxlint) performed by human author.