fix(openai-chat): treat a non-string repeat as padding once that field is known - #2165
Conversation
|
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 (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe adapter now tracks canonical streamed tool-call fields, tolerates malformed repeated padding after resolution, and emits call-specific diagnostics for invalid fields. Tests cover accepted padding, terminal errors, provenance isolation, malformed IDs, and parallel-call attribution. ChangesStream validation hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The change is localized to OpenAI-compatible stream parsing and its tests, with no actionable merge-blocking risk remaining beyond normal checks and review. Possibly related PRs
Suggested labels: 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 |
Stack mapMerge bottom-up; each layer's base is the branch below it.
All five are rebased onto the current |
|
✅ Deterministic PR hygiene checks passed. |
…d is known Some OpenAI-compatible streamers repeat an already-sent id, name, or arguments as a non-string placeholder on a continuation delta rather than as null. Validation ran before the pending-call lookup, so the whole turn died with a 502 and the tool never ran -- even though the value being repeated was already held in canonical form. The lookup now happens first and tolerance is per field, keyed on that field's own provenance. Two corrections on top of @waw4303's #2155. It gated arguments acceptance on the call having a canonical NAME. A name says nothing about whether arguments was ever sent as a string, so a real argument payload could be silently dropped. PendingToolCall now carries sawArgumentsString; an empty string counts, because it proves the upstream sent the field with the right wire type. It also left a non-string repeated id unconditionally terminal even after a canonical id was stored. Ids now follow the same rule as the other two. Diagnostics are passed from the rejection site instead of rescanned. A stateless rescan stops at the first structurally odd value, so a stream carrying accepted padding on call 0 and a real defect on call 1 blamed call 0.
fc97a6d to
772d375
Compare
4ee6161 to
293494c
Compare
|
Manual review of the current layer found the per-field provenance correction materially safer than #2155: I am not approving yet because CodeRabbit skipped this non- After automated review is complete, the lower-stack blockers are resolved, and the child is retargeted with exact-head CI, this remains a strong merge candidate. |
|
✅ Action performedReview finished.
|
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Approved for this stacked layer at exact head 772d375f. The per-field provenance model keeps name, arguments, and id independent, accepts non-string repeats only after that exact field has a canonical value, and preserves call-specific fail-closed diagnostics without logging attacker-controlled values. Focused validation passed 97/97 at the stack tip; exact-head CI is now green on all reported platforms, and the requested CodeRabbit review produced no actionable findings.
This approval is for the #2165 layer only. Do not merge it out of order: resolve the lower-stack blockers, retarget this child to dev after its parent lands, and require fresh exact-head CI on the retargeted PR.
리뷰 · 우선순위 56 / 80#2155를 가져온 거임. 지금 스트림 루프가 pending 조회 전에 #2155 구멍 둘이 여기서 막힘. 원본은 args 검사를 #2156이랑 다른 구멍임. 이건 패딩 502. 저건 finish_reason 없이 툴콜 잘림. 이 PR로 #2156 닫지 말 것. 스택 레이어 5. 베이스 #2164. #2151이랑 openai-chat 구간이 달라서 코드 의존은 아님. types.ts/config.ts 안 건드림. #2155는 머지되면 닫고 드래프트 리베이스하지 말 것. 점수는 56임. 맞음. 2.28. #2164 뒤. 해결방안: #2164 다음. 이 댓글은 grok-bot이 작성했습니다 |
2 participants
Summary
Absorbs #2155 by @waw4303. Stack layer 5 — base is
codex/absorb-opencode-go-quota-siblings(#2164), notdev.The defect. Some OpenAI-compatible streamers repeat an already-sent
id,name, orargumentsas a non-string placeholder on a continuation delta rather than asnull. Validation ran before the pending-call lookup, so the adapter could not see that the same wire index already held that value in canonical form. The stream died with a 502 and the tool never ran.Reproduced on unpatched
dev:upstream response contained invalid tool calls (tool_call_function_name_invalid; callIndex=0; valueType=object), with the following valid{"command":"ls"}fragment never consumed.Two corrections on top of #2155.
Too wide for
arguments. The original gated acceptance on the call having a canonical name. A name says nothing about whetherargumentswas ever sent as a string — so a chunk establishingname: "terminal"with noargumentsfield, followed byarguments: {command:"ls"}, would silently drop a real payload the model intended to send.PendingToolCallnow carriessawArgumentsString. An empty string counts: it proves the upstream sent the field with the right wire type, which is exactly what a later malformed repeat would be padding for.Too narrow for
id. A non-string repeatedidstayed unconditionally terminal even after a canonical id was stored. Ids now follow the same per-field rule.Diagnostics. These are now passed from the rejection site rather than rescanned. A stateless rescan stops at the first structurally odd value it sees — so a stream carrying accepted padding on call 0 and a real defect on call 1 blamed call 0. Both
invalidToolCallsEventandlogInvalidToolCallstake an optional override; structural failures before pending state is relevant still use the stateless fallback.Sibling-vs-stacked note: #2155 has no dependency on the openai-chat hunks in #2151 (imports/service-tier around lines 14–1343 vs stream parsing around 1628–1677). It sits here for a clean single chain, not because the code requires it.
Closes #2155.
Verification
bun run typecheck— clean.bun test --isolate tests/openai-chat-hardening.test.ts tests/openai-chat-invalid-tool-call-diagnostics.test.ts— 62 pass / 0 fail (58 pre-existing, unchanged by the refactor).src/adapters/openai-chat.tsgives 2 pass / 2 fail. The two that fail are the bug being fixed. The two that pass — canonical-name-does-not-authorize-arguments, and malformed-id-stays-terminal — pass on unpatcheddevbecausedevrejects everything; they are the guards proving this PR did not widen tolerance while fixing it.bun run testat the stack tip — 13562 pass / 10 skip / 0 fail across 857 files.bun run privacy:scan— passed.Checklist
Diagnostics still report shape only, never values — the existing privacy contract for this channel is unchanged.
Stack (bottom to top): #2134 → #2160 → #2162 → #2164 → this
Summary by CodeRabbit
Bug Fixes
Tests