fix(adapters): validate buffered response and ndjson frame shapes (#2531) - #2532
Conversation
`JSON.parse("null")` returns null without throwing, so a try/catch around a
body parse cannot see it. lidge-jun#1240 closed that at the SSE frame root for the four
SSE parsers; the buffered bodies and the NDJSON transport were never swept.
- google/anthropic parseResponse: a valid-JSON non-record body reached
`raw.error` and `json.content` and threw out of the adapter. A buffered body
has no next frame to recover into, so both now fail closed with a structured
error, matching the unparseable-body branch beside them.
- anthropic parseResponse: `content` was consumed unchecked. A present
non-array was silently accepted, and a string is iterable, so a claimed
answer was walked one character at a time and reported as a successful empty
turn. Absence (omitted or null) stays legal.
- openai-chat parseResponse: `if (!choice.message)` split this input class on
truthiness, not shape - null and 0 failed closed while "text", true and
[{...}] passed and completed as a successful empty turn, stranding any tool
call the choice claimed. Empty array stays legal, as in the google adapter.
- command-code ndjson: a non-record line crashed on `event.type`. A stream
frame does have a next frame, so it is skipped as padding, per lidge-jun#1240.
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request has been marked Ready for Review. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe adapters now validate buffered JSON response shapes before reading nested fields. Anthropic reports structured 502 errors for malformed content. Command Code skips malformed NDJSON frames. OpenAI Chat rejects non-object messages. Tests cover failures, recovery, valid responses, and budget cleanup. ChangesResponse shape guards
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR adds localized validation for malformed buffered and NDJSON responses, converting crashes or silent empty results into the intended error or skip behavior. No actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue Full details: Out of Scope Changes checkExplanation The changes remain within issue ✨ Finishing Touches🧪 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 |
|
Verification record for this head ( Local test evidence
Disclosing the one failure rather than silently ticking the box. It is The typecheck line above is parity, not a clean exit: clean How the findings were produced. Not from a field report — from an enumerative sweep that drives
Pre-submission review. The diff went through an independent three-reviewer panel before
The third is the |
리뷰 · 우선순위 68 / 80설명: 이 풀은 버퍼로 받은 응답과 커맨드코드 NDJSON 한 줄이 레코드가 아닐 때 턴이 터지거나 빈 성공으로 끝나는 구멍을 막는다. 지금 CURRENT dev HEAD 는 98ed186 이다. 이번 시간에 SHA 는 안 움직였다. 작성자는 snowyukitty 다. 라벨은 bug 와 review-ready 다. 드래프트가 아니다. mergeable 은 true 다. 베이스는 지금 origin/dev 다. 라벨은 바꾸지 말 것. HEAD 의 구멍은 네 군데다. command-code.ts ndjson 368줄과 379줄은 JSON.parse 를 try/catch 로만 감싼다. JSON.parse 에 null 을 넣으면 던지지 않고 null 을 준다. 그 null 이 parseStream 527줄 switch (event.type) 에서 TypeError 가 된다. google.ts parseResponse 1161줄은 같은 이유로 raw.error 에서 터진다. anthropic.ts 1282줄은 json.content 에서 터진다. openai-chat.ts 1934줄은 if (!choice.message) 가 참거짓만 본다. 문자열이나 true 나 배열은 통과하고 속성 읽기가 전부 undefined 가 되어 빈 done 으로 끝난다. anthropic 의 content 가 문자열이면 for-of 가 글자 하나씩 돌고 역시 빈 성공이다. 이 길은 죽어 있지 않다. responses/core.ts 가 비스트림 턴에서 parseResponse 를 부른다. 이 풀은 네 규칙을 맞춘다. 스트림 프레임 루트가 레코드가 아니면 건너뛴다. 버퍼 본문 루트가 레코드가 아니면 에러로 닫는다. 안에 있다고 주장한 content 와 message 가 모양이 아니면 역시 닫는다. content 빈 배열은 살리고 message 빈 배열은 거절한다. 구글의 isGoogleRecord 와 googleStructuralValueType 은 이미 HEAD 483-490줄에 있다. 새로 만들지 않았다. 커맨드코드는 decodeEventLine 한 곳으로 줄 루프와 끝 버퍼를 모았다. debugDroppedFrame 은 이미 src/lib/debug.ts 18줄에 있다. 시험은 tests/buffered-response-shape-guards.test.ts 하나다. 구글 본문 루트, 클로드 본문 루트, content 칸과 블록, 커맨드코드 중간 줄과 끝 버퍼, 챗 메시지, 예산 누수를 본다. 끝 버퍼 시험은 마지막 줄바꿈을 빼서 final 분기를 탄다. 예산 시험은 dispose 전에 재고를 본다. 구글 비레코드 본문은 예산 목록에 없다. 그 길은 charge 전에 돌아가서 새 누수는 아니다. 위쪽이 준 숫자는 46 통과다. 내가 여기서 다시 돌리지는 않는다. 합치기 전에 남는 제품 결정은 커맨드코드다. 쓰레기만 있는 스트림은 예전엔 TypeError 였고 이 풀 뒤에는 done 하나다. 빈 본문, 빈 줄, 파싱 실패 스트림이 이미 HEAD 592줄 if (!sawFinish) 에서 같은 끝이다. 비대칭을 만들지 않으려면 네 번째도 같다. 이건 고치기 전에 이미 있던 계약이다. 이 풀이 새로 정할 일이 아니다. types.ts 와 config.ts 가르기와는 무관하다. 프리뷰 배포가 아니다. 2531 은 이 풀이 머지되기 전에 닫지 말 것. 1240 을 다시 열지 말 것. 2423 2472 2210 도 이 풀로 닫지 말 것. 그 구멍은 다른 길이다. src/adapters/command-code.ts 368줄 HEAD - JSON.parse 가 null 을 던지지 않아 event.type 에서 턴이 죽는다 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
2 participants
Closes #2531.
Problem
JSON.parse("null")returnsnullwithout throwing, so atry/catcharound a body parse cannot seeit. #1240 closed that at the SSE frame root for the four SSE parsers, and reported: "The other
eight SSE data-frame parsers were audited and are already correct."
Accurate about what it covered. A buffered body is not an SSE data frame, and Command Code is
NDJSON:
command-code.tsndjson()nullnull is not an object ('event.type')google.tsparseResponsenullnull is not an object ('raw.error')anthropic.tsparseResponsenullnull is not an object ('json.content')anthropic.tsparseResponsecontent=true/{}is not iterableanthropic.tsparseResponsecontent=[null]/content[i]=nullnull is not an object ('block.type')anthropic.tsparseResponsecontent= a stringopenai-chat.tsparseResponsemessage="txt"/true/[{…}]parseResponseis the live non-streaming path (src/server/responses/core.ts:5065), not dead code.F and G are the ones worth reading twice. A string is iterable, so
for (const block of "a claimed answer")walks it one character at a time, emits nothing, and reportsa clean
done— a successful empty turn for a response that claimed content.G is the same outcome from a different cause:
if (!choice.message)splits this input class ontruthiness, not shape.
nulland0fail closed;"text",trueand[{…}]pass, everyproperty read yields
undefined, and the turn completes successfully — stranding any tool call thechoice claimed. The guard is one line below a full record check on the choice container itself.
Fix
Four rules, each precedented, so the diff contains no asymmetry for one input class:
A non-record frame root in a stream is skipped as padding —
src/adapters/command-code.ts.#1240's rule: terminating discards an answer whose deltas have already arrived. The parse and guard
move into one
decodeEventLinehelper so the newline loop and the trailing-buffer branch — whichshared the defect — cannot drift apart again.
A non-record body root in a buffered response fails closed —
src/adapters/google.ts,src/adapters/anthropic.ts. No next frame to recover into, and this matches the unparseable-bodybranch sitting immediately beside it in
google.ts.Malformed nested claimed content fails closed —
src/adapters/anthropic.ts(contentmust be anarray of records),
src/adapters/openai-chat.ts(messagemust be a record). #1332/#2232's rule.An empty array stays legal for
content, and NOT formessage.contentis genuinely an arrayof blocks, and
diagnoseGoogleContentalready accepts[]there.messageis a record on aplain-JSON wire that already has
{}, so the google carve-out is a protobuf-wire artifact that doesnot transfer — importing it would be reasoning from analogy rather than from this wire. Absence
(omitted, and
null) is unchanged everywhere.openai-chatreadschoice.messagethroughunknownrather than the declared type:choicesis acast over wire data, so its
message?: Record<string, unknown>is an assertion the upstream nevermade — narrowing against it is what let the missing check look type-safe.
Diagnostics name the rung and value type (
content_not_array,content_block_not_object; blockIndex=1; valueType=null), shaped likeinvalidGoogleShapeEvent.Tests
tests/buffered-response-shape-guards.test.ts— 46 pass / 0 fail.Every assertion is parity against a control that was already correct — an unparseable body, an
unparseable line, or the falsy-message cases that already failed closed — so the test states the
requirement rather than re-encoding each adapter's wording.
Activation: on clean
dev@98ed186c, same file and runner: 21 pass / 25 fail. The 22 passingon both are the controls, including the absence encodings and empty-array carve-outs that must keep
working.
Blast radius: 97 files importing any of the four adapters — 1591 pass / 1 fail. The failure is
tests/translator-budget.test.ts(--ignoreConfigis not a recognisedtscoption in thisenvironment; expects
TS2554, getsTS5023). Controlled on a detachedupstream/dev@98ed186ccheckout, same runner and file: fails identically, 14 pass / 1 fail. Pre-existing.
Typecheck identical to clean
dev(one pre-existing@napi-rs/keyringerror). Privacy scanpassed.
Notes
How this was found. Not from a report — from an enumerative sweep driving each adapter's real
parser over every addressable JSON location in a healthy exemplar, with values drawn from what
actually reproduced in #1219/#1325/#2231/#2232 rather than a generic corpus. 2,937 single-location
mutations. Baseline vs. patched, same harness:
openai-responsesand every streaming path are cell-for-cell identical — the change is confined tothe four adapters it claims.
Coverage is a census, not a sample.
kiroandcursorcannot be driven by the sweep (binaryevent-stream framing;
runTurnnever receives aResponse). Rather than build exemplar builders,every
JSON.parseandresponse.json()site insrc/adapters/andsrc/web-search/was enumeratedand its guard read — for this class that is conclusive where a sweep is only ever a sample. All are
clean; the table is in the issue. Kiro is the positive example this PR is modelled on.
What this does not cover: single mutations only, one healthy exemplar per adapter. Finding F was
found by a hand probe, not the sweep; the oracle was then extended so that class surfaces
automatically, which is what found G.
One behaviour change to flag rather than bury. 19 anthropic cells and 4 openai-chat cells move
from silently-accepted to a structured error: a present-but-malformed
content/messageused to beaccepted as an empty turn and is now refused. That is the point of D/F/G, but it is a real tightening
on paths that previously accepted anything. Absence and empty-array encodings are unchanged and
covered by their own tests.
A behaviour change to state plainly rather than soften. On
dev, a Command Code stream whoseonly frame is
null(or any non-record JSON) throwsTypeError: null is not an objectout ofswitch (event.type). After this change it yields[done]. A crash becomes an empty success forthat input. The full picture:
dev[done][done][done][done]{not json})[done][done]null)[done]nullbetween real deltas[text_delta, …, done]The last row is the observed #1219 shape and the reason for the change. The fourth row joins an
existing class: a stream that produced no valid event, whose other three members already end in a
single terminal
doneon unmodifieddev. Making only the fourth fail closed would create theasymmetry, not remove it; making all four fail closed would rewrite pre-existing behaviour this diff
does not otherwise touch, and would make this the only parser in the family that fails closed at EOF.
This does not contradict findings F and G above. Those are about reporting success while
delivering nothing when the payload claimed content. A junk-only stream claims nothing. Where
content is claimed, this diff reports it — that is the last row.
The residual question is real, and is deliberately left open.
if (!sawFinish) yield donemeansthis adapter reports any no-valid-event stream as an empty success, which for a client that is
itself a coding agent is arguably worse than a 502. That is pre-existing, applies to all four inputs
equally, and is a product decision about this adapter's terminal contract — not something a crash
fix should decide. Happy to file it separately if you want it addressed.
Review record. This diff was put through an independent three-reviewer panel before submission
(gpt-5.6-sol xhigh, grok-4.6, gemini-3.7-flash). Two approved; one requested changes with three
findings. Two were accepted and fixed here — a cargo-culted empty-array carve-out on
choice.message, and a test whose comment claimed trailing-buffer coverage it did not have becausethe helper appended a newline to every line. The third is the behaviour change disclosed above,
declined on the reasoning given. A fourth finding from a different reviewer (an orphaned JSDoc) was
also accepted. Details available if useful.
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
Bug Fixes
Tests