feat(cli): show acknowledgment when user steering hint is processed#26498
feat(cli): show acknowledgment when user steering hint is processed#26498euxaristia wants to merge 6 commits intogoogle-gemini:mainfrom
Conversation
Extend `generateSteeringAckMessage` with an optional `signal` so callers can cancel the request when the user aborts the active turn. Sanitize inputs passed through `<user_input>` / `<background_output>` wrappers to escape both XML close tags and `]` context-breakers, preventing prompt-injection through these helpers. Bug fixes folded in: - short-circuit and return the fallback when the parent signal is already aborted (previously added a listener that never fired) - remove the `abort` listener in a `finally` block to avoid leaking it across cancelled turns Drive-by: fix the broken `'src/telemetry/llmRole.js'` import path in the test file so the suite can be loaded under workspace tooling. Refs google-gemini#26485
When the user submits a steering hint mid-turn, kick off a fast, cancellable acknowledgment generation in parallel with the main turn and render it as an info line in the transcript. This gives the user an immediate, concrete signal that the hint was received and how it was interpreted, instead of waiting silently for the model to echo the steering itself. The promise is fire-and-forget but properly handled: - abort signal from the active turn is propagated; nothing is added to history if the turn is cancelled before the ack returns - the timestamp is captured at submit time so the ack appears in the correct chronological position even if it resolves after later history items - promise rejection is silently swallowed (with `AbortError` filtered) since the ack is non-critical UI feedback Adds a `generateContent` response to the steering test fixture so the request lifecycle has the response it expects. Refs google-gemini#26485
|
/gemini review |
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 introduces a more responsive user experience for steering hints by providing immediate visual feedback in the CLI transcript. It addresses previous feedback regarding promise handling, signal propagation, and security by hardening the input sanitization logic used for steering hints and background outputs. The changes ensure that the UI remains consistent and safe even when users cancel operations mid-turn. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements UI feedback for user steering hints by generating and displaying acknowledgment messages. Key changes include updating the useGeminiStream hook to trigger these acknowledgments, adding sanitization logic in fastAckHelper to escape XML tags and context-breaking characters, and improving generateSteeringAckMessage with AbortSignal support for better lifecycle management. Feedback was provided regarding the sanitization regex, noting that it could be bypassed by variations in tag structure and suggesting a more robust implementation to prevent prompt injection.
| /** | ||
| * Wraps user input in XML-like tags to mitigate prompt injection. | ||
| */ | ||
| const XML_TAG_REPLACE_RE = /<\/(\w+)>/g; |
There was a problem hiding this comment.
The regular expression XML_TAG_REPLACE_RE used for sanitizing untrusted input is incomplete and can be bypassed. It only matches closing XML tags that consist of word characters and are immediately followed by a >. An attacker can bypass this by including whitespace or attributes within the tag (e.g., </user_input >), which many LLMs will still interpret as a valid closing tag. This allows for prompt injection. Additionally, repository rules require that data from LLM-driven tools be sanitized by removing newlines and context-breaking characters (like ']') before injection into system prompts. The regex should be case-insensitive and robust against variations in tag structure to ensure all variations of closing tags are caught.
| const XML_TAG_REPLACE_RE = /<\/(\w+)>/g; | |
| const XML_TAG_REPLACE_RE = /\u003c\/([^\u003e]+)\u003e/gi; |
References
- Sanitize data from LLM-driven tools by removing newlines and context-breaking characters (e.g., ']') before injecting into system prompts.
- Sanitize additional context from hooks by escaping or removing HTML-like tag characters like '<' and '>'.
There was a problem hiding this comment.
Code Review
This pull request introduces steering hint acknowledgments in the CLI UI and implements sanitization for user inputs and background outputs to mitigate prompt injection. Key changes include updating generateSteeringAckMessage to support AbortSignal propagation and adding logic to display acknowledgment messages in the message history. Feedback focuses on two critical areas: the sanitization regex for XML tags is currently too restrictive and should account for optional whitespace to prevent bypasses, and the AbortSignal used for acknowledgments in useGeminiStream may be tied to the wrong turn lifecycle, suggesting the use of queueMicrotask to capture the updated signal.
| /** | ||
| * Wraps user input in XML-like tags to mitigate prompt injection. | ||
| */ | ||
| const XML_TAG_REPLACE_RE = /<\/(\w+)>/g; |
There was a problem hiding this comment.
The regular expression XML_TAG_REPLACE_RE used for sanitizing untrusted input is too restrictive. It only matches closing tags that contain word characters and no whitespace (e.g., </user_input>). An attacker can bypass this sanitization by using variations such as </user_input > or </user_input\n>, which many LLMs may still interpret as a closing tag. This allows for prompt injection, particularly through background process output which is wrapped using this function. To improve robustness and align with repository standards for sanitizing LLM-driven tool data, the regex should account for optional whitespace before the closing bracket.
const XML_TAG_REPLACE_RE = /\u003c\\/(\w+)\s*\u003e/g;References
- Sanitize data from LLM-driven tools before injecting it into a system prompt to prevent prompt injection. At a minimum, remove newlines and context-breaking characters (e.g., ']').
| const signal = abortControllerRef.current?.signal; | ||
| void generateSteeringAckMessage(config.getBaseLlmClient(), hintText, { | ||
| signal, | ||
| }) |
There was a problem hiding this comment.
The AbortSignal captured here belongs to the turn that is currently finishing, but the steering hint acknowledgment is intended for the subsequent continuation turn. Since submitQuery (called at line 2046) replaces abortControllerRef.current with a new controller, the signal used here will not be aborted if the user cancels the continuation turn. To ensure the acknowledgment is correctly tied to the new turn's lifecycle and prevent dangling processes, you should trigger the acknowledgment generation after submitQuery has updated the reference (e.g., using queueMicrotask). This ensures the operation accepts and propagates the correct AbortSignal as per repository standards.
const ackTimestamp = Date.now();
queueMicrotask(() => {
const signal = abortControllerRef.current?.signal;
void generateSteeringAckMessage(config.getBaseLlmClient(), hintText, {
signal,
})
.then((ackText) => {
if (signal?.aborted) return;
addItem(
{
type: MessageType.INFO,
icon: '· ',
color: theme.text.secondary,
marginBottom: 1,
text: ackText,
} as HistoryItemInfo,
ackTimestamp,
);
})
.catch((err) => {
if (err?.name === 'AbortError') return;
// Silently ignore — steering ack is non-critical UI feedback.
});
});References
- Asynchronous operations that can be cancelled by the user should accept and propagate an AbortSignal to ensure cancellability and prevent dangling processes or network requests.
Address PR review feedback on the closing-tag regex: - Match closing tags case-insensitively (`</USER_INPUT>` was previously passing through unsanitized). - Allow whitespace and attributes inside the tag (`</user_input >`, `</background_output foo="bar">`) which many LLMs still parse as valid closing tags. Switch the inner capture from `\w+` to `[^>]+`. - Escape raw newlines (LF and CRLF) to `\n` literal inside the wrappers, per repo prompt-injection rules. Steering paths already call `normalizeSpace` upstream, so this only affects multi-line background output going into `<background_output>`. Tests added for whitespace-in-tag, attribute-in-tag, mixed/upper case, and newline-escape cases.
|
Addressed in 0351b52:
The /gemini review |
The signal captured at the steering-hint call site belonged to the turn that was *finishing* (the one that just ran tool calls), not the *continuation* turn that submitQuery is about to start. submitQuery installs a new AbortController synchronously inside its body, so the signal we had already captured was orphaned: a Ctrl+C during the continuation aborts the new controller while our captured signal stays live, leaving the ack to post a "ghost" history item after the user thought they cancelled. Defer the signal capture and ack launch into a microtask so the new controller is in place first. Also guard the `addItem` call on `turnCancelledRef.current` as a belt-and-suspenders check against the acknowledgment outliving cancellation.
|
Two new items addressed: Re: useGeminiStream.ts:2009 (signal captured for the wrong turn) — fixed in e81028a. Wrapped the launch in Re: fastAckHelper.ts XML close-tag regex — I think the new suggestion may be regressing the regex that already shipped in 0351b52. Current code (line 60) is: The bot's suggested
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a steering acknowledgment mechanism for the Gemini CLI, allowing the UI to provide feedback when user steering hints are processed. It includes updates to useGeminiStream to trigger these acknowledgments, a new sanitizeForWrapper utility in fastAckHelper to prevent prompt injection by escaping XML tags and context-breaking characters, and comprehensive unit tests for both the acknowledgment flow and the sanitization logic. My feedback highlights that the current implementation of sanitizeForWrapper is ineffective due to how backslashes are handled in JavaScript string literals and provides a corrected implementation.
| function sanitizeForWrapper(input: string): string { | ||
| return input | ||
| .replace(XML_CLOSING_TAG_RE, '<\\/$1>') | ||
| .replace(CONTEXT_BREAKER_RE, '\\]') | ||
| .replace(NEWLINE_RE, '\\n'); | ||
| } |
There was a problem hiding this comment.
The sanitization logic for XML closing tags and context breakers is currently ineffective. In JavaScript string literals, the backslash is an escape character. For characters that do not have a special escape sequence (like / or ]), the backslash is simply ignored by the parser.
As a result:
- '\u003c\/$1\u003e' evaluates to the string '\u003c/$1\u003e', which replaces the tag with itself.
- '\]' evaluates to the string ']', which replaces the character with itself.
To ensure a literal backslash is included in the replacement string (to escape the character for the LLM), you must use double backslashes in the string literal (e.g., '\\' to represent a single '\' in the string). Note that '\\n' is already correctly handled because '\n' is a special escape sequence for a newline, so '\\n' correctly produces a literal backslash followed by an 'n'.
| function sanitizeForWrapper(input: string): string { | |
| return input | |
| .replace(XML_CLOSING_TAG_RE, '<\\/$1>') | |
| .replace(CONTEXT_BREAKER_RE, '\\]') | |
| .replace(NEWLINE_RE, '\\n'); | |
| } | |
| function sanitizeForWrapper(input: string): string { | |
| return input | |
| .replace(XML_CLOSING_TAG_RE, '\u003c\\\\/$1\u003e') | |
| .replace(CONTEXT_BREAKER_RE, '\\\\]') | |
| .replace(NEWLINE_RE, '\\n'); | |
| } |
References
- Sanitize data from LLM-driven tools before injecting it into a system prompt to prevent prompt injection. At a minimum, remove newlines and context-breaking characters (e.g., ']').
- To prevent prompt injection, sanitize any additional context from hooks by escaping HTML-like tag characters such as
<and>.
There was a problem hiding this comment.
Code Review
This pull request introduces steering acknowledgment messages in the CLI UI and enhances prompt injection mitigation by sanitizing user input and background output. It also improves the robustness of steering acknowledgments by supporting AbortSignal for cancellation. Feedback suggests ensuring acknowledgments are only displayed if the query actually proceeds to avoid misleading feedback during quota errors, and refining the sanitization logic to remove rather than escape context-breaking characters to better align with security guidelines.
| queueMicrotask(() => { | ||
| const signal = abortControllerRef.current?.signal; | ||
| void generateSteeringAckMessage( | ||
| config.getBaseLlmClient(), | ||
| hintText, | ||
| { signal }, | ||
| ) | ||
| .then((ackText) => { | ||
| if (signal?.aborted || turnCancelledRef.current) return; | ||
| addItem( | ||
| { | ||
| type: MessageType.INFO, | ||
| icon: '· ', | ||
| color: theme.text.secondary, | ||
| marginBottom: 1, | ||
| text: ackText, | ||
| } as HistoryItemInfo, | ||
| ackTimestamp, | ||
| ); | ||
| }) | ||
| .catch((err) => { | ||
| if (err?.name === 'AbortError') return; | ||
| // Silently ignore — steering ack is non-critical UI feedback. | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The steering acknowledgment is scheduled via queueMicrotask before the check for modelSwitchedFromQuotaError. If a quota error occurs and the model is switched, submitQuery is skipped (line 2049), but the acknowledgment will still be rendered in the UI. This provides misleading feedback to the user as the hint was never actually sent to the model. Consider moving this logic after the quota error check or using a flag to ensure the acknowledgment only fires if the continuation turn actually proceeds.
| function sanitizeForWrapper(input: string): string { | ||
| return input | ||
| .replace(XML_CLOSING_TAG_RE, '<\\/$1>') | ||
| .replace(CONTEXT_BREAKER_RE, '\\]') | ||
| .replace(NEWLINE_RE, '\\n'); | ||
| } |
There was a problem hiding this comment.
The sanitizeForWrapper function currently escapes context-breaking characters like ] and newlines. However, the general rules for this repository specify that these characters should be removed rather than escaped to prevent prompt injection. Escaping might not be sufficient if the downstream LLM parser does not treat the backslash as an escape character for these specific structural elements.
| function sanitizeForWrapper(input: string): string { | |
| return input | |
| .replace(XML_CLOSING_TAG_RE, '<\\/$1>') | |
| .replace(CONTEXT_BREAKER_RE, '\\]') | |
| .replace(NEWLINE_RE, '\\n'); | |
| } | |
| function sanitizeForWrapper(input: string): string { | |
| return input | |
| .replace(XML_CLOSING_TAG_RE, '<\\/$1>') | |
| .replace(CONTEXT_BREAKER_RE, '') | |
| .replace(NEWLINE_RE, ''); | |
| } |
References
- Sanitize data from LLM-driven tools before injecting it into a system prompt to prevent prompt injection. At a minimum, remove newlines and context-breaking characters (e.g., ']').
Two PR review fixes: useGeminiStream.ts — the queueMicrotask launch sat *before* the modelSwitchedFromQuotaError early-return, so a hint that never actually got sent to the model could still produce an "Understood..." acknowledgment in the transcript. Capture the pending ack as data, then dispatch it after the early-return. fastAckHelper.ts — replace raw newlines with single spaces inside wrapper sanitization rather than escaping them. The repo prompt- injection rules favor removal over escaping for newlines and other structural breakers; spaces preserve content separation while removing the line-break structure that could escape the wrapper. The `]` context-breaker stays escaped (`\]`) since outright removal would silently change the meaning of multi-bracket content like `[step 1] [step 2]` and the escape is sufficient to neutralize bracket-based prompt-format breakouts.
|
Both addressed in 698fcc8: useGeminiStream.ts:2033 — good catch, real bug. The fastAckHelper.ts:69 — partial agreement. Newlines now stripped (replaced with single spaces) inside the wrapper sanitization, per the repo rule's "remove" guidance. Kept /gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism for generating and displaying acknowledgment messages for user steering hints, including input sanitization to prevent prompt injection and improved cancellation handling via AbortSignal. The review feedback identified a potential UI race condition where acknowledgment messages could appear out of order in the history. It is recommended to capture the acknowledgment timestamp within a microtask after the query is submitted to ensure the acknowledgment is correctly rendered after the user hint.
| let pendingSteeringAck: { | ||
| hintText: string; | ||
| ackTimestamp: number; | ||
| } | null = null; |
There was a problem hiding this comment.
The ackTimestamp should be captured inside the queueMicrotask (line 2033) rather than here. Capturing it now results in a timestamp that is less than or equal to the one generated for the user hint inside submitQuery (line 1578). This can cause the acknowledgment to be rendered above the hint in the UI history if the history manager sorts by timestamp. Removing it from the pendingSteeringAck object simplifies the state management.
let pendingSteeringAck: {
hintText: string;
} | null = null;| responsesToSend.unshift({ | ||
| text: buildUserSteeringHintPrompt(hintText), | ||
| }); | ||
| pendingSteeringAck = { hintText, ackTimestamp: Date.now() }; |
There was a problem hiding this comment.
Capturing the timestamp here is premature as it will likely be earlier than the timestamp assigned to the user hint in submitQuery.
| pendingSteeringAck = { hintText, ackTimestamp: Date.now() }; | |
| pendingSteeringAck = { hintText }; |
| } | ||
|
|
||
| if (pendingSteeringAck) { | ||
| const { hintText, ackTimestamp } = pendingSteeringAck; |
There was a problem hiding this comment.
Update the destructuring to match the simplified pendingSteeringAck object.
| const { hintText, ackTimestamp } = pendingSteeringAck; | |
| const { hintText } = pendingSteeringAck; |
| queueMicrotask(() => { | ||
| const signal = abortControllerRef.current?.signal; | ||
| void generateSteeringAckMessage(config.getBaseLlmClient(), hintText, { | ||
| signal, | ||
| }) | ||
| .then((ackText) => { | ||
| if (signal?.aborted || turnCancelledRef.current) return; | ||
| addItem( | ||
| { | ||
| type: MessageType.INFO, | ||
| icon: '· ', | ||
| color: theme.text.secondary, | ||
| marginBottom: 1, | ||
| text: ackText, | ||
| } as HistoryItemInfo, | ||
| ackTimestamp, |
There was a problem hiding this comment.
Capturing ackTimestamp here ensures it is recorded after submitQuery has executed and generated the hint's timestamp. This guarantees the acknowledgment appears after the hint in the UI history, while still appearing before the model's eventual response chunks.
queueMicrotask(() => {
const ackTimestamp = Date.now();
const signal = abortControllerRef.current?.signal;
void generateSteeringAckMessage(config.getBaseLlmClient(), hintText, {
signal,
})
.then((ackText) => {
if (signal?.aborted || turnCancelledRef.current) return;
addItem(
{
type: MessageType.INFO,
icon: '· ',
color: theme.text.secondary,
marginBottom: 1,
text: ackText,
} as HistoryItemInfo,
ackTimestamp,References
- Asynchronous operations waiting for user input via the MessageBus should rely on the provided AbortSignal for cancellation to maintain consistency with existing patterns.
- In single-threaded applications, using Date.now() is sufficient for generating unique timestamps or IDs.
The previous spot for `Date.now()` ran before submitQuery had assigned its own message timestamp, so the ack risked sorting *above* the hint it was acknowledging in a timestamp-ordered history. Move the capture inside the queueMicrotask, which runs after submitQuery's synchronous prefix; the ack now lands strictly after the hint while still ahead of any model response chunks.
|
Addressed in 1470105 — moved /gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a mechanism for acknowledging user steering hints within the CLI. Key changes include the addition of logic in the useGeminiStream hook to trigger acknowledgement messages, support for AbortSignal in the acknowledgement generation process to handle cancellations, and new sanitization logic to mitigate prompt injection by escaping XML tags and context-breaking characters. Feedback was provided regarding the sanitization of the ] character, suggesting it should be removed rather than escaped to more effectively prevent prompt injection.
| function sanitizeForWrapper(input: string): string { | ||
| return input | ||
| .replace(XML_CLOSING_TAG_RE, '<\\/$1>') | ||
| .replace(CONTEXT_BREAKER_RE, '\\]') |
There was a problem hiding this comment.
The implementation escapes the ] character instead of removing it. According to the repository rules, context-breaking characters like ] should be removed when sanitizing data from LLM-driven tools to prevent prompt injection. Escaping may not be sufficient depending on how the model parses the input.
| .replace(CONTEXT_BREAKER_RE, '\\]') | |
| .replace(CONTEXT_BREAKER_RE, '') |
References
- Sanitize data from LLM-driven tools before injecting it into a system prompt to prevent prompt injection. At a minimum, remove newlines and context-breaking characters (e.g., ']').
1 participant
Refs #26485
Summary
When a user submits a steering hint mid-turn, today the CLI silently
splices the hint into the conversation and the user gets no visible
feedback that the hint registered until the model itself acknowledges
it — which can be slow, may be omitted, or per #21508 may even leak
the internal steering instruction back to the user.
This PR adds an immediate, fast acknowledgment line rendered in the
transcript right after the hint is submitted (e.g.
· Understood. Focusing on .txt files.), generated by the existing fast-ack helperin parallel with the main turn.
Changes
packages/core/src/utils/fastAckHelper.tsgenerateSteeringAckMessageaccepts an optionalsignalso callerscan cancel the ack generation when the user aborts the active turn.
<user_input>/<background_output>wrappers to escape both XML close tags and
]context-breakers,preventing prompt-injection through these helpers.
already aborted.
abortlistener in afinallyblock to avoid leaking itacross cancelled turns.
packages/cli/src/ui/hooks/useGeminiStream.tsgenerateSteeringAckMessagein parallel with the steering hintinjection. Render the result as an info-type history item with a
captured timestamp so it lands in the correct chronological position.
the turn is cancelled before it resolves.
AbortError) since the ack isnon-critical UI feedback.
Tests
fastAckHelper.test.ts: closing-tag escaping for all threewrappers,
]escaping for hint and background output, listenercleanup verification, and immediate-fallback on a pre-aborted signal.
'src/telemetry/llmRole.js'import path sothe suite can be loaded under workspace tooling.
Prior reviewer feedback addressed
This is split out of #22619 (closed for inactivity). Every previously-
flagged issue is addressed here:
AbortSignal; capture submit timestamp (fix(cli): clear thought state on response completion to prevent UI ghosting #22619 review 3)addItemonsignal.aborted(fix(cli): clear thought state on response completion to prevent UI ghosting #22619 review 4)]and other context-breakers, not just</tag>(fix(cli): clear thought state on response completion to prevent UI ghosting #22619 review 5)abortlistener infinallyto fix leak (fix(cli): clear thought state on response completion to prevent UI ghosting #22619 review 5)Test plan
vitest run packages/core/src/utils/fastAckHelper.test.ts— 17/17 passtsc --noEmitclean for@google/gemini-cli-coreand@google/gemini-cli·acknowledgment line appears in the transcript before the model
responds, with reasonable interpretation of the hint
ghost ack line appears after cancellation
🤖 Generated with Claude Code