fix(hooks): preserve non-text parts in fromHookLLMRequest#26275
Conversation
Before this change, fromHookLLMRequest rebuilt the SDK request's contents text-only from hookRequest.messages, discarding every non-text part (functionCall, functionResponse, inlineData, thought, etc.) from baseRequest. This broke any BeforeModel hook that modified text in conversations containing tool calls: the hook stripped tool call/response history, the model lost its prior tool context, and it re-invoked the same tool — producing an infinite tool-call loop. Fix it by merging hook text edits back into baseRequest.contents in place. Walk baseContents in order, consume hook messages by index, replace text parts with the hook's edited text, and preserve all non-text parts in their original positions. Entries that contributed no text to the hook view (e.g. a content with only a functionResponse) are preserved untouched. Extra hook messages beyond baseContents are appended as new turns. Falls back to the existing text-only behavior when baseRequest is absent or has no contents, and preserves the existing model-only-override fallback (when hookRequest.messages is undefined) added in #22326. Implementation and the six new regression tests are adapted from the contributor PR #23340 (auto-closed under the 14-day no-help-wanted policy, not on technical merit). Fixes #25558
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug where text-modifying hooks inadvertently destroyed essential tool-call and response context in LLM conversations. By refactoring the request translation logic to merge edits in place, the system now preserves non-text components, ensuring model continuity and preventing infinite tool-call loops. The changes also include improved type safety and comprehensive regression testing. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request addresses issue #25558 by updating the fromHookLLMRequest method to merge hook-modified text back into the original baseRequest.contents. This ensures that non-text parts, such as functionCall and functionResponse, are preserved during the translation process. The changes include a more robust merging logic and a comprehensive suite of regression tests. Feedback was provided regarding a potential logic discrepancy in how the translator identifies which content entries contributed to the hook view, specifically noting that empty text parts might cause the message index to become desynchronized.
| const parts: Part[] = content.parts ?? []; | ||
| const hasText = parts.some(hasTextProperty); | ||
| const baseContent: Content = { ...content, parts }; | ||
|
|
||
| if (!hasText) { | ||
| // toHookLLMRequest skipped this entry — preserve it untouched so | ||
| // tool-call/response history is not lost. | ||
| merged.push(baseContent); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The logic for determining if a content entry contributed a message to the hook view should exactly match the logic in toHookLLMRequest. Currently, toHookLLMRequest only emits a message if the joined text content is non-empty (see lines 191-197). However, fromHookLLMRequest uses parts.some(hasTextProperty), which returns true even if the text parts are empty strings (e.g., [{ text: '' }]). This discrepancy will cause messageIndex to get out of sync if the conversation contains empty text parts.
Additionally, to be consistent with the defensive handling in toHookLLMRequest (lines 186-188), we should ensure parts is treated as an array to avoid potential runtime errors if the SDK returns a single part object instead of an array.
const parts = Array.isArray(content.parts)
? content.parts
: content.parts
? [content.parts]
: [];
const textContent = parts
.filter(hasTextProperty)
.map((part) => part.text)
.join('');
const baseContent: Content = { ...content, parts };
if (!textContent) {
// toHookLLMRequest skipped this entry — preserve it untouched so
// tool-call/response history is not lost.
merged.push(baseContent);
continue;
}|
Size Change: +2.04 kB (+0.01%) Total Size: 33.9 MB
ℹ️ View Unchanged
|
… #26275 Second batch of drip-213 reviews: - openai/codex#20465 derive PermissionProfile directly in test helper (merge-as-is) - BerriAI/litellm#26895 preserve aiohttp raw response headers for UTF-8 (merge-as-is) - BerriAI/litellm#26893 cache-read pricing in custom-cost path (merge-after-nits) - google-gemini/gemini-cli#26275 preserve non-text parts through hook translator (merge-after-nits)
Summary
Fixes a bug where
BeforeModelhooks that modify text in conversationscontaining tool calls cause the model to enter an infinite tool-call loop.
HookTranslatorGenAIv1.fromHookLLMRequestrebuilt the SDK request'scontentstext-only fromhookRequest.messages, discarding every non-textpart (
functionCall,functionResponse,inlineData,thought, etc.) frombaseRequest. The hook stripped tool call/response history, the model lostits prior tool context, and it re-invoked the same tool.
This blocks any text-modifying hook (PII redaction for FERPA/HIPAA, prompt
rewriting, etc.) the moment a tool call enters the conversation — which is
essentially always for an agent.
Details
fromHookLLMRequestnow merges hook text edits back intobaseRequest.contentsin place instead of rebuilding
contentswholesale:baseContentsin order, consuming hook messages by index.replaces text parts with the hook's edited text and preserves all non-text
parts in their original positions.
toHookLLMRequest(no text parts), passesthem through unmodified.
baseRequestis absent or has nocontents (preserves existing semantics).
hookRequest.messagesis undefined) added in fix(core): handle partial llm_request in BeforeModel hook override #22326.isContentWithPartsto narrow toContentso the implementationhas zero unsafe casts and no eslint suppressions.
Implementation and the six new regression tests are adapted from the
contributor PR #23340, which was auto-closed under the 14-day no-
help wantedpolicy (not on technical merit — Gemini Code Assist's own review on that PR:
"The implementation is robust... I found no issues."). Credit to
@mk-imagine for the original diagnosis, algorithm, and tests.
Related Issues
Fixes #25558
Supersedes #23340
How to Validate
Reproduce the bug on
main— write aBeforeModelhook that modifiestext (e.g. PII redaction), start a conversation that triggers a tool call,
then submit a follow-up prompt. Observe the model re-invoking the same tool
in a loop because prior tool context is gone.
Run the new regression tests (which fail on
main):npm test -w @google/gemini-cli-core -- src/hooks/hookTranslator.test.tsAll 18 tests should pass — 12 pre-existing plus 6 new under
fromHookLLMRequest with baseRequest (non-text part preservation):functionCallparts when merging hook text backbaseRequestis undefinedbaseRequesthas no contentsConfirm no regression in adjacent hook code:
npm test -w @google/gemini-cli-core -- src/hooks/All 155 tests across
packages/core/src/hooks/should pass.Typecheck and lint clean on the touched files:
Pre-Merge Checklist
data that was previously dropped). Hooks that were already round-tripping
successfully (text-only conversations, or hooks that returned
{}) areunaffected.