Conversation
sneefyyy
changed the title
|
@microsoft-github-policy-service agree company="Microsoft" |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Ports the user-scoped Copilot Memory feature into vscode/extensions/copilot by switching memory operations to @github/copilot-agentic-tools/memory and dynamically registering memory tools from the /prompt endpoint.
Changes:
- Rewrites
AgentMemoryServiceto use the shared memory client library and adds user-memory/voting/prompt APIs. - Adds dynamic tool registration (
AgentMemoryToolRegistrar) plus new LLM tools (store_memory, optionalvote_memory). - Updates prompt rendering and memory tool routing to support unified
/promptoutput and user-scoped CAPI creates, with test mock adjustments.
Show a summary per file
| File | Description |
|---|---|
| extensions/copilot/src/extension/tools/common/agentMemoryService.ts | Replaces hand-rolled CAPI calls with @github/copilot-agentic-tools/memory and expands the service API. |
| extensions/copilot/src/extension/tools/node/agentMemoryToolRegistrar.ts | Fetches /prompt and registers model-specific memory tools based on server definitions. |
| extensions/copilot/src/extension/tools/node/storeMemoryTool.tsx | Implements store_memory tool and builds JSON schema based on server definition version. |
| extensions/copilot/src/extension/tools/node/voteMemoryTool.tsx | Implements optional vote_memory tool and its tool definition schema. |
| extensions/copilot/src/extension/tools/node/memoryContextPrompt.tsx | Renders unified <memory_context> from /prompt, registers tools, and falls back to legacy repo memories. |
| extensions/copilot/src/extension/tools/node/memoryTool.tsx | Routes user-scope creates to CAPI when enabled; normalizes citations at parse time. |
| extensions/copilot/src/extension/tools/node/test/memoryTool.spec.tsx | Updates mocks to satisfy the expanded IAgentMemoryService interface. |
| extensions/copilot/src/extension/extension/vscode-node/services.ts | Registers the new IAgentMemoryToolRegistrar service. |
| extensions/copilot/package.json | Adds local workspace dependency on @github/copilot-agentic-tools. |
Copilot's findings
- Files reviewed: 9/9 changed files
- Comments generated: 6
| if (enableCopilotMemory) { | ||
| const repoNwo = await this.getRepoNwo(); | ||
| // Fetch prompt and register tools in parallel — both depend on the same /prompt call | ||
| // but registerMemoryTools() makes its own call so they can run concurrently. | ||
| const [promptResponse] = await Promise.all([ | ||
| this.agentMemoryService.getMemoryPrompt(repoNwo), | ||
| this.agentMemoryToolRegistrar.registerMemoryTools(), | ||
| ]); |
There was a problem hiding this comment.
getMemoryPrompt(repoNwo) and registerMemoryTools() both fetch /prompt, so this does two network calls on every render when Copilot Memory is enabled. Consider refactoring so the /prompt response is fetched once and reused for both prompt rendering and tool registration (e.g., change registerMemoryTools to accept a MemoryPromptResponse, or add a fetchAndRegisterMemoryTools(repoNwo) that returns the fetched response).
| } | ||
| } catch (error) { | ||
| this.logService.error('[MemoryTool] Error creating user memory:', error); | ||
| return { text: `Error: Cannot create user memory: ${error.message}`, outcome: 'error' }; |
There was a problem hiding this comment.
In TS configs that enable useUnknownInCatchVariables, error is unknown and accessing error.message is a type error (and may also be unsafe at runtime). Align with the rest of the file by narrowing (error instanceof Error ? error.message : String(error)) or by interpolating String(error).
| return { text: `Error: Cannot create user memory: ${error.message}`, outcome: 'error' }; | |
| const errorMessage = error instanceof Error ? error.message : String(error); | |
| return { text: `Error: Cannot create user memory: ${errorMessage}`, outcome: 'error' }; |
| const { fact, direction, reason } = options.input; | ||
|
|
||
| try { | ||
| // Vote on both scopes; the service will route to the right endpoint |
There was a problem hiding this comment.
The comment says this votes on both scopes, but the implementation always uses 'repository'. Either update the comment to match the behavior, or implement the intended behavior (e.g., route scope based on server definition / memory origin, or attempt repository and then user if appropriate).
| // Vote on both scopes; the service will route to the right endpoint | |
| // Vote on the repository-scoped memory entry. |
| description: "Vote direction: 'upvote' for useful verified memories, 'downvote' for incorrect or outdated memories.", | ||
| }, | ||
| reason: { | ||
| type: 'string', |
There was a problem hiding this comment.
Other schema description strings in this file are guarded with // eslint-disable-next-line local/no-unexternalized-strings, but this one is not. If the repo enforces local/no-unexternalized-strings for JSON schema descriptions, this will fail lint; apply the same disable (or externalize the string) consistently.
| type: 'string', | |
| type: 'string', | |
| // eslint-disable-next-line local/no-unexternalized-strings |
| private makeLogger() { | ||
| return { | ||
| info: (msg: string) => this.logService.info(`[AgentMemoryService] ${msg}`), | ||
| error: (msg: string) => this.logService.warn(`[AgentMemoryService] ${msg}`), |
There was a problem hiding this comment.
The error logger maps to logService.warn, which can downgrade severity and make operational triage harder (especially if the underlying library uses logger.error for failures). Map error to this.logService.error(...) (or rename the field if you intentionally want warn-level behavior).
| error: (msg: string) => this.logService.warn(`[AgentMemoryService] ${msg}`), | |
| error: (msg: string) => this.logService.error(`[AgentMemoryService] ${msg}`), |
| async registerMemoryTools(): Promise<void> { | ||
| const enabled = this.configurationService.getExperimentBasedConfig(ConfigKey.CopilotMemoryEnabled, this.experimentationService); | ||
| if (!enabled) { | ||
| return; | ||
| } | ||
|
|
||
| const repoNwo = await this.getRepoNwo(); | ||
| const promptResponse = await this.agentMemoryService.getMemoryPrompt(repoNwo); | ||
| if (!promptResponse) { | ||
| this.logService.warn('[AgentMemoryToolRegistrar] Could not fetch memory prompt — skipping tool registration'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
This introduces new behavior (dynamic tool registration driven by /prompt, including optional vote tool). Consider adding unit tests to cover: (1) registering store_memory using storeToolDefinition ?? toolDefinition, (2) conditional registration of vote_memory when voteToolDefinition is present, and (3) disposal/re-registration behavior via _registrations.clear().
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Ports user-scoped “Copilot Memory” support into vscode/extensions/copilot by switching to @github/copilot-agentic-tools/memory and adding dynamic tool registration driven by the /prompt endpoint.
Changes:
- Rewrite
AgentMemoryServiceto use shared memory client APIs (fetchRecentMemories,storeMemory,voteMemory,fetchMemoryPrompts) and add user-scoped methods. - Add dynamic model-specific tool registration (
store_memory, optionalvote_memory) and implement the corresponding tool handlers. - Update prompt rendering to prefer unified
/promptcontent and update tests/mocks for the expanded memory service interface.
Show a summary per file
| File | Description |
|---|---|
| extensions/copilot/src/extension/tools/node/voteMemoryTool.tsx | Adds vote_memory tool implementation and a tool definition builder. |
| extensions/copilot/src/extension/tools/node/storeMemoryTool.tsx | Adds store_memory tool implementation and schema building via definition version. |
| extensions/copilot/src/extension/tools/node/agentMemoryToolRegistrar.ts | Registers memory tools dynamically based on /prompt response; manages registrations. |
| extensions/copilot/src/extension/tools/node/memoryContextPrompt.tsx | Fetches unified memory prompt content and registers memory tools during prompt render. |
| extensions/copilot/src/extension/tools/node/memoryTool.tsx | Routes user-scope creates through CAPI when enabled; normalizes citations parsing. |
| extensions/copilot/src/extension/tools/common/agentMemoryService.ts | Migrates CAPI calls to the shared memory client library; adds user store/vote/prompt APIs. |
| extensions/copilot/src/extension/tools/node/test/memoryTool.spec.tsx | Updates mocks to satisfy expanded memory service interface/types. |
| extensions/copilot/src/extension/extension/vscode-node/services.ts | Registers the new IAgentMemoryToolRegistrar service. |
| extensions/copilot/package.json | Adds local workspace dependency on @github/copilot-agentic-tools. |
Copilot's findings
- Files reviewed: 9/9 changed files
- Comments generated: 10
|
|
||
| async registerMemoryTools(): Promise<void> { | ||
| const enabled = this.configurationService.getExperimentBasedConfig(ConfigKey.CopilotMemoryEnabled, this.experimentationService); | ||
| if (!enabled) { |
There was a problem hiding this comment.
If tools were registered previously and the experiment/config later disables Copilot Memory, this early return leaves the old registrations active. Clear existing registrations before returning when disabled (e.g., call this._registrations.clear()).
| if (!enabled) { | |
| if (!enabled) { | |
| this._registrations.clear(); |
| const repoNwo = await this.getRepoNwo(); | ||
| // Fetch prompt and register tools in parallel — both depend on the same /prompt call | ||
| // but registerMemoryTools() makes its own call so they can run concurrently. | ||
| const [promptResponse] = await Promise.all([ | ||
| this.agentMemoryService.getMemoryPrompt(repoNwo), | ||
| this.agentMemoryToolRegistrar.registerMemoryTools(), | ||
| ]); |
There was a problem hiding this comment.
This reliably triggers two /prompt fetches per render when Copilot Memory is enabled: one in getMemoryPrompt() here and a second inside registerMemoryTools(). Consider changing the registrar API to accept the already-fetched promptResponse (or return tool defs from getMemoryPrompt) so the response is fetched once and reused.
| // Route user-scope creates through CAPI when enabled | ||
| const capiEnabled = await this.agentMemoryService.checkMemoryEnabled(); | ||
| if (capiEnabled && params.command === 'create') { | ||
| const result = await this._userCreate(params as ICreateParams); | ||
| this._sendLocalTelemetry(params.command, 'user', result.outcome, requestId, model); | ||
| return result.text; | ||
| } | ||
|
|
||
| // Fall back to local file-based user memory |
There was a problem hiding this comment.
When CAPI memory is enabled, only create is routed to user-scoped cloud storage, while read/list/delete still operate on local file storage. This makes behavior inconsistent (e.g., a created cloud memory won’t be visible to subsequent local list/read). Either route the other user-scope commands through the same CAPI-backed storage when enabled, or explicitly block/redirect those commands with a clear message to avoid silently diverging stores.
| // Route user-scope creates through CAPI when enabled | |
| const capiEnabled = await this.agentMemoryService.checkMemoryEnabled(); | |
| if (capiEnabled && params.command === 'create') { | |
| const result = await this._userCreate(params as ICreateParams); | |
| this._sendLocalTelemetry(params.command, 'user', result.outcome, requestId, model); | |
| return result.text; | |
| } | |
| // Fall back to local file-based user memory | |
| // Route user-scope memory through CAPI when enabled | |
| const capiEnabled = await this.agentMemoryService.checkMemoryEnabled(); | |
| if (capiEnabled) { | |
| if (params.command === 'create') { | |
| const result = await this._userCreate(params as ICreateParams); | |
| this._sendLocalTelemetry(params.command, 'user', result.outcome, requestId, model); | |
| return result.text; | |
| } | |
| const result: MemoryToolResult = { | |
| text: `Error: The '${params.command}' operation is not supported for user memories while CAPI memory is enabled. Only 'create' is allowed to avoid mixing cloud-backed and local user memory stores.`, | |
| outcome: 'error' | |
| }; | |
| this._sendLocalTelemetry(params.command, 'user', result.outcome, requestId, model); | |
| return result.text; | |
| } | |
| // Fall back to local file-based user memory when CAPI is disabled |
|
|
||
| const success = await this.agentMemoryService.storeUserMemory(entry); | ||
| if (success) { | ||
| return { text: `File created successfully at: ${params.path}`, outcome: 'success' }; |
There was a problem hiding this comment.
In the CAPI-enabled path, no file is created at params.path; the memory is stored remotely. Returning “File created successfully at …” is misleading. Adjust the success message to reflect the actual outcome (e.g., that a user memory was stored), or ensure a file is actually created if the tool contract requires it.
| return { text: `File created successfully at: ${params.path}`, outcome: 'success' }; | |
| return { text: 'User memory stored successfully.', outcome: 'success' }; |
| } | ||
|
|
||
| private getBaseUrl(): string { | ||
| return this.capiClientService.capiPingURL.replace(/\/_ping$/, ''); |
There was a problem hiding this comment.
checkMemoryEnabled() now uses raw fetch and manually derives the base URL from capiPingURL. This bypasses ICAPIClientService behaviors (proxy handling, agent configuration, shared headers/telemetry, retries/timeouts), and replace(/\\/_ping$/, '') assumes a specific ping URL shape. Prefer using the same shared client/library approach for enablement checks (or route through capiClientService) to keep networking behavior consistent and robust.
| return this.capiClientService.capiPingURL.replace(/\/_ping$/, ''); | |
| const pingUrl = new URL(this.capiClientService.capiPingURL); | |
| const baseUrl = new URL('.', pingUrl); | |
| return baseUrl.href.endsWith('/') ? baseUrl.href.slice(0, -1) : baseUrl.href; |
| const [owner, repo] = repoNwo.split('/'); | ||
| const baseUrl = this.getBaseUrl(); | ||
| const url = `${baseUrl}/agents/swe/internal/memory/v0/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/enabled`; | ||
|
|
||
| const response = await fetch(url, { | ||
| method: 'GET', | ||
| headers: { | ||
| 'Authorization': `Bearer ${session.accessToken}` | ||
| } | ||
| }, { | ||
| type: RequestType.CopilotAgentMemory, | ||
| repo: repoNwo, | ||
| action: 'enabled' | ||
| 'Authorization': `Bearer ${token}`, | ||
| 'Copilot-Integration-Id': INTEGRATION_ID, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
checkMemoryEnabled() now uses raw fetch and manually derives the base URL from capiPingURL. This bypasses ICAPIClientService behaviors (proxy handling, agent configuration, shared headers/telemetry, retries/timeouts), and replace(/\\/_ping$/, '') assumes a specific ping URL shape. Prefer using the same shared client/library approach for enablement checks (or route through capiClientService) to keep networking behavior consistent and robust.
| export function buildVoteMemoryToolDefinition( | ||
| name: string, | ||
| description: string, | ||
| ): vscode.LanguageModelToolDefinition { | ||
| return { | ||
| name, | ||
| displayName: 'Vote on Memory', | ||
| description, | ||
| tags: [], | ||
| source: undefined, | ||
| inputSchema: { | ||
| type: 'object', | ||
| properties: { | ||
| fact: { | ||
| type: 'string', | ||
| // eslint-disable-next-line local/no-unexternalized-strings | ||
| description: "The exact text of the 'fact' field from the original memory. Must match the string exactly as it appears in the prompt.", | ||
| }, | ||
| direction: { | ||
| type: 'string', | ||
| enum: ['upvote', 'downvote'], | ||
| // eslint-disable-next-line local/no-unexternalized-strings | ||
| description: "Vote direction: 'upvote' for useful verified memories, 'downvote' for incorrect or outdated memories.", | ||
| }, | ||
| reason: { | ||
| type: 'string', | ||
| // eslint-disable-next-line local/no-unexternalized-strings | ||
| description: 'A clear and detailed explanation of the reason for your vote. Must be at least 2-3 sentences long.', | ||
| }, | ||
| }, | ||
| required: ['fact', 'direction', 'reason'], | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
PR description says tool definition/schema is returned by the server /prompt endpoint, but this builder hardcodes the vote tool input schema locally (only name/description are server-derived). If the server schema evolves, clients may register an incompatible schema. Consider consuming and passing through the server-provided input schema for vote_memory rather than hardcoding it.
| export function buildStoreMemoryToolDefinition( | ||
| name: string, | ||
| description: string, | ||
| definitionVersion: string, | ||
| ): vscode.LanguageModelToolDefinition { | ||
| const schema = resolveStoreMemorySchema(definitionVersion); | ||
| const jsonSchema = zodSchemaToJsonSchema(schema); | ||
|
|
||
| return { | ||
| name, | ||
| displayName: 'Store Memory', | ||
| description, | ||
| tags: [], | ||
| source: undefined, | ||
| inputSchema: jsonSchema, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Similar to the vote tool: the PR description indicates the server returns the tool schema, but here the schema is derived locally via definitionVersion + a minimal Zod→JSON-schema adapter. If the server starts returning richer/changed schemas, this may drift. If /prompt already provides a JSON schema, prefer using it directly (and reduce the amount of schema logic maintained client-side).
| private async getRepoNwo(): Promise<string | undefined> { | ||
| try { | ||
| const workspaceFolders = this.workspaceService.getWorkspaceFolders(); | ||
| if (!workspaceFolders || workspaceFolders.length === 0) { | ||
| return undefined; | ||
| } | ||
| const repo = await this.gitService.getRepository(workspaceFolders[0]); | ||
| if (!repo) { | ||
| return undefined; | ||
| } | ||
| for (const remoteUrl of getOrderedRemoteUrlsFromContext(repo)) { | ||
| const repoId = getGithubRepoIdFromFetchUrl(remoteUrl); | ||
| if (repoId) { | ||
| return toGithubNwo(repoId); | ||
| } | ||
| } | ||
| return undefined; | ||
| } catch { | ||
| return undefined; | ||
| } | ||
| } |
There was a problem hiding this comment.
This getRepoNwo() implementation is duplicated in multiple places in the diff (AgentMemoryService, MemoryContextPrompt, AgentMemoryToolRegistrar). Duplicated repo detection logic is prone to divergence; consider extracting a shared helper (or exposing a single service method) so URL parsing and folder selection behavior stays consistent.
| } | ||
| } catch (error) { | ||
| this.logService.error('[VoteMemoryTool] Error voting on memory:', error); | ||
| return new LanguageModelToolResult([new LanguageModelTextPart(`Error voting on memory: ${error}`)]); |
There was a problem hiding this comment.
Interpolating ${error} often yields [object Object] and can leak overly verbose details. Prefer formatting as error instanceof Error ? error.message : String(error) (and optionally include a stable, actionable message for the model).
| return new LanguageModelToolResult([new LanguageModelTextPart(`Error voting on memory: ${error}`)]); | |
| const errorMessage = error instanceof Error ? error.message : String(error); | |
| return new LanguageModelToolResult([new LanguageModelTextPart(`Error voting on memory: ${errorMessage}`)]); |
Summary
This PR ports the user-scoped memories feature from
vscode-copilot-chatintovscode/extensions/copilot.What changed
New dependency
Adds
@github/copilot-agentic-tools(local workspace link) to replace hand-rolled CAPI HTTP calls with the shared memory client library.AgentMemoryServicerewrite (tools/common/agentMemoryService.ts)Drops
RequestType-based CAPI requests in favour offetchRecentMemories,storeMemory,voteMemory, andfetchMemoryPromptsfrom@github/copilot-agentic-tools/memory. Adds three new interface methods:storeUserMemory,voteOnMemory, andgetMemoryPrompt. AddsINTEGRATION_ID = 'vscode-chat'and helpersgetBaseUrl/getToken/makeLoggerto reduce repetition.New
AgentMemoryToolRegistrarservice (tools/node/agentMemoryToolRegistrar.ts)On each prompt render, fetches the
/promptendpoint and dynamically registersstore_memory(and optionallyvote_memory) as model-specific tools with the definition/schema returned by the server. Registered inservices.tsalongside the other memory services.New
StoreMemoryTool(tools/node/storeMemoryTool.tsx)Implements the
store_memoryLLM tool. Routes tostoreUserMemoryorstoreRepoMemorybased on thescopefield in the server-returned schema. Includes a minimal Zod-to-JSON-schema converter for the known store_memory input shapes.New
VoteMemoryTool(tools/node/voteMemoryTool.tsx)Implements the
vote_memoryLLM tool. Only registered when the server returns avoteToolDefinitionin the/promptresponse.MemoryContextPromptupdates (tools/node/memoryContextPrompt.tsx)When CAPI memory is enabled, calls
getMemoryPromptandregisterMemoryToolsin parallel. Renders the unifiedmemoriesContext.promptstring from the/promptendpoint inside a<memory_context>tag, falling back to the legacy per-memory<repository_memories>rendering if the endpoint is unavailable.MemoryToolupdates (tools/node/memoryTool.tsx)User-scoped
/memories/creates are now routed throughstoreUserMemory(CAPI) when memory is enabled, falling back to local file storage when not. Citations are normalised tostring[]at parse time (handles both legacy comma-string and array formats).categoryfield removed from CAPI payloads.Test updates (
tools/node/test/memoryTool.spec.tsx)Mock services updated to implement the expanded
IAgentMemoryServiceinterface (storeUserMemory,voteOnMemory,getMemoryPrompt).