-
Notifications
You must be signed in to change notification settings - Fork 2.1k
feat: add session tracking for GitHub Copilot premium request optimization #7011
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 |
---|---|---|
@@ -1,5 +1,6 @@ | ||
import { Anthropic } from "@anthropic-ai/sdk" | ||
import * as vscode from "vscode" | ||
import * as crypto from "crypto" | ||
|
||
import { type ModelInfo, openAiModelInfoSaneDefaults } from "@roo-code/types" | ||
|
||
|
@@ -44,13 +45,17 @@ export class VsCodeLmHandler extends BaseProvider implements SingleCompletionHan | |
private client: vscode.LanguageModelChat | null | ||
private disposable: vscode.Disposable | null | ||
private currentRequestCancellation: vscode.CancellationTokenSource | null | ||
private sessionMessageCount: Map<string, number> = new Map() | ||
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. Memory Leak Risk: The sessionMessageCount Map could grow indefinitely if sessions are started but never properly ended. Consider implementing: (1) A maximum session limit, (2) TTL-based cleanup for stale sessions, or (3) Periodic cleanup of old sessions. |
||
private currentSessionId: string | null = null | ||
|
||
constructor(options: ApiHandlerOptions) { | ||
super() | ||
this.options = options | ||
this.client = null | ||
this.disposable = null | ||
this.currentRequestCancellation = null | ||
this.sessionMessageCount = new Map() | ||
this.currentSessionId = null | ||
|
||
try { | ||
// Listen for model changes and reset client | ||
|
@@ -165,6 +170,9 @@ export class VsCodeLmHandler extends BaseProvider implements SingleCompletionHan | |
* Tool calls handling is currently a work in progress. | ||
*/ | ||
dispose(): void { | ||
// End any active session | ||
this.endSession() | ||
|
||
if (this.disposable) { | ||
this.disposable.dispose() | ||
} | ||
|
@@ -330,6 +338,52 @@ export class VsCodeLmHandler extends BaseProvider implements SingleCompletionHan | |
return content | ||
} | ||
|
||
/** | ||
* Start a new conversation session | ||
* @param sessionId - Optional session ID, will generate one if not provided | ||
* @returns The session ID being used | ||
*/ | ||
public startSession(sessionId?: string): string { | ||
const id = sessionId || crypto.randomUUID() | ||
this.currentSessionId = id | ||
this.sessionMessageCount.set(id, 0) | ||
console.debug(`Roo Code <Language Model API>: Started new session ${id}`) | ||
return id | ||
} | ||
|
||
/** | ||
* End the current conversation session | ||
*/ | ||
public endSession(): void { | ||
if (this.currentSessionId) { | ||
const messageCount = this.sessionMessageCount.get(this.currentSessionId) || 0 | ||
console.debug( | ||
`Roo Code <Language Model API>: Ended session ${this.currentSessionId} with ${messageCount} messages`, | ||
) | ||
this.sessionMessageCount.delete(this.currentSessionId) | ||
this.currentSessionId = null | ||
} | ||
} | ||
|
||
/** | ||
* Get the current message count for the active session | ||
* @returns The number of messages in the current session, or 0 if no session | ||
*/ | ||
public getSessionMessageCount(): number { | ||
if (!this.currentSessionId) { | ||
return 0 | ||
} | ||
return this.sessionMessageCount.get(this.currentSessionId) || 0 | ||
} | ||
|
||
/** | ||
* Check if this is the first message in the current session | ||
* @returns true if this is the first message or no session exists | ||
*/ | ||
private isFirstMessage(): boolean { | ||
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. The private method 'isFirstMessage' is never used. Consider removing it to reduce dead code. 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. This isFirstMessage() method is defined but never used. Was this intended to help determine when to set different header values? Consider either using this method in the message creation logic or removing it if it's not needed. |
||
return this.getSessionMessageCount() === 0 | ||
} | ||
|
||
override async *createMessage( | ||
systemPrompt: string, | ||
messages: Anthropic.Messages.MessageParam[], | ||
|
@@ -339,6 +393,26 @@ export class VsCodeLmHandler extends BaseProvider implements SingleCompletionHan | |
this.ensureCleanState() | ||
const client: vscode.LanguageModelChat = await this.getClient() | ||
|
||
// Track session from metadata if available | ||
if (metadata?.taskId) { | ||
// Use taskId as session identifier | ||
if (!this.currentSessionId || this.currentSessionId !== metadata.taskId) { | ||
this.startSession(metadata.taskId) | ||
} | ||
} | ||
|
||
// Increment message count for the current session | ||
if (this.currentSessionId) { | ||
const currentCount = this.sessionMessageCount.get(this.currentSessionId) || 0 | ||
this.sessionMessageCount.set(this.currentSessionId, currentCount + 1) | ||
|
||
// Log session tracking for debugging | ||
const isFirst = currentCount === 0 | ||
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. Critical Issue: This implementation tracks sessions but doesn't actually use the information to optimize premium request consumption. The issue #7010 specifically mentions using X-Initiator headers with values of 'user' for first messages and 'agent' for subsequent ones. While the VS Code LM API may not expose HTTP headers directly, the session tracking alone doesn't solve the problem described in the issue. Is there a way to leverage this session information to actually reduce premium request usage, or should we document this limitation more clearly? |
||
console.debug( | ||
`Roo Code <Language Model API>: Session ${this.currentSessionId} - Message ${currentCount + 1} (First: ${isFirst})`, | ||
) | ||
} | ||
|
||
// Process messages | ||
const cleanedMessages = messages.map((msg) => ({ | ||
...msg, | ||
|
@@ -366,8 +440,9 @@ export class VsCodeLmHandler extends BaseProvider implements SingleCompletionHan | |
justification: `Roo Code would like to use '${client.name}' from '${client.vendor}', Click 'Allow' to proceed.`, | ||
} | ||
|
||
// Note: Tool support is currently provided by the VSCode Language Model API directly | ||
// Extensions can register tools using vscode.lm.registerTool() | ||
// Note: While we can't directly set X-Initiator headers through the VS Code API, | ||
// we track session state to understand usage patterns. The VS Code extension | ||
// host manages the actual GitHub Copilot API communication internally. | ||
|
||
const response: vscode.LanguageModelChatResponse = await client.sendRequest( | ||
vsCodeLmMessages, | ||
|
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.
The tests are comprehensive but don't cover: (1) Session timeout/cleanup scenarios, (2) Maximum session limit handling, or (3) Concurrent session edge cases. Consider adding tests for these scenarios to ensure robustness.