-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix(memory): avoid stale tool schema recall #5058
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -128,6 +128,7 @@ import { type File, type IdeContext } from '../ide/types.js'; | |
| import { PermissionMode, type StopHookOutput } from '../hooks/types.js'; | ||
|
|
||
| const MAX_TURNS = 100; | ||
| const MAX_RECENT_TOOL_NAMES_FOR_MEMORY = 20; | ||
|
|
||
| export enum SendMessageType { | ||
| UserQuery = 'userQuery', | ||
|
|
@@ -211,6 +212,7 @@ export class GeminiClient { | |
| private lastPromptId: string | undefined = undefined; | ||
| private lastSentIdeContext: IdeContext | undefined; | ||
| private forceFullIdeContext = true; | ||
| private recentCompletedToolNames: string[] = []; | ||
| private pendingMemoryPrefetch: MemoryPrefetchHandle | undefined; | ||
| private lastSessionStartContext: string | undefined; | ||
| private lastSessionStartSource: SessionStartSource | undefined; | ||
|
|
@@ -286,12 +288,16 @@ export class GeminiClient { | |
| // Check if we're resuming from a previous session | ||
| const resumedSessionData = this.config.getResumedSessionData(); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] When a session is resumed ( Consider walking — qwen3.7-max via Qwen Code /review |
||
| if (resumedSessionData) { | ||
| replayUiTelemetryFromConversation(resumedSessionData.conversation, this.config.getSessionId()); | ||
| replayUiTelemetryFromConversation( | ||
| resumedSessionData.conversation, | ||
| this.config.getSessionId(), | ||
| ); | ||
| // Convert resumed session to API history format | ||
| // Each ChatRecord's message field is already a Content object | ||
| const resumedHistory = buildApiHistoryFromConversation( | ||
| resumedSessionData.conversation, | ||
| ); | ||
| this.seedRecentCompletedToolNamesFromHistory(resumedHistory); | ||
| await this.startChat( | ||
| resumedHistory, | ||
| sessionStartSource ?? SessionStartSource.Resume, | ||
|
|
@@ -636,6 +642,7 @@ export class GeminiClient { | |
| this.cachedGitStatus = undefined; | ||
| this.lastApiCompletionTimestamp = null; | ||
| this.lastHookMicrocompactionTimestamp = null; | ||
| this.recentCompletedToolNames = []; | ||
| // startChat() rewrites the chat to its initial state. Any prior | ||
| // read_file tool results the FileReadCache still tracks are no | ||
| // longer in history, so a follow-up Read would serve a placeholder | ||
|
|
@@ -1494,6 +1501,8 @@ export class GeminiClient { | |
| toolName: string, | ||
| args?: Record<string, unknown>, | ||
| ): void { | ||
| this.rememberCompletedToolName(toolName); | ||
|
|
||
| if (args && SKILL_WRITE_TOOL_NAMES.has(toolName)) { | ||
| const filePath = args['file_path'] ?? args['path'] ?? args['target_file']; | ||
| if ( | ||
|
|
@@ -1506,6 +1515,45 @@ export class GeminiClient { | |
| this.toolCallCount += 1; | ||
| } | ||
|
|
||
| private rememberCompletedToolName(toolName: string): void { | ||
| const normalizedToolName = toolName.trim(); | ||
| if (!normalizedToolName) { | ||
| return; | ||
| } | ||
| this.recentCompletedToolNames = [ | ||
| ...this.recentCompletedToolNames.filter( | ||
| (name) => name !== normalizedToolName, | ||
| ), | ||
| normalizedToolName, | ||
| ].slice(-MAX_RECENT_TOOL_NAMES_FOR_MEMORY); | ||
| } | ||
|
|
||
| private seedRecentCompletedToolNamesFromHistory(history: Content[]): void { | ||
| const completedCallIds = new Set<string>(); | ||
| for (const message of history) { | ||
| for (const part of message.parts ?? []) { | ||
| const responseId = part.functionResponse?.id; | ||
| if (responseId) { | ||
| completedCallIds.add(responseId); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| this.recentCompletedToolNames = []; | ||
| for (const message of history) { | ||
| for (const part of message.parts ?? []) { | ||
| const call = part.functionCall; | ||
| if (!call?.name) { | ||
| continue; | ||
| } | ||
| if (call.id && !completedCallIds.has(call.id)) { | ||
| continue; | ||
| } | ||
| this.rememberCompletedToolName(call.name); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private async microcompactIdleHistory( | ||
| lastCompletionTimestamp: number | null, | ||
| ): Promise<boolean> { | ||
|
|
@@ -1692,6 +1740,7 @@ export class GeminiClient { | |
| .recall(this.config.getProjectRoot(), partToString(request), { | ||
| config: this.config, | ||
| excludedFilePaths: this.surfacedRelevantAutoMemoryPaths, | ||
| recentTools: [...this.recentCompletedToolNames], | ||
| abortSignal: controller.signal, | ||
| }) | ||
| .catch((error: unknown) => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion]
resetChat()(line ~628) does not clearrecentCompletedToolNames. Every other piece of per-conversation state is explicitly reset there (surfacedRelevantAutoMemoryPaths,cachedGitStatus,lastApiCompletionTimestamp, file read cache, deferred tools, etc.), but this new field was omitted.After a
/clear, stale tool names from the previous conversation persist and continue to drive theisActiveToolUsageMemoryfilter — ephemeral tool-schema memories for tools used in the old conversation will be incorrectly suppressed in the new one.— qwen3.7-max via Qwen Code /review