fix(hooks): fix BeforeAgent/AfterAgent inconsistencies (#18514) - #21383
Conversation
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 resolves several inconsistencies in the BeforeAgent and AfterAgent hooks within the Highlights
Changelog
Activity
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request addresses several issues. The main fix resolves inconsistencies with BeforeAgent/AfterAgent hooks during recursive calls in sendMessageStream by improving state management with a continuationHandled flag and resetting hook state appropriately. This is a good, targeted fix for the described problem. The PR also includes two other unrelated changes: a feature to allow escaping @ characters in the input prompt to treat them as literals, and a fix to ensure the chat transcript is flushed even for model responses that only contain tool calls or thoughts.
However, the changes related to handling @ characters introduce a critical regression. By automatically escaping @ on paste, it's no longer possible to paste and execute at-commands, which is a core feature. I've left specific comments on how to address this, referencing the rule about maintaining UI consistency.
While all these changes are valuable, bundling unrelated fixes and features into a single pull request goes against the repository's guideline to keep PRs small and focused (line 67). This makes the changes harder to review and understand. In the future, please submit distinct changes in separate pull requests.
4160e8c to
85b0a55
Compare
|
Cleaned up - the PR now touches only packages/core/src/core/client.ts. The paste-related files (InputPrompt.tsx, atCommandProcessor.ts, unescapeLiteralAt.ts) were accidentally bundled from a prior branch and have been removed. The previous bot feedback on those files is now outdated. |
17dab9f to
8983717
Compare
spencer426
left a comment
There was a problem hiding this comment.
Thanks for the PR! The core logic fix for the hook states looks good, but it appears your branch is out of sync with main and is actively reverting some very recent changes. Could you please rebase or restore these missing lines?
|
|
||
| constructor(private readonly context: AgentLoopContext) { | ||
| this.loopDetector = new LoopDetectionService(this.config); | ||
| constructor(private readonly config: Config) { |
There was a problem hiding this comment.
It looks like this reverts the recent introduction of AgentLoopContext. This constructor should still accept private readonly context: AgentLoopContext instead of falling back to just Config.
| } | ||
|
|
||
| getHistory(): readonly Content[] { | ||
| getHistory(): Content[] { |
There was a problem hiding this comment.
This reverts the method signature by removing the readonly modifier. Please keep readonly Content[] to preserve type safety.
| } | ||
|
|
||
| setHistory(history: readonly Content[]) { | ||
| setHistory(history: Content[]) { |
There was a problem hiding this comment.
Same here, please keep the readonly modifier in the parameter signature: setHistory(history: readonly Content[]).
| authType: this.config.getContentGeneratorConfig()?.authType, | ||
| maxAttempts: availabilityMaxAttempts, | ||
| retryFetchErrors: this.config.getRetryFetchErrors(), | ||
| getAvailabilityContext, |
There was a problem hiding this comment.
This unintentionally reverts PR #22027 by removing retryFetchErrors and onRetry from the generateContentWithRetry call, which are needed for network retry telemetry. Please restore these properties.
34f5855 to
f6acb81
Compare
|
@spencer426 all four points addressed, AgentLoopContext constructor, readonly Content[] on getHistory/setHistory/tryMaskToolOutputs, retryFetchErrors + onRetry from #22027, and this.context.toolRegistry at all 3 call sites. Single commit on top of upstream/main, 79 tests passing, preflight green. |
f6acb81 to
0d9a921
Compare
…#18514) (google-gemini#21383) Co-authored-by: Spencer <spencertang@google.com>
…#18514) (google-gemini#21383) Co-authored-by: Spencer <spencertang@google.com>
…#18514) (google-gemini#21383) Co-authored-by: Spencer <spencertang@google.com>
…#18514) (google-gemini#21383) Co-authored-by: Spencer <spencertang@google.com>
Fixes #18514
Root Cause
All three bugs trace to hook state management in
sendMessageStreaminpackages/core/src/core/client.ts.When
isBlockingDecision()fires and triggers a recursivesendMessageStreamcall with the sameprompt_id:BeforeAgent not firing on injected prompt,
hasFiredBeforeAgentis stilltruefrom the prior turn. The guard atfireBeforeAgentHookSafereturns early, silently skipping BeforeAgent for the injected prompt.AfterAgent not firing after text-only continuation, the recursive call increments
activeCallsto 2.fireAfterAgentHookSafeguards onactiveCalls !== 1and returnsundefined, so AfterAgent never fires for that turn.BeforeAgent firing spuriously after first tool call — the
finallyblock checkspendingToolCallson the outer turn (not the continuation turn), which can prematurely delete the hookState mid-sequence. The next call creates a fresh hookState withhasFiredBeforeAgent=false, causing an unexpected BeforeAgent fire.Fix
Three surgical changes, all in
sendMessageStream:sendMessageStreamcall in theisBlockingDecisionbranch: resethasFiredBeforeAgent = falseand decrementactiveCallson the hookState, so the continuation is treated as a fresh outermost turn.continuationHandledflag to prevent thefinallyblock from double-decrementingactiveCallswhen the continuation has already managed its own state.Testing
npx vitest run packages/core/src/core/client.test.ts)npm run preflight) - 0 errors