fix(google): enforce response part field contract - #2246
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe Google adapter validates function-call payloads and names before emitting tool-call events. Streaming and buffered parsers share the validation path. Non-string text values are ignored. Tests and documentation cover these behaviors. ChangesGoogle response field validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: ✨ 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. |
리뷰 · 우선순위 57 / 80#2233 본체임. 컨테이너는 이미 #2232가 이 PR이 그 정책을 그대로 박음. 있는 테스트가 1st-party Google이 이 셰이프를 안 낸다는 말은 여전함. 노출은 호환
해결방안: 체크리스트 채우고 draft 해제 후 이 댓글은 grok-bot이 작성했습니다 |
snowyukitty
left a comment
There was a problem hiding this comment.
Reviewed at exact head cb4fbc1c, rebuilt locally and run through bun scripts/test.ts.
Declaring my position: I filed #2233, so this is the reporter confirming the implementation
matches the report and does not break anything adjacent — not an independent maintainer approval.
Per MAINTAINERS.md the merge still needs one of you; GitHub also refuses an APPROVE from an
account without repository access, so this is submitted as a comment with the verdict stated at the
bottom.
All fourteen reported shapes behave per the stated policy
| # | parts element |
On cb4fbc1c |
|---|---|---|
| A–D | functionCall is "x" / 5 / [], streaming and buffered |
terminal invalid function call (function_call_not_object; partIndex=0; valueType=…) — cannot dispatch |
| E, F, I | name absent / 5, streaming and buffered |
terminal function_call_name_invalid |
| G, H | name is "" / " " |
terminal function_call_name_blank |
| J–N | text is 5 / {a:1} / [1,2] / 5+thought:true, streaming and buffered |
dropped, turn completes |
| O | control: [{text:"hi"},{functionCall:{name:"lookup",args:{q:1}}}] |
unchanged |
Seventeen adjacent cases I probed for a gap, and did not find one
The ones that could plausibly have gone wrong:
- Fail-closed lands before any content in the same chunk escapes.
[{text:"hello"},{functionCall:{}}]
emits notext_delta— the wholepartsarray is validated first, matching how #2232 validates
the container before iterating. Same on the buffered path, and[{functionCall:{name:"ok"}},{functionCall:5}]
emits no tool call either. - Content already delivered in an earlier chunk is kept: chunk 1 text, chunk 2 malformed call
→[text("hello"), error(…)]. That is the right split and it is the #1219 line. partIndexis accurate — a bad call at index 2 reportspartIndex=2, not 0.name: "\t\n "is caught by the blank check, not just space.functionCall: nullis absence, not corruption —[{functionCall:null,text:"hi"}]still emits
text("hi")and completes. Consistent with howparts: nullandcontent: nullare read.argsmissing andargs: nullboth still serialize to{}— unchanged, as intended.- A non-string
thoughtSignatureon an otherwise valid call is inert.thoughtSignature: 5
emits the call with noproviderMetadata; a real 20-char base64 signature still attaches. Both
consumers were alreadytypeof === "string"-guarded —isLikelyRealThoughtSignature
(src/adapters/google-antigravity-wire.ts:30-31) andextractSignature
(src/adapters/google-antigravity-replay.ts:484-490) — so wideningtext/functionCallto
unknowndoes not expose them. - CCA rejects before replay observation. A
response-wrapped malformed call terminates without
entering the replay cache, which is the part of the docstring I most wanted to see hold. applyAntigravityReplayis a cleared site — it reads request contents, not response parts,
and already guardstypeof fc.name === "string"(:707).- #2232's container guards are intact, checked on this head:
parts: {}still
invalid content parts, bufferedcandidates: [null]stillinvalid candidates, and the
mid-stream{"candidates":null}frame still completes with its text rather than terminating.
The one design refinement worth calling out
The owner review on #2233 said "비문자열 text는 파트 drop". This implements field-drop, not
part-drop, and the docstring says why: "Dropping only this optional field preserves the rest of the
part". That is a better reading and it is load-bearing — [{text:5, functionCall:{name:"ok",args:{}}}]
still dispatches the call. Part-drop would have silently discarded a valid, named tool call over a
malformed sibling field, which is the exact "strand a tool turn" outcome #1332 was written to
prevent. Worth keeping the sentence in structure/04_transports-and-sidecars.md as the record of it.
One non-blocking observation
A part whose only content is a malformed text now yields a completed, empty turn
([{text:5}] → heartbeat, done). That is the chosen policy working as specified and I am not
asking for a change — terminating on optional malformed text would discard otherwise usable
responses, exactly as the Decision Log argues. Recording it because the net underneath it is
guardEmptyCompletionEventStream, not the adapter: the retry is what keeps this from being a silent
loss, so it is worth knowing that the two are now coupled for this shape.
Smaller, and genuinely cosmetic: GoogleResponsePart widens text and functionCall to unknown
but leaves thoughtSignature?: string / thought_signature?: string declaring string. Runtime is
safe for the reason above. Only mentioning it because the PR's own thesis is that this interface
should not claim what the wire does not guarantee.
Verification on this head
All through the repository runner, bun scripts/test.ts:
- 660 pass / 0 fail across 28 files —
google-hardening,google-adapter,google-empty-content,
google-buffered-stop-reason, the three Vertex suites, the three Antigravity suites,
google-signature-history-roundtrip,google-claude-prefill-guard,google-wire-compiler,
google-tool-schema,sse-null-data-frame, both adapter-conformance suites,
openai-chat-hardening,images/gemini-inline,server-images,
compatibility-provider-equivalence. bun x tsc --noEmit— output identical todev, both carrying only the pre-existing
Cannot find module '@napi-rs/keyring'.
No change requested from me. The three-way split — terminate on call identity, drop on optional
text, leave args provider-native — is the right set of lines, and it holds on every shape I could
construct.
cb4fbc1 to
0103abd
Compare
Summary
Fixes #2233 after the project owner policy decision:
functionCallunless it is an object with a nonblank stringnamepart.textvalues without coercionfunctionCall.argsexactly as beforeWhy this disposition
Gemini delivers a complete function call in one response part, so a missing or malformed name cannot be repaired by a later delta. Passing or coercing it can dispatch an unidentified tool. Optional malformed text is different: dropping only that field avoids fabricating assistant output while preserving the rest of the response.
Verification
bun run typecheck: passedbun run privacy:scan: passedgit diff --check: passedgui/node_modules, and service-token shim failures passed after unsetting the host token environment. The remaining translator contract failure is the installed TypeScript rejecting the repository test option--ignoreConfigbefore checking the fixture.Decision Log
Review readiness checklist
devat creation time.Summary by CodeRabbit
Bug Fixes
Documentation