Repository navigation
🧵 fix: Neutralize Harmony Control Tokens for OpenAI Models on Bedrock - #611
danny-avila wants to merge 4 commits into
Conversation
OpenAI models on Bedrock answer a request whose text carries a harmony header such as `<|channel|>analysis` with a 200 event stream that closes before any event. The empty stream left `finalChunk` undefined and, in v4.0.4, crashed tool-call collection with `Cannot use 'in' operator to search for 'tool_calls' in undefined`. Every later turn that still carried the text failed the same way. Swap the bars of harmony control tokens for fullwidth bars in every text, tool input and tool result sent to an identified OpenAI model on Bedrock, reject a Bedrock stream that ends without any event with an explanatory error, and report a missing attempt message as an empty response rather than a TypeError.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de4ee41828
ℹ️ 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".
| converseSystem: neutralizeHarmonyRequest( | ||
| combinedConverseMessages, | ||
| converseSystem | ||
| ), |
There was a problem hiding this comment.
When consumers use the public CustomChatBedrockConverse.invoke() path, this sanitizer is never reached: _generateNonStreaming still delegates directly to super._generateNonStreaming in src/llm/bedrock/index.ts, whose upstream converter does not call this local function. Consequently, an OpenAI Bedrock request containing a Harmony header still sends the raw token and fails in non-streaming mode even though .stream() succeeds; apply the same conversion or sanitization before issuing the non-streaming Converse command.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5661120. _generateNonStreaming now sends identified OpenAI models through this package's converter (generateWithLocalConverter: local convertToConverseMessages, cache points, ConverseCommand), so non-streaming requests get the same neutralization and reasoning-replay suppression as the stream. Other families keep the parent path. Covered by a mocked invoke() test and verified live against global.openai.gpt-6.1-sol.
| export function neutralizeHarmonyRequest( | ||
| messages: BedrockMessage[], | ||
| system: BedrockSystemContentBlock[] | ||
| ): BedrockSystemContentBlock[] { |
There was a problem hiding this comment.
When a bound tool description or JSON-schema description contains a Harmony token—for example, an MCP tool documenting the format—invocationParams() places that string under params.toolConfig, which src/llm/bedrock/index.ts spreads unchanged into ConverseStreamCommand. This function only walks messages and system blocks, so the offending token still reaches the OpenAI prompt and can preserve the same deterministic empty-stream failure; include tool configuration strings in the OpenAI-specific sanitization.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5661120. invocationParams neutralizes tool descriptions and input-schema strings (keys included) for OpenAI models before the tool cache point is inserted; a changed tool is copied, so bound tools are untouched. Tested through attemptInvoke with a bound tool for an OpenAI and a Claude model.
| ? block | ||
| : { ...block, toolResult: { ...block.toolResult, content: next } }; | ||
| } | ||
| return block; |
There was a problem hiding this comment.
When an OpenAI target receives a LangChain standard file block with source_type: 'text', fromStandardFileBlock in message_inputs.ts encodes its text into document.source.bytes, but this function leaves every document block untouched. Thus an attached text file containing a Harmony header still carries the raw token despite this patch's attachment use case and can trigger the same failed response; neutralize text-backed document contents before encoding them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not applicable: OpenAI models on Bedrock reject document blocks outright. Verified live: a document block with format: 'txt' to global.openai.gpt-6.1-sol returns ValidationException: This model doesn't support documents. before any rendering, so neutralizing the bytes would not change the outcome. Text attachments only reach these models as text blocks, which are covered.
| 'g' | ||
| ); | ||
| const ASCII_BAR = /\|/g; | ||
| const FULLWIDTH_BAR = '｜'; |
There was a problem hiding this comment.
The name pattern accepts every alphanumeric or underscore-delimited marker, so ordinary user data such as <|customer_id|> or <|template_slot|> is rewritten even though those strings are not Harmony control tokens. On OpenAI Bedrock models this silently changes prompts, pasted code, tool arguments, and tool results and prevents exact reproduction of otherwise harmless content; match the finite Harmony vocabulary and its reserved-token pattern instead of every identifier-shaped marker.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 5661120. The match is limited to harmony's special-token names (startoftext, endoftext, endofprompt, start, end, message, channel, constrain, return, call, reserved_N), case-sensitive. <|customer_id|>, <|im_start|> and <|CHANNEL|> are now sent as written (asserted in the test).
| for (const key of Object.keys(value)) { | ||
| const entry = value[key]; | ||
| const item = neutralizeDocument(entry); |
There was a problem hiding this comment.
When tool input or a JSON tool result uses a Harmony token as an object key, such as {"<|channel|>": "analysis"}, this recursion only sanitizes the corresponding value and copies the original key unchanged. Because object keys are serialized into the Bedrock prompt too, the raw control token remains in the request and can still produce the empty-stream failure; rebuild changed objects with neutralized keys in addition to values.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5661120. Object keys are neutralized along with values; a changed object is rebuilt lazily from the first changed entry. Covered for tool-result JSON and tool input schemas.
… Tokens Neutralize harmony tokens in tool descriptions and input schemas, and in JSON object keys. Send non-streaming requests to OpenAI models through the local converter, as streaming already does. Match only harmony's special-token names so other `<|name|>` text reaches the model as written.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56611209de
ℹ️ 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".
| } | ||
| entries?.push([nextKey, nextEntry]); | ||
| } | ||
| return entries == null ? value : Object.fromEntries(entries); |
There was a problem hiding this comment.
When a tool argument, tool result, or input schema contains both an ASCII Harmony-shaped key and its already-neutralized fullwidth counterpart (for example, "<|channel|>" and "<｜channel｜>"), both entries are emitted with the same nextKey, so Object.fromEntries silently drops whichever value appears first. This corrupts otherwise valid user/tool data for OpenAI Bedrock requests; use a collision-safe encoding or otherwise preserve a one-to-one mapping between original keys.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bcf2716 by no longer rewriting keys at all (this reverses the key handling added for the earlier key finding). Keys are structure: rewriting them can merge two keys, as here, or rename an argument. A key alone cannot carry the failing header in serialized form either, since the trigger is <|channel|> followed by analysis and a JSON key is always followed by a quote. Only string values are neutralized now; the test asserts a <|channel|> key passes through unchanged.
| ? spec.description | ||
| : neutralizeHarmonyControlTokens(spec.description); | ||
| const schema = spec.inputSchema?.json; | ||
| const json = schema === undefined ? schema : neutralizeDocument(schema); |
There was a problem hiding this comment.
When a tool schema uses a Harmony token in a property name or semantic value such as enum/const, this rewrites the contract sent to the model, so the model generates fullwidth keys or values while the original bound tool still validates and consumes the ASCII form. Because response conversion forwards toolUse.input unchanged, such calls fail validation or arrive with missing arguments even when there is no key collision; retain a reversible mapping and restore generated arguments before dispatch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bcf2716. Input schemas are now treated as a contract: only description and title strings are neutralized, while property names, enum/const values, defaults and patterns are sent as written, so the model's arguments match what the bound tool validates and no reverse mapping is needed. Asserted with a schema carrying a harmony-token property name, enum and default.
| { model: this.model } | ||
| ); | ||
| const params = this.invocationParams(options); | ||
| const modelId = this.getModelId(); |
There was a problem hiding this comment.
For a non-streaming OpenAI model configured with applicationInferenceProfile: '' (for example, from an unset environment variable), this new path calls getModelId() and sends the empty string as modelId. The existing non-OpenAI path explicitly treats an empty profile as absent and falls back to this.model, so this branch introduces an OpenAI-only validation failure; apply the same non-empty check before choosing the profile.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bcf2716. getModelId() now treats an empty applicationInferenceProfile as absent, which fixes the new non-streaming path and the existing streaming path (which also sent the empty string). Covered by a non-streaming test with applicationInferenceProfile = ''.
Rewrite only string values in replayed tool inputs and results, never keys, so no two keys merge. In tool schemas rewrite only `description` and `title`, so names, enum and const values, and defaults stay the contract the bound tool validates. Treat an empty application inference profile as absent when choosing the model id.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bcf27169a5
ℹ️ 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".
| return rewriteDocument(value, neutralizeHarmonyControlTokens); | ||
| } | ||
|
|
||
| const SCHEMA_TEXT_KEYS = new Set(['description', 'title']); |
There was a problem hiding this comment.
The new prose whitelist leaves valid JSON Schema annotations such as $comment untouched. When a bound tool uses $comment: '<|channel|>analysis...' to document Harmony, that raw header is still sent in toolConfig and can reproduce the empty Bedrock response this sanitizer is intended to prevent. This is fresh evidence after the earlier tool-config fix because bcf2716 narrowed sanitization from every schema string to only these two keys; include other non-contract annotations such as $comment while continuing to preserve contract values like enum, const, and default.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f6299f3: $comment joins description and title as schema prose that is neutralized; contract values (enum, const, default, examples, pattern, names) stay as written. Asserted in the tool-config test.
`$comment` carries prose and takes no part in validation, like `description` and `title`.
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
This branch has not been deployed
1 participant
Summary
OpenAI models on Bedrock (
global.openai.gpt-6.1-solhere) fail a turn whenever the request text contains an OpenAI harmony header such as<|channel|>analysis. Bedrock answers200 OKwith a ConverseStream that closes before a single event: nomessageStart, nomessageStop, no exception event.CustomChatBedrockConverseyielded nothing,finalChunkstayed undefined, and v4.0.4 returned{ messages: [undefined] }and crashed collecting tool calls:(reproduced in the demo pod with v4.0.4, real Bedrock, and the exact production message.) The trigger in production was a pasted
text/plainattachment. LibreChat sends those inline as file context, so every later turn resends the text and fails the same way; a fork without the files works. Images carry no text, so they never trigger it, and other Bedrock model families are unaffected.Live characterization against
global.openai.gpt-6.1-sol(us-west-2, synthetic text, 2–3 runs each):<|channel|>analysis<|message|>…<|end|>(with or without<|start|>assistant)<|channel|> analysis,< |channel| >analysis, ZWSP or word-joiner after<<|channel|>final…,<|channel|>commentary…,<|start|>user<|message|>…, single tokenschannel|>analysis,|channel|analysis,<｜channel｜>analysis(fullwidth bars)documentblock (format: 'txt')ValidationException: This model doesn't support documents.Change
convertToConverseMessagesneutralizes harmony special tokens for identified OpenAI targets (isBedrockOpenAIModel, whichreplaysBedrockReasoningnow delegates to): the ASCII bars of each harmony token (start,end,message,channel,constrain,return,call,startoftext,endoftext,endofprompt,reserved_N) become fullwidth bars across system text, user/assistant text,toolUse.input, andtoolResulttext and JSON string values. JSON keys are never rewritten, since they are argument and property names. Other<|name|>text is sent as written. The model still reads the token (asked which channel a neutralized example uses, it answersanalysis). Spaces and zero-width characters inside the token are matched, since Bedrock still renders those. Blocks that change are copied, so LangChain inputs (includingtool_calls[].args) are never mutated. Other model families are untouched.invocationParamsapplies the same neutralization to tool descriptions and to thedescription/titleprose of input schemas for OpenAI targets (property names,enum/constvalues and defaults stay as written, so arguments still match the bound tool), and_generateNonStreamingsends OpenAI targets through this package's converter instead of the parent's, soinvoke()without streaming gets the same treatment (and the existing reasoning-replay suppression).CustomChatBedrockConverserejects a stream that ends without any event (or a response without a stream) withBEDROCK_EMPTY_STREAM_MESSAGEinstead of yielding nothing. A deliberate abort is excluded.getModelId()treats an emptyapplicationInferenceProfileas absent.attemptInvokecollects tool calls throughcollectAttemptToolCalls, which reports a missing message asThe model provider returned an empty response.rather than aTypeError. 📭 fix: Reject Empty Provider Streams #598 already rejects zero-chunk streams earlier; this keeps the crash site safe for any other path.Verification
attemptInvoke+CustomChatBedrockConversewith real Bedrock credentials: the attached-file request that returns zero events unpatched completed 3/3 streaming, and once through non-streaminginvoke().src/llm/bedrock/utils/harmony.test.ts(token rewrite and vocabulary limits, tool config, model identification, full request conversion for system/user/assistant/tool input/tool result text and JSON, input immutability, non-OpenAI passthrough) andsrc/llm/bedrock/openaiStream.test.ts(recorded OpenAI-on-Bedrock tool-call event stream throughattemptInvoke, bound tool descriptions for OpenAI vs Claude, non-streaminginvoke()and its empty-profile fallback, zero-event and stream-less responses,collectAttemptToolCallswith[undefined]).npx jest src/llm/bedrock src/llm/invoke: 20 suites, 258 tests pass. Acrosssrc/llm, the only failures are live Google/Anthropic/Vertex specs that need provider credentials.npx tsc --noEmit, ESLint on touched files,npm run check:circular-deps, andnpm run buildpass.