fix(responses): validate message content blocks before use (#435)#436
fix(responses): validate message content blocks before use (#435)#436snowyukitty wants to merge 2 commits into
Conversation
A message item whose content blocks fail their strict schema falls through to inputItemSchema's permissive catch-all, which passes the raw content through untouched. inputContentParts and outputTextOf then cast without checking, so a malformed block could crash parseRequest itself (system role, null block), produce undefined or object message content that throws in the provider adapter, or emit a text part with no text field. Validate each block and field before use, matching the pattern outputToToolResultContent in the same file already applies.
📝 WalkthroughWalkthroughThe response parser now validates malformed content blocks before reading fields, safely handles invalid images and files, and updates call sites to pass raw content. New tests cover user, system, assistant, Google, and Anthropic parsing paths. ChangesMalformed content handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/responses/parser.ts`:
- Line 38: Guard the content container before iteration: update
inputContentParts and outputTextOf to accept unknown values and return [] unless
the value is a string or an array, adding an Array.isArray(blocks) check before
each loop. In tests/responses-parser-malformed-content.test.ts lines 33-43, add
regressions for malformed user, system, and assistant content such as content:
{}, asserting parsing does not throw.
🪄 Autofix (Beta)
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: c6a6a6f2-fe7d-48f9-bca3-20b1eccb9cb4
📒 Files selected for processing (2)
src/responses/parser.tstests/responses-parser-malformed-content.test.ts
![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: d3f090494f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // A malformed message item fails its strict schema and falls through to inputItemSchema's | ||
| // permissive catch-all, so blocks reaching here are NOT guaranteed to match the declared | ||
| // shape. Validate each field before use, as outputToToolResultContent already does. | ||
| if (!isObj(raw)) continue; |
There was a problem hiding this comment.
When the permissive catch-all receives a message whose content is a plain object or scalar—for example, {type:"message", role:"user", content:{type:"input_text", text:"x"}}—that outer value also passes through unvalidated. The for (const raw of blocks) loop then throws before this new per-block guard executes, so malformed user, system, or assistant content can still cause a server exception. Check Array.isArray(blocks) before iterating, apply the same guard in outputTextOf, and add focused object/scalar regression cases.
AGENTS.md reference: AGENTS.md:L75-L77
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in acfe5c1 — thank you, this was a real gap in the fix.
I had guarded each block but not the container. Verified against the previous commit:
user content = object -> THREW: {} is not iterable
user content = number -> THREW: number is not iterable
user content = true -> THREW: true is not iterable
system content = object -> THREW: {} is not iterable
assistant content = object -> THREW: {} is not iterable
Both readers now take unknown and return [] unless the value is a string or an array, so the container is validated before the loop. That also let me drop the now-redundant as unknown[] | string | undefined casts at all four call sites.
Added two cases covering object/number/boolean containers across the user, system, and assistant roles; both fail against the previous commit. Suite is now 12 passed, 22 parser-adjacent suites 310 passed, typecheck and privacy scan green.
The catch-all can also retain a non-array `content` (an object, a number, a boolean). Both readers threw at `for (const raw of blocks)` before any per-block guard ran, so a malformed container still crashed parseRequest. Accept unknown and return [] unless the value is a string or an array, which also removes the now-redundant call-site casts. Addresses CodeRabbit review feedback on lidge-jun#436.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/responses/parser.ts (1)
44-56: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDrop reference-less malformed image/file blocks.
When
image_urlis invalid and no non-empty stringfile_idexists, this emits[image: ?];{ type: "input_file" }emits[file: ?]. Those fabricated parts can be forwarded to providers as user-visible content, contrary to the PR objective to drop malformed blocks. Emit a marker only when a non-empty string reference exists; otherwise skip the block. Add regressions for both cases intests/responses-parser-malformed-content.test.ts.Proposed fix
} else if (block.type === "input_image") { const b = block as { image_url?: string; file_id?: string; detail?: string }; if (typeof b.image_url === "string" && b.image_url.length > 0) { parts.push({ type: "image", imageUrl: b.image_url, ...(typeof b.detail === "string" && b.detail.length > 0 ? { detail: normalizeImageDetail(b.detail) } : {}) }); - } else { - parts.push({ type: "text", text: `[image: ${typeof b.file_id === "string" ? b.file_id : "?"}]` }); + } else if (typeof b.file_id === "string" && b.file_id.length > 0) { + parts.push({ type: "text", text: `[image: ${b.file_id}]` }); } } else if (block.type === "input_file") { const b = block as { file_id?: string; filename?: string }; - const ref = typeof b.file_id === "string" ? b.file_id : typeof b.filename === "string" ? b.filename : "?"; - parts.push({ type: "text", text: `[file: ${ref}]` }); + const ref = typeof b.file_id === "string" && b.file_id.length > 0 + ? b.file_id + : typeof b.filename === "string" && b.filename.length > 0 + ? b.filename + : undefined; + if (ref !== undefined) parts.push({ type: "text", text: `[file: ${ref}]` }); }🤖 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/responses/parser.ts` around lines 44 - 56, Update the input_image and input_file branches in the response parser to emit fallback text markers only when a non-empty string file_id or filename reference exists; otherwise skip the malformed block without adding a part. Preserve structured image handling for valid non-empty image_url values, and add regressions covering reference-less malformed images and files in tests/responses-parser-malformed-content.test.ts.
🤖 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.
Outside diff comments:
In `@src/responses/parser.ts`:
- Around line 44-56: Update the input_image and input_file branches in the
response parser to emit fallback text markers only when a non-empty string
file_id or filename reference exists; otherwise skip the malformed block without
adding a part. Preserve structured image handling for valid non-empty image_url
values, and add regressions covering reference-less malformed images and files
in tests/responses-parser-malformed-content.test.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 85fb0661-2587-40dd-be98-ef8b855370f5
📒 Files selected for processing (2)
src/responses/parser.tstests/responses-parser-malformed-content.test.ts
🔒 Maintainer integration in progress — please hold off on merging@lidge-jun (maintainer) is actively integrating this PR into What is happening This PR is being integrated on the maintainer branch What this means for you
This PR will be closed with a merge receipt (integration commit SHA, verification output, Integration tracker: |
PR #436 (head acfe5c1, author snowyukitty)을 통합하고, 유효한 참조가 없는 malformed image/file 블록이 가짜 첨부 마커가 되던 결함을 함께 고친다. inputItemSchema의 catch-all 때문에 여기 도달하는 블록은 선언된 shape를 보장하지 않는다. 무검증 cast는 content가 [null]이면 block.type 접근에서 throw했다. 추가 repair: 기존 fallback은 참조가 없을 때 '[image: ?]' / '[file: ?]'를 만들어 모델에게 실제 첨부가 있었다고 믿게 했다. 이제 참조 우선순위는 image가 image_url > file_id > 생략, file이 file_id > file_data(+filename) > 생략이다. 빈 문자열과 non-string은 참조로 보지 않고, filename 단독은 파일 resource가 아니며, file_data 바이트는 어떤 경로에서도 사용자 content로 새지 않는다. Co-authored-by: snowyukitty <snowyukitty@users.noreply.github.com>
✅ Integrated into
|
|
Closing: integrated into |
2 participants
Closes #435.
Problem
The strict schemas do require well-formed content blocks —
inputTextSchemais{ type: "input_text", text: z.string() }. ButinputItemSchemaends with a permissive catch-all:A message item whose content blocks fail their strict schema is therefore not rejected — it falls
through to that catch-all, which passes the raw
contentthrough untouched.inputContentParts()and
outputTextOf()then cast it without checking:Five outcomes, all confirmed against
devat6f4cd1d6:user+{"type":"input_text"}(notext)contentbecomesundefined→ adapter throwsTypeErrorsystem+{"type":"input_text"}parseRequestthrowsundefined is not an object (evaluating 'text.map')user+[null]parseRequestthrowsnull is not an object (evaluating 'block.type')user+{"type":"input_text","text":{"a":1}}contentbecomes the object{"a":1}— neither string nor arrayassistant+{"type":"output_text"}(notext){"type":"text"}part with notextThe
undefinedcontent comes from the single-part collapse, which returns the unvalidated fielddirectly:
That last table row is the same malformed shape behind the Anthropic 400 in #420
(
content.N.text.text: Field required), reached from a different direction.outputToToolResultContent()in this same file already validates correctly —if (!isObj(raw)) continue;,if (typeof raw.text === "string"). This PR brings the other two readers in line with it.Fix
src/responses/parser.ts, ininputContentParts()andoutputTextOf():isObj(raw)guard, exactly asoutputToToolResultContentdoes.text/refusalbefore emitting a part, so a malformed block is droppedinstead of becoming
{"type":"text"}with notext.image_urlbefore treating a block as an image. A non-stringpreviously reached
parseDataUrl(), which calls.match()on it; it now falls back to theexisting
[image: <file_id>]marker.input_file/file_idrefs only from strings, so a non-string cannot be interpolatedinto the marker.
With every emitted part well-formed, the single-part collapse can no longer return
undefined, andmessage content is always a string or an array — which is what every adapter already assumes.
Malformed blocks are dropped rather than coerced: coercing an object to text yields
"[object Object]", which is worse than omitting it, and dropping matches the precedent inoutputToToolResultContent.Tests
tests/responses-parser-malformed-content.test.ts(new, 10 cases): one per table row, plusvalid-blocks-survive-alongside-malformed, a valid-image structural control, and an end-to-end case
asserting the Google and Anthropic adapters build a request instead of throwing. 9 of 10 fail
without the source change; the valid-image control passes either way.
compaction and friends (21 files): 272 passed.
bun run typecheckandbun run privacy:scanpass.pre-existing on this Windows machine and were each reproduced on a clean
devcheckout whileverifying fix(google): never emit empty or malformed content parts (#420) #430 —
ci-workflows(2,Bun.spawnSync(["bun", …])needsbunonPATH; passes 10/10with it),
codex-catalogrouted-normalization (1), andopenai-provider-option-e2e(1, passesalone). None touch the Responses parser.
A note on how the full suite was run: a single
bun testprocess hangs on this machine partwaythrough the anthropic e2e files, and that hang reproduces on a clean
devcheckout with no changesapplied, so it is environmental. I ran the suite in 27 batches instead; the one batch that hit the
hang was then re-run file-by-file (120 passed, 0 failed). CI remains the authority.
Notes
matched the strict schema, so valid requests produce identical output. The one place I had to be
careful was
detail:b.detail ? …andtypeof b.detail === "string"differ fordetail: "", sothe check is
typeof b.detail === "string" && b.detail.length > 0to preserve the existingomit-on-empty behavior.
inputItemSchema's catch-allwould be the deeper fix, but that catch-all is load-bearing — it is how unknown and newly added
item types pass through the proxy — so narrowing it risks rejecting items that are currently
relayed fine. Happy to look at that separately if you want it.
Summary by CodeRabbit