fix(google): keep thought parts out of visible text - #1508
Conversation
|
✅ Deterministic PR hygiene checks passed. |
|
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 (4)
📝 WalkthroughWalkthroughThe Google adapter now preserves the ChangesGoogle thought-text handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GoogleResponse
participant GoogleAdapter
participant ResponseEvents
GoogleResponse->>GoogleAdapter: return streaming or buffered response part
GoogleAdapter->>GoogleAdapter: inspect thought flag
GoogleAdapter->>ResponseEvents: emit reasoning_raw_delta for thought text
GoogleAdapter->>ResponseEvents: emit text_delta for ordinary text
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 |
320f905 to
b3ba283
Compare
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
First review on exact head b3ba2838e2d226cd7967e2a17b0545f8a653d260.
The implementation matches #1503's visibility-boundary requirement. The shared classifier maps text-bearing thought: true parts to reasoning_raw_delta in both SSE and buffered JSON parsing, ordinary text remains text_delta, and function-call ordering remains intact. The focused regressions exercise standalone thought text, thought-before-tool ordering, ordinary visible text, and both response modes. I did not find a code-level blocker in this patch.
The exact-head validation is also real here: Cross-platform CI completed successfully with 18 jobs, including the macOS test job and the gates job, and React Doctor completed successfully.
The only remaining blocker is integration freshness. Current dev is 70d2e1758c180188a729dd63812703b76cfeeba6; this head is 11 commits behind with merge base 4fed8d3fe431ad23be83f3aff2af18ef8b8ecd71. The intervening production changes do not touch src/adapters/google.ts or the focused Google test, but the combined tree has not been exercised by CI, and structure/04_transports-and-sidecars.md has advanced on dev.
Please rebase onto current dev and rerun exact-head CI. If the integrated diff remains equivalent and CI stays green, I see no remaining blocker and would approve on re-review.
b3ba283 to
219e7f3
Compare
|
Rebased onto current |
|
The current |
|
I prepared a current fork-side integration candidate without touching the maintainer-owned PR branch. Fork branch |
|
Correction to my previous status note: the repository |
|
Superseded by the final current-dev update below. The authoritative fork-side candidate is now based on |
|
Updated candidate after the final current-dev rebase: Base: Post-rebase proof: all three Google regression files passed — 81 tests, 0 failures, 226 expect calls; typecheck, privacy scan, and diff check passed. Please cherry-pick |
|
Current candidate for this maintainer-owned PR: rebased onto upstream Post-rebase proof: Google hardening, Antigravity wire, and Vertex thought-signature tests 81 passed, 0 failed, 226 expect calls; typecheck, privacy scan, and diff check passed. Please cherry-pick/rebase this candidate into the maintainer branch and rerun exact-head CI. I have not rewritten the maintainer-owned branch. |
…hought parts Two contracts the thought-visibility change touches implicitly, made explicit so neither can drift. emittedContentEvent decides content vs continue, and its only consumer is the synthetic-heartbeat suppression in the read loop. A thought delta is real upstream activity, so it must count as content — emitting a heartbeat alongside it would claim the stream was idle while the model was working. The visible-text case is asserted next to it as the control. Gemini 3 rejects a follow-up turn whose first function-call part lost its thoughtSignature, so a classification change that also dropped replay would trade a visible-text bug for a hard 400. The new case observes a payload mixing a thought part with a signed function call and asserts the signature is still replayed, rather than inferring safety from unrelated fixtures that happen to still pass.
|
Reviewed and verified this against current I pushed two tests on top of your commits rather than changing your implementation, because the change touches two contracts implicitly and neither was pinned:
Verification at the merged head (
Red-before check for the new tests: reverting only Marking ready for review. |
|
Final current candidate: upstream Final local proof: 81/81 Google/Antigravity/Vertex tests, 226 expect calls, plus typecheck, privacy scan, and diff check. Please cherry-pick/rebase this candidate into the maintainer-owned branch and rerun exact-head CI; the maintainer branch remains untouched. |
![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: e0d2df3b35
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function googlePartTextEvent(part: GoogleResponsePart): AdapterEvent | undefined { | ||
| if (!part.text) return undefined; | ||
| return part.thought === true | ||
| ? { type: "reasoning_raw_delta", text: part.text } | ||
| : { type: "text_delta", text: part.text }; |
There was a problem hiding this comment.
When Google returns a thought: true part, this changes the public Responses/Chat contract from ordinary assistant text to reasoning output (and Responses requests with reasoning.summary: "none" suppress it entirely), but docs-site/src/content/docs/reference/adapters.md and its translations still omit this behavior. Add the visibility and output-channel rule to the Google adapter documentation and synchronize the translated pages.
AGENTS.md reference: src/AGENTS.md:L24-L28
Useful? React with 👍 / 👎.
|
Final CI status: the exact merged head |
4 participants
Summary
thought: trueout of ordinary assistant text in both SSE and buffered JSON parsing.reasoning_raw_deltawhile preserving ordinary text, function-call ordering, inline images, and Antigravity/Vertex thought-signature observation.Closes #1503
Verification
bun test tests/google-hardening.test.ts tests/google-antigravity-wire.test.ts tests/google-vertex-thought-signature.test.ts— 81 passed, 0 failed.bun run typecheck— passed.bun run privacy:scan— passed.bun run test— the repository-wide run reached the 600-second local resource limit before completion. The new Google regressions passed in that run; unrelated existing timing/environment failures were observed in Claude replay, native-main ownership, server auth, and npm-cache preflight tests. A focused rerun of those unrelated files still reproduced baseline failures and did not involve the Google adapter paths changed here.Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation