fix(adapters): reject malformed nested response shapes - #1332
Conversation
📝 WalkthroughWalkthroughThe adapters now validate nested Google candidate and OpenAI Chat tool-call payloads. Malformed structures emit terminal adapter errors instead of causing unsafe processing or runtime exceptions. Tests cover streaming and non-streaming OpenAI responses, Google streaming responses, usage preservation, and completion suppression. ChangesAdapter payload validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
✅ Deterministic PR hygiene checks passed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/adapters/google.ts`:
- Line 605: Update the candidate processing around the parts variable and its
iteration to allow absent content or parts, while validating present parts as an
array containing only non-null objects. Route invalid containers and elements
through the terminal adapter error channel rather than allowing iteration or
property access to throw, and add regression coverage for parts: {} and parts:
[null].
In `@src/adapters/openai-chat.ts`:
- Around line 931-947: Strengthen validation in the streaming tool-call handling
around rawToolCall and flushToolCalls: accept absent partial fields, but reject
present id and function.name/function.arguments values unless they are strings,
and reject function unless it is an object when provided. Before emitting final
tool-call events, require each call to have received string id, name, and
arguments values; route all invalid cases through
terminateWithError(invalidToolCallsEvent(pendingUsage)). Add regressions
covering malformed function, id, name, and arguments payloads.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fa454dca-4429-4bfb-839b-f0eaf6ddcd18
📒 Files selected for processing (4)
src/adapters/google.tssrc/adapters/openai-chat.tstests/google-hardening.test.tstests/openai-chat-hardening.test.ts
| } | ||
|
|
||
| const parts = candidates[0].content?.parts as { text?: string; functionCall?: { name: string; args: unknown } }[] | undefined; | ||
| const parts = candidate.content?.parts as { text?: string; functionCall?: { name: string; args: unknown } }[] | undefined; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate candidate.content.parts before iterating.
The candidate guard does not validate content.parts. A frame with parts: {} throws at Line 615. A frame with parts: [null] throws at Line 616. The outer catch rethrows these errors because they are not translator-budget errors.
Allow absent content or parts frames. If parts is present, reject a non-array container or non-object part through the terminal adapter error channel. Add regression cases for both shapes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/adapters/google.ts` at line 605, Update the candidate processing around
the parts variable and its iteration to allow absent content or parts, while
validating present parts as an array containing only non-null objects. Route
invalid containers and elements through the terminal adapter error channel
rather than allowing iteration or property access to throw, and add regression
coverage for parts: {} and parts: [null].
| const rawToolCalls = delta.tool_calls; | ||
| if (rawToolCalls !== undefined) { | ||
| // A claimed tool-call payload is not benign padding. Dropping it can leave the | ||
| // matching result permanently orphaned, so malformed nested shapes fail closed | ||
| // through the adapter error channel instead of escaping as TypeError (#1325). | ||
| if (!Array.isArray(rawToolCalls)) { | ||
| return yield* terminateWithError(invalidToolCallsEvent(pendingUsage)); | ||
| } | ||
| for (const rawToolCall of rawToolCalls) { | ||
| if (!isRecord(rawToolCall)) { | ||
| return yield* terminateWithError(invalidToolCallsEvent(pendingUsage)); | ||
| } | ||
| const tc = rawToolCall as { | ||
| index?: number; | ||
| id?: string; | ||
| function?: { name?: string; arguments?: string }; | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate nested streaming tool-call fields and final call state.
Lines 943-947 only validate that each entry is an object. A fragment such as { "function": [] } or { "id": 7 } is accepted. The adapter can then emit a tool_call_start with an empty or non-string id or name, followed by done.
Accept partial fragments when fields are absent. If a field is present, require its expected type. Before flushToolCalls emits events, reject calls that never received a string id, name, and arguments value. Add streaming regressions for malformed function, id, name, and arguments.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/adapters/openai-chat.ts` around lines 931 - 947, Strengthen validation in
the streaming tool-call handling around rawToolCall and flushToolCalls: accept
absent partial fields, but reject present id and
function.name/function.arguments values unless they are strings, and reject
function unless it is an object when provided. Before emitting final tool-call
events, require each call to have received string id, name, and arguments
values; route all invalid cases through
terminateWithError(invalidToolCallsEvent(pendingUsage)). Add regressions
covering malformed function, id, name, and arguments payloads.
snowyukitty
left a comment
snowyukitty
left a comment
There was a problem hiding this comment.
The core boundary is right on 8ef72105: this treats a malformed nested candidate/tool-call payload as claimed response data and fails closed, while preserving #1240's benign root-padding skip. That is the important distinction here, and the patch gets it right.
I verified the issue's six reported shapes against this exact head:
- Google streaming
candidates: [null]emits onlygoogle response contained invalid candidates; the laterSTOPframe is not consumed as a successful completion. - OpenAI streaming object-shaped
tool_callsand[null]emit onlyupstream response contained invalid tool calls, preserve usage, and suppress the following[DONE]. - OpenAI non-streaming object-shaped
tool_calls,[null], and a call withoutfunctionreturn the same structured error with usage instead of leaking a JavaScript exception.
The #1240 control also holds: tests/sse-null-data-frame.test.ts still proves that mid-stream root data: null is skipped in both Google and OpenAI Chat, the later content/finish signal and [DONE] complete the turn, and an all-padding stream still fails closed. Local result was 119 passed / 0 failed across that file plus the four focused suites named in the PR. Local typecheck remains the known Windows control failure only (@napi-rs/keyring missing): exit 2 with output line-for-line identical on this head and current dev@a9838c1a. The exact PR head's cross-platform ci check is successful.
I also independently reproduced both current CodeRabbit findings rather than taking them on trust:
content.partscontainer/element validation:parts: {}andparts: [null]still escape as rawTypeErrors.- streaming tool-call field validation: present
function: [], numericid, numericfunction.name, and numericfunction.argumentsare accepted and followed bydone; the latter three also emit non-string values through the string-typed adapter event contract. A temporary fail-closed probe was 0 passed / 6 failed across those two Google and four OpenAI shapes.
One qualification to the second automated recommendation: validating fields that are present is supported by the reproduction, but requiring every assembled streaming call to have received an ID and name would conflict with the existing, passing compatibility contracts in tests/openai-chat-parallel-stream.test.ts (synthesized missing ID, and T7's explicit missing-name parity). I would preserve those absence cases unless changing them is intentional and separately justified.
So, for the six #1325 cases and the #1240 interaction, I found no additional defect; the implementation and focused regressions are sound. I am leaving this as a comment rather than an approval while the two independently verified adjacent findings are unresolved. Please correct me if there is provider evidence that makes the existing missing-ID/name compatibility contract obsolete.
|
Landed on Re-verified against the moving dev head before merge: Thanks — covering all six issue shapes across both streaming and non-streaming paths made this easy to trust. |
3 participants
Summary
data: nullpadding behavior unchanged while failing closed once an upstream claims a candidate or tool call.TypeErrorexceptions from escaping the adapter.Verification
taskset -c 0-1 bun run typecheck— passed on the rebased exact head.taskset -c 0-1 bun test tests/openai-chat-hardening.test.ts tests/google-hardening.test.ts tests/openai-chat-parallel-stream.test.ts tests/google-vertex-stream.test.ts— 83 passed, 0 failed on the rebased exact head.tests/codex-shim.test.ts(Unix shim exports persisted service API token before running Codex); that failure reproduces in isolation and does not touch adapter code.git diff --check origin/dev...HEAD— passed.Decision Log
candidates: [null], object-shapedtool_calls, null tool calls, and missing tool functions were trusted through casts.TypeError; malformed providers now receive a terminal error instead of best-effort continuation.Checklist
Closes #1325
Summary by CodeRabbit