fix: strip internal reasoning placeholder from user-visible content (#8081) - #8162
Conversation
…iegosouzapw#8081) The internal reasoning replay sentinel '(prior reasoning summary unavailable)' can leak into user-visible assistant content when a model echoes it through ordinary message.content / delta.content. Existing suppression only checked reasoning_content fields and reasoning-specific events. Changes: - Add stripInternalReasoningPlaceholder() to reasoningPlaceholder.ts — removes all occurrences of the sentinel and trims; returns '' when nothing meaningful remains - Streaming: strip in responsesTransformer.ts, openai-responses.ts, and openai-to-claude.ts at the delta.content entry point; skip emission entirely when only the placeholder was present - Non-streaming: strip in responseSanitizer.ts sanitizeMessageContent() and sanitizeResponsesMessageContent() (all three text paths) translateText is unaffected (uses mode='translate' via plain newsClient). The per-provider reasoning_content check remains as defense-in-depth.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9dd2bfd1a8
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * meaningful remains so callers can skip emission entirely. | ||
| */ | ||
| export function stripInternalReasoningPlaceholder(value: string): string { | ||
| return value.replaceAll(NON_ANTHROPIC_THINKING_PLACEHOLDER, "").trim(); |
There was a problem hiding this comment.
Preserve streamed token whitespace when stripping sentinel
Because stripInternalReasoningPlaceholder() is now called for every delta.content chunk in the Responses and Claude translators, the unconditional .trim() changes normal content even when the sentinel is absent. Streaming chunks commonly begin with a space (for example " world" after "Hello"), so this turns user-visible text into "Helloworld"; only use trimming to decide whether the placeholder-only result is empty, not for the returned content.
Useful? React with 👍 / 👎.
| // through ordinary content (#8081). | ||
| if (delta.content) { | ||
| const strippedContent = stripInternalReasoningPlaceholder(delta.content); | ||
| if (!strippedContent) return; |
There was a problem hiding this comment.
Keep processing after placeholder-only SSE events
When an upstream emits a placeholder-only delta.content, this exits the entire transform() call, not just the text-emission branch. Since a single network chunk can contain multiple complete SSE messages, a placeholder event at the front of the buffer causes later visible deltas, tool calls, or the finish event in the same buffer to be dropped; suppress this event’s text while continuing to process the remaining messages and handlers.
Useful? React with 👍 / 👎.
…ish_reason/tool_calls (diegosouzapw#8081) Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
07dada6
into
diegosouzapw:release/v3.8.49
|
Obrigado, @Dingding-leo — mergeado na |
…s + eslint baseline The release branch accumulated deterministic unit-test failures (fast-path red on every open PR). These are the ones with a clear, surgical root cause: 1. diegosouzapw#6863 combo model-lockout — the diegosouzapw#7940/diegosouzapw#7980 "cap exactCooldownMs against maxCooldownMs" clamp was also clamping an AUTHORITATIVE parsed upstream quota reset (e.g. "Resets in 92h27m28s") down to maxCooldownMs, so an exhausted model was retried far too early. recordModelLockoutFailure now takes exactCooldownIsUpstreamReset — set by the combo callers when the exact cooldown is a real upstream reset — which exempts it from the cap. The diegosouzapw#7980 computed until-midnight cap is unchanged (flag absent → still capped). 2. diegosouzapw#5786 streaming claude←codex — stripInternalReasoningPlaceholder (diegosouzapw#8081/diegosouzapw#8162) unconditionally .trim()'d every value. On the per-delta streaming path this ate the meaningful edge spaces of each delta ("Hello, " + "world." + " Bye." glued to "Hello,world.Bye."). It now only collapses to "" when whitespace is all that remains after removing the placeholder, preserving real content verbatim. 3. SPAWN_CAPABLE_PREFIXES test — diegosouzapw#7892 added /api/vnc-session (11th spawn-capable prefix, spawns Docker) but the client-safe guard test still expected 10 and did not list it. Aligned to 11 + added the entry to the checklist. 4. ESLint baseline — diegosouzapw#8008/diegosouzapw#8062 merged new test files with no-explicit-any without refreshing the frozen suppressions, so "No new ESLint warnings" went red for the whole branch. Regenerated the two affected entries (combo-routing-engine.test.ts 269→271, oauth-refresh-connection-dedup-8059.test.ts +1). Validated: the three failing tests now pass; the sibling guards they interact with stay green (diegosouzapw#7980 exact-cooldown-cap 4/4, diegosouzapw#8162 placeholder suites 17+12+41, account-fallback 77); typecheck:core clean; lint:json --max-warnings 0 exits 0. NOTE: the release branch has ~20 further real base-red failures (compression-engine catalog, handleChat fallback, provider candidate transparency, i18n, misc). Those are tracked separately, one focused PR per root-cause cluster; this PR is the first slice.
…s + eslint baseline The release branch accumulated deterministic unit-test failures (fast-path red on every open PR). These are the ones with a clear, surgical root cause: 1. diegosouzapw#6863 combo model-lockout — the diegosouzapw#7940/diegosouzapw#7980 "cap exactCooldownMs against maxCooldownMs" clamp was also clamping an AUTHORITATIVE parsed upstream quota reset (e.g. "Resets in 92h27m28s") down to maxCooldownMs, so an exhausted model was retried far too early. recordModelLockoutFailure now takes exactCooldownIsUpstreamReset — set by the combo callers when the exact cooldown is a real upstream reset — which exempts it from the cap. The diegosouzapw#7980 computed until-midnight cap is unchanged (flag absent → still capped). 2. diegosouzapw#5786 streaming claude←codex — stripInternalReasoningPlaceholder (diegosouzapw#8081/diegosouzapw#8162) unconditionally .trim()'d every value. On the per-delta streaming path this ate the meaningful edge spaces of each delta ("Hello, " + "world." + " Bye." glued to "Hello,world.Bye."). It now only collapses to "" when whitespace is all that remains after removing the placeholder, preserving real content verbatim. 3. SPAWN_CAPABLE_PREFIXES test — diegosouzapw#7892 added /api/vnc-session (11th spawn-capable prefix, spawns Docker) but the client-safe guard test still expected 10 and did not list it. Aligned to 11 + added the entry to the checklist. 4. ESLint baseline — diegosouzapw#8008/diegosouzapw#8062 merged new test files with no-explicit-any without refreshing the frozen suppressions, so "No new ESLint warnings" went red for the whole branch. Regenerated the two affected entries (combo-routing-engine.test.ts 269→271, oauth-refresh-connection-dedup-8059.test.ts +1). Validated: the three failing tests now pass; the sibling guards they interact with stay green (diegosouzapw#7980 exact-cooldown-cap 4/4, diegosouzapw#8162 placeholder suites 17+12+41, account-fallback 77); typecheck:core clean; lint:json --max-warnings 0 exits 0. NOTE: the release branch has ~20 further real base-red failures (compression-engine catalog, handleChat fallback, provider candidate transparency, i18n, misc). Those are tracked separately, one focused PR per root-cause cluster; this PR is the first slice.
… sentinel (#8382) Regression: #8162 (port of #8081) added an unconditional `.trim()` to stripInternalReasoningPlaceholder(), applied to every streaming delta.content chunk across 3 call-sites (openai-to-claude.ts, openai-responses.ts, responsesTransformer.ts). Leading/trailing whitespace at a chunk boundary is a real word boundary between streaming fragments; trimming it glues adjacent chunks together on the client ("Hello, " + "world." + " Bye." -> "Hello,world.Bye."). Fix: early-return via .includes() before the replaceAll+trim, so the function is a true no-op when the sentinel is absent from the chunk. Behavior when the sentinel IS present is unchanged. Validation: - tests/unit/streaming-reasoning-dedup-5786.test.ts: the "(A-guard)" test was RED on the base branch ('Hello,world.Bye.' vs 'Hello, world. Bye.'); GREEN after the fix (4/4 passing). - tests/unit/translator-resp-openai-to-claude.test.ts: added a new multi-chunk boundary-whitespace regression test, proven RED against the pre-fix code (12/13), GREEN after (13/13). - No regressions in responses-transformer.test.ts (17/17), responses-transformer-dense-output.test.ts (3/3), or the other suites exercising the shared placeholder utility (160/160 total across all consumers). Refs #8162 Refs #8081
…rd spaces (#8341) Live incident: streamed assistant text was losing the spaces BETWEEN words (e.g. "Bilden är en riktig JPEG nu" -> "Bildenärenriktig JPEG nu") on the Responses-API and Claude streaming paths. stripInternalReasoningPlaceholder() (#8081/#8162) is called on every individual delta.content chunk, and unconditionally called .trim() even when its sentinel ("(prior reasoning summary unavailable)") was never present in that chunk. Tokenizers commonly emit sub-word tokens with a leading space as part of the token (e.g. " en", " riktig") -- each such chunk got its only whitespace character (the inter-word space) silently trimmed away before being appended to the accumulated message, while the words themselves stayed intact. Punctuation-only chunks were largely unaffected, matching what was observed live. Only trims when the sentinel is actually present -- preserves the original #8081 intent (collapse a placeholder-only chunk to "") without touching the overwhelming majority of chunks that never contain it. Co-authored-by: Markus Hartung <markus.hartream@gmail.com>
…iegosouzapw#8081) (diegosouzapw#8162) * fix: strip internal reasoning placeholder from user-visible content (diegosouzapw#8081) The internal reasoning replay sentinel '(prior reasoning summary unavailable)' can leak into user-visible assistant content when a model echoes it through ordinary message.content / delta.content. Existing suppression only checked reasoning_content fields and reasoning-specific events. Changes: - Add stripInternalReasoningPlaceholder() to reasoningPlaceholder.ts — removes all occurrences of the sentinel and trims; returns '' when nothing meaningful remains - Streaming: strip in responsesTransformer.ts, openai-responses.ts, and openai-to-claude.ts at the delta.content entry point; skip emission entirely when only the placeholder was present - Non-streaming: strip in responseSanitizer.ts sanitizeMessageContent() and sanitizeResponsesMessageContent() (all three text paths) translateText is unaffected (uses mode='translate' via plain newsClient). The per-provider reasoning_content check remains as defense-in-depth. * fix: skip only empty content block on reasoning-placeholder, keep finish_reason/tool_calls (diegosouzapw#8081) Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> * chore(quality): rebaseline openai-responses.ts own-growth (diegosouzapw#8081 guard) --------- Co-authored-by: Austin Liu <austinliu@Austins-MacBook-Air-3.local> Co-authored-by: Probe Test <probe@example.com> Co-authored-by: Dingding-leo <Dingding-leo@users.noreply.github.com> Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
… sentinel (diegosouzapw#8382) Regression: diegosouzapw#8162 (port of diegosouzapw#8081) added an unconditional `.trim()` to stripInternalReasoningPlaceholder(), applied to every streaming delta.content chunk across 3 call-sites (openai-to-claude.ts, openai-responses.ts, responsesTransformer.ts). Leading/trailing whitespace at a chunk boundary is a real word boundary between streaming fragments; trimming it glues adjacent chunks together on the client ("Hello, " + "world." + " Bye." -> "Hello,world.Bye."). Fix: early-return via .includes() before the replaceAll+trim, so the function is a true no-op when the sentinel is absent from the chunk. Behavior when the sentinel IS present is unchanged. Validation: - tests/unit/streaming-reasoning-dedup-5786.test.ts: the "(A-guard)" test was RED on the base branch ('Hello,world.Bye.' vs 'Hello, world. Bye.'); GREEN after the fix (4/4 passing). - tests/unit/translator-resp-openai-to-claude.test.ts: added a new multi-chunk boundary-whitespace regression test, proven RED against the pre-fix code (12/13), GREEN after (13/13). - No regressions in responses-transformer.test.ts (17/17), responses-transformer-dense-output.test.ts (3/3), or the other suites exercising the shared placeholder utility (160/160 total across all consumers). Refs diegosouzapw#8162 Refs diegosouzapw#8081
…rd spaces (diegosouzapw#8341) Live incident: streamed assistant text was losing the spaces BETWEEN words (e.g. "Bilden är en riktig JPEG nu" -> "Bildenärenriktig JPEG nu") on the Responses-API and Claude streaming paths. stripInternalReasoningPlaceholder() (diegosouzapw#8081/diegosouzapw#8162) is called on every individual delta.content chunk, and unconditionally called .trim() even when its sentinel ("(prior reasoning summary unavailable)") was never present in that chunk. Tokenizers commonly emit sub-word tokens with a leading space as part of the token (e.g. " en", " riktig") -- each such chunk got its only whitespace character (the inter-word space) silently trimmed away before being appended to the accumulated message, while the words themselves stayed intact. Punctuation-only chunks were largely unaffected, matching what was observed live. Only trims when the sentinel is actually present -- preserves the original diegosouzapw#8081 intent (collapse a placeholder-only chunk to "") without touching the overwhelming majority of chunks that never contain it. Co-authored-by: Markus Hartung <markus.hartream@gmail.com>
…iegosouzapw#8081) (diegosouzapw#8162) * fix: strip internal reasoning placeholder from user-visible content (diegosouzapw#8081) The internal reasoning replay sentinel '(prior reasoning summary unavailable)' can leak into user-visible assistant content when a model echoes it through ordinary message.content / delta.content. Existing suppression only checked reasoning_content fields and reasoning-specific events. Changes: - Add stripInternalReasoningPlaceholder() to reasoningPlaceholder.ts — removes all occurrences of the sentinel and trims; returns '' when nothing meaningful remains - Streaming: strip in responsesTransformer.ts, openai-responses.ts, and openai-to-claude.ts at the delta.content entry point; skip emission entirely when only the placeholder was present - Non-streaming: strip in responseSanitizer.ts sanitizeMessageContent() and sanitizeResponsesMessageContent() (all three text paths) translateText is unaffected (uses mode='translate' via plain newsClient). The per-provider reasoning_content check remains as defense-in-depth. * fix: skip only empty content block on reasoning-placeholder, keep finish_reason/tool_calls (diegosouzapw#8081) Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> * chore(quality): rebaseline openai-responses.ts own-growth (diegosouzapw#8081 guard) --------- Co-authored-by: Austin Liu <austinliu@Austins-MacBook-Air-3.local> Co-authored-by: Probe Test <probe@example.com> Co-authored-by: Dingding-leo <Dingding-leo@users.noreply.github.com> Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
… sentinel (diegosouzapw#8382) Regression: diegosouzapw#8162 (port of diegosouzapw#8081) added an unconditional `.trim()` to stripInternalReasoningPlaceholder(), applied to every streaming delta.content chunk across 3 call-sites (openai-to-claude.ts, openai-responses.ts, responsesTransformer.ts). Leading/trailing whitespace at a chunk boundary is a real word boundary between streaming fragments; trimming it glues adjacent chunks together on the client ("Hello, " + "world." + " Bye." -> "Hello,world.Bye."). Fix: early-return via .includes() before the replaceAll+trim, so the function is a true no-op when the sentinel is absent from the chunk. Behavior when the sentinel IS present is unchanged. Validation: - tests/unit/streaming-reasoning-dedup-5786.test.ts: the "(A-guard)" test was RED on the base branch ('Hello,world.Bye.' vs 'Hello, world. Bye.'); GREEN after the fix (4/4 passing). - tests/unit/translator-resp-openai-to-claude.test.ts: added a new multi-chunk boundary-whitespace regression test, proven RED against the pre-fix code (12/13), GREEN after (13/13). - No regressions in responses-transformer.test.ts (17/17), responses-transformer-dense-output.test.ts (3/3), or the other suites exercising the shared placeholder utility (160/160 total across all consumers). Refs diegosouzapw#8162 Refs diegosouzapw#8081
…rd spaces (diegosouzapw#8341) Live incident: streamed assistant text was losing the spaces BETWEEN words (e.g. "Bilden är en riktig JPEG nu" -> "Bildenärenriktig JPEG nu") on the Responses-API and Claude streaming paths. stripInternalReasoningPlaceholder() (diegosouzapw#8081/diegosouzapw#8162) is called on every individual delta.content chunk, and unconditionally called .trim() even when its sentinel ("(prior reasoning summary unavailable)") was never present in that chunk. Tokenizers commonly emit sub-word tokens with a leading space as part of the token (e.g. " en", " riktig") -- each such chunk got its only whitespace character (the inter-word space) silently trimmed away before being appended to the accumulated message, while the words themselves stayed intact. Punctuation-only chunks were largely unaffected, matching what was observed live. Only trims when the sentinel is actually present -- preserves the original diegosouzapw#8081 intent (collapse a placeholder-only chunk to "") without touching the overwhelming majority of chunks that never contain it. Co-authored-by: Markus Hartung <markus.hartream@gmail.com>
Summary
Fixes #8081. The internal reasoning replay sentinel
(prior reasoning summary unavailable)can leak into user-visible assistant content when a model echoes it through ordinarymessage.content/delta.content. Existing suppression only checkedreasoning_contentfields and reasoning-specific events.Changes
open-sse/utils/reasoningPlaceholder.ts: AddstripInternalReasoningPlaceholder()— removes all occurrences of the sentinel and trims; returns""when nothing meaningful remainsdelta.contententry point inresponsesTransformer.ts,openai-responses.ts, andopenai-to-claude.ts; skip emission entirely when only the placeholder was presentresponseSanitizer.ts): Strip insanitizeMessageContent()andsanitizeResponsesMessageContent()(all three text paths)translateTextis unaffected (usesmode='translate'via the plainnewsClient). The per-providerreasoning_contentcheck remains as defense-in-depth.Verification
npx tsc --pretty false -p tsconfig.typecheck-core.json— zero errorsreasoning-cache.test.ts— 52/52 passtranslator-resp-openai-to-claude.test.ts— 11/11 passtool-request-sanitization.test.ts— 8/8 passresponse-sanitizer.test.ts+responses-transformer.test.ts+translator-resp-openai-responses.test.ts— 83/83 pass5 files changed, +33 −11