Implementing user-scoped memory#309942
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
| 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 |
There was a problem hiding this comment.
look into why the local one even exists
There was a problem hiding this comment.
and make sure it already existed before
|
|
||
| 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; |
There was a problem hiding this comment.
implement this change, but also change the package
| 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.
There was a problem hiding this comment.
perhaps outdated, check to see if it uses agentic tools
There was a problem hiding this comment.
The server's /prompt response (ToolDefinition) only provides name, description, and definitionVersion. It doesn't include an input schema. The input schema here mirrors voteMemoryInputSchema from @github/copilot-agentic-tools/memory, which is the source of truth. The duplication is a known trade-off since VS Code's LanguageModelToolDefinition.inputSchema requires a plain JSON Schema object and we don't have zod-to-json-schema available to derive it automatically.
| 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).
There was a problem hiding this comment.
why are we converting zod schemas to json?
There was a problem hiding this comment.
Same thing as above, needs to be in JSON
There was a problem hiding this comment.
Server doesn't provide a JSON schema, so this isn't actionable here right no
| 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}`)]); |
|
look at fallbacks and make sure that no new fallbacks were created, sometimes it is just unreachable |
…istrar.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| * @deprecated Use StoreMemoryRequest from @github/copilot-agentic-tools/memory directly. | ||
| */ | ||
| export interface RepoMemoryEntry { | ||
| export interface UserMemoryEntry { |
There was a problem hiding this comment.
Once Audree's PR is merged and used here, we should check on interface definations for combined user and repo entry - making the swap like this does not look right.
There was a problem hiding this comment.
Agreed, this doesn't look right, especially since there shouldn't be a difference between UserMemoryEntry and RepoMemoryEntry (they contain the same fields).
| } | ||
|
|
||
| /** | ||
| * Type guard to validate if an object is a valid RepoMemoryEntry. |
There was a problem hiding this comment.
please undo the formatting and use the repo defaults
| "@anthropic-ai/sdk": "^0.82.0", | ||
| "@github/blackbird-external-ingest-utils": "^0.3.0", | ||
| "@github/copilot": "^1.0.24", | ||
| "@github/copilot-agentic-tools": "file:../../../copilot-agentic-tools", |
There was a problem hiding this comment.
stand in, until Audree's PR is merged, I can remove it
| * Implements the vote_memory tool using @github/copilot-agentic-tools/memory. | ||
| * Only registered when voteToolDefinition is present in the /prompt response. | ||
| */ | ||
| export class VoteMemoryTool implements ICopilotModelSpecificTool<VoteMemoryInput> { |
There was a problem hiding this comment.
what is this tool helping with? we should not introduce this if copilot memory is not enabled.
There was a problem hiding this comment.
was a fragment of me porting over from an old repo and didn't specify to copilot correctly, removing it
There was a problem hiding this comment.
Pull request overview
Ports the user-scoped memory integration into extensions/copilot by replacing the previous CAPI request approach with the shared @github/copilot-agentic-tools/memory client, and wiring prompt/tooling changes to support the new unified /prompt flow.
Changes:
- Reworked
AgentMemoryServiceto use@github/copilot-agentic-tools/memoryfor enablement checks, fetching recent memories, storing memories, and fetching the unified prompt response. - Added a new
AgentMemoryToolRegistrarandStoreMemoryToolto dynamically register thestore_memorymodel-specific tool based on the/promptresponse. - Updated
MemoryContextPromptandMemoryToolto consume the unified prompt (with fallback) and normalize citations handling.
Show a summary per file
| File | Description |
|---|---|
| extensions/copilot/src/extension/tools/node/test/memoryTool.spec.tsx | Updates mock IAgentMemoryService implementation for the expanded memory API surface. |
| extensions/copilot/src/extension/tools/node/storeMemoryTool.tsx | Introduces the store_memory tool implementation and a minimal schema builder. |
| extensions/copilot/src/extension/tools/node/memoryTool.tsx | Adjusts repository-memory create path parsing/citations normalization and success messaging. |
| extensions/copilot/src/extension/tools/node/memoryContextPrompt.tsx | Uses unified /prompt response (<memory_context>) with fallback to legacy repo memory rendering. |
| extensions/copilot/src/extension/tools/node/agentMemoryToolRegistrar.ts | Adds a service that registers model-specific memory tools from /prompt response definitions. |
| extensions/copilot/src/extension/tools/common/agentMemoryService.ts | Replaces request-type based CAPI calls with @github/copilot-agentic-tools/memory APIs and adds /prompt support. |
| extensions/copilot/src/extension/extension/vscode-node/services.ts | Registers IAgentMemoryToolRegistrar in the extension service container. |
| extensions/copilot/package.json | Adds the new @github/copilot-agentic-tools dependency. |
Copilot's findings
Comments suppressed due to low confidence (1)
extensions/copilot/src/extension/tools/node/memoryTool.tsx:658
- _localCreate() writes a file to the filesystem, but the success message was changed to "User memory stored successfully." This is inaccurate for session/user local memory operations and will fail the unit tests that assert "File created successfully at: ...". Please restore a file-create success message for local storage paths (only the CAPI-backed repo path should avoid file wording).
const content = new TextEncoder().encode(params.file_text);
await this.fileSystemService.writeFile(uri, content);
if (scope === 'session') {
this.memoryCleanupService.markAccessed(uri);
}
this.logService.debug(`[MemoryTool] Created memory file: ${params.path}`);
return { text: 'User memory stored successfully.', outcome: 'success'};
} catch (error) {
- Files reviewed: 8/8 changed files
- Comments generated: 6
| /** | ||
| * Register store_memory (and optionally vote_memory) as model-specific tools using an | ||
| * already-fetched prompt response. When called without a response, fetches the /prompt | ||
| * endpoint itself. Safe to call multiple times — re-registers on each call to pick up | ||
| * updated tool definitions from the server. | ||
| */ | ||
| registerMemoryTools(promptResponse?: MemoryPromptResponse): Promise<void>; | ||
| } |
There was a problem hiding this comment.
The JSDoc says this registrar will register store_memory "(and optionally vote_memory)", and the PR description mentions vote_memory, but this implementation only registers store_memory. Either implement vote_memory registration based on the /prompt response (and add the tool), or remove/update the comment/PR description so behavior matches what's shipped.
There was a problem hiding this comment.
Same thing--let's just update the PR description
| "@anthropic-ai/sdk": "^0.82.0", | ||
| "@github/blackbird-external-ingest-utils": "^0.3.0", | ||
| "@github/copilot": "^1.0.24", | ||
| "@github/copilot-agentic-tools": "file:../../../copilot-agentic-tools", |
There was a problem hiding this comment.
Adding "@github/copilot-agentic-tools" as a file:../../../copilot-agentic-tools dependency points outside this repository, which will break clean installs/CI checkouts (the target directory isn't present here). It also leaves extensions/copilot/package-lock.json out of sync (no entry for this package). Please switch to a published/workspace-managed dependency that exists in CI (or vendor it into the repo) and update the lockfile accordingly.
| "@github/copilot-agentic-tools": "file:../../../copilot-agentic-tools", | |
| "@github/copilot-agentic-tools": "^1.0.0", |
| const success = await this.agentMemoryService.storeRepoMemory(entry); | ||
| if (success) { | ||
| return { text: `File created successfully at: ${params.path}`, outcome: 'success' }; | ||
| return { text: 'User memory stored successfully.', outcome: 'success'}; |
There was a problem hiding this comment.
In the /memories/repo/ CAPI path, this command stores a repository-scoped memory via storeRepoMemory(), but the success text says "User memory stored successfully." This is misleading (and breaks the existing tests that look for the repo create message). Please return a repo-scoped success message (and/or include the path/subject), distinct from user memory wording.
This issue also appears on line 651 of the same file.
| return { text: 'User memory stored successfully.', outcome: 'success'}; | |
| return { text: l10n.t("Repository memory stored successfully for \"{0}\".", entry.subject), outcome: 'success' }; |
| private formatMemories(memories: MemoryResponse[]): string { | ||
| return memories.map(m => { | ||
| const lines = [`**${m.subject}**`, `- Fact: ${m.fact}`]; | ||
|
|
||
| // Format citations (handle both string and string[] formats) | ||
| if (m.citations) { | ||
| const citationsArray = normalizeCitations(m.citations) ?? []; | ||
| if (citationsArray.length > 0) { | ||
| lines.push(`- Citations: ${citationsArray.join(', ')}`); | ||
| } | ||
| if (m.citations.length > 0) { | ||
| lines.push(`- Citations: ${m.citations.join(', ')}`); | ||
| } |
There was a problem hiding this comment.
MemoryResponse.citations is treated as always-present (m.citations.length), but the service/type guard still allows citations to be undefined/legacy. This can throw at runtime when formatting telemetry/context. Please guard with optional chaining or default to an empty array before reading .length/join().
See below for a potential fix:
const citations = m.citations ?? [];
if (citations.length > 0) {
lines.push(`- Citations: ${citations.join(', ')}`);
There was a problem hiding this comment.
believe this should be fixed, double check
| @@ -61,45 +88,47 @@ export function isRepoMemoryEntry(obj: unknown): obj is RepoMemoryEntry { | |||
| return true; | |||
There was a problem hiding this comment.
isRepoMemoryEntry() is declared as a type guard for MemoryResponse, but it currently accepts legacy shapes like citations: string and category. That makes the guard unsound: callers can treat the result as MemoryResponse and do m.citations.length (as in MemoryContextPrompt), leading to runtime errors. Either (1) tighten the guard to only accept the actual MemoryResponse shape (e.g., citations as string[]/default []), or (2) change the guard return type to a legacy/union type and normalize before returning MemoryResponse.
There was a problem hiding this comment.
I am not sure if this needs to be changed
There was a problem hiding this comment.
Before this PR, VSCode was calling the CAPI API directly and getting back raw JSON. This function was a runtime check that said, "does this blob of JSON actually look like a memory object?" Now that we're using the package, it makes the API call and returns a typed MemoryResponse, and that validation is happening within the package. So we shouldn't need this
| this.logService.error('[StoreMemoryTool] Error storing memory:', error); | ||
| return new LanguageModelToolResult([new LanguageModelTextPart(`Error storing memory: ${error}`)]); |
There was a problem hiding this comment.
ILogService.error expects the Error (or error string) as the first argument and an optional string context message as the second. Here it's called with a string first and the caught error second, which is both type-incorrect and loses structured error details. Also, returning the raw ${error} to the model/user can leak internal details. Please log as error-first with a stable context message, and return a generic failure message (optionally include a correlation id).
| this.logService.error('[StoreMemoryTool] Error storing memory:', error); | |
| return new LanguageModelToolResult([new LanguageModelTextPart(`Error storing memory: ${error}`)]); | |
| const correlationId = `store-memory-${Date.now()}`; | |
| const errorForLog = error instanceof Error ? error : String(error); | |
| this.logService.error(errorForLog, `[StoreMemoryTool] Error storing memory. CorrelationId: ${correlationId}`); | |
| return new LanguageModelToolResult([new LanguageModelTextPart(`Failed to store memory. Please try again later. Reference ID: ${correlationId}`)]); |
| confirmation = this.forward(this._wrapped.confirmation.bind(this._wrapped)); | ||
| warning = this.forward(this._wrapped.warning.bind(this._wrapped)); | ||
| info = this.forward(this._wrapped.info.bind(this._wrapped)); | ||
| info = this.forward(this._wrapped.info?.bind(this._wrapped) || (() => { })); |
There was a problem hiding this comment.
Is it related to our changes? I'd say if it's not directly related to or necessary for shipping the user memory capability, I'd remove it from the PR. My opinion is that, especially when working in code that we're not the owners of, we should have the lightest touch possible
|
|
||
| const cachedMemoryPrompt = agentMemoryService.getCachedMemoryPrompt(); | ||
| if (!cachedMemoryPrompt && configurationService.getExperimentBasedConfig(ConfigKey.CopilotMemoryEnabled, experimentationService)) { | ||
| logService.debug('[getAgentTools] CopilotMemory is enabled but cache is not primed yet — StoreMemory tool will be disabled this turn'); |
There was a problem hiding this comment.
So what's happening here is a timing issue: getAgentTools() runs before buildPrompt(), but the cache doesn't get primed until buildPrompt() calls registerMemoryTools() on turn 0 (when history.length === 0). That means on the
very first turn of every conversation, getCachedMemoryPrompt() will always return undefined here, and store_memory will be excluded from the tool list.
As a general pattern, logging an expected failure instead of fixing it is an anti-pattern. If we know the cache won't be ready here, the right move is to fix the ordering, not log around it.
A couple of ways to fix this:
- Prime the cache earlier (e.g., at session creation) so it's ready by the time
getAgentTools()runs - Allow the tool unconditionally when the config flag is on and have it fetch on-demand if the cache misses
- Await priming here if the cache isn't populated yet
| allowTools[ToolName.MultiReplaceString] = true; | ||
| } | ||
|
|
||
| const cachedMemoryPrompt = agentMemoryService.getCachedMemoryPrompt(); |
There was a problem hiding this comment.
See comment below, that this might not be the right approach. But, more generally, getCachedMemoryPrompt() is being called here without a sessionId, which means it falls back to returning the first entry in the Map — that's non-deterministic when multiple sessions are active and could return the prompt/schema from a completely different conversation. Since request is available in this function, you should be able to extract and pass a sessionId here (and everywhere you're calling this function).
| import { describe, expect, it } from 'vitest'; | ||
| import { isRepoMemoryEntry, normalizeCitations } from '../agentMemoryService'; | ||
|
|
||
| describe('AgentMemoryService', () => { |
There was a problem hiding this comment.
The old test file covered isRepoMemoryEntry and normalizeCitations, and since those helpers are gone, deleting their tests makes sense. But the new service has significant new behavior that needs coverage:
- Cache lifecycle:
getMemoryPrompt()cache hit/miss, session-scoped keying,getCachedMemoryPrompt()with and without sessionId - Cache cleanup:
clearCache(sessionId)viaonDidDisposeChatSession storeUserMemory()/storeRepoMemory(): success, failure, and missing auth token paths
| baseUrl: this.getBaseUrl(), | ||
| agent: 'vscode', | ||
| logger: this.makeLogger(), | ||
| }; |
There was a problem hiding this comment.
Can we add the base_model if available here and for storing repo memories? See https://github.com/github/copilot-agentic-tools/blob/main/src/memory/api.ts#L241)
| Note: Only `create` is supported for `/memories/repo/` paths.<br /> | ||
| If the user asks how to view or manage their repo memories refer them to https://docs.github.com/en/copilot/how-tos/use-copilot-agents/copilot-memory.<br /> | ||
| {storeInstructionsPrompt && ( | ||
| <Tag name='storeMemoryInstructions'> |
There was a problem hiding this comment.
You're replacing the entire hardcoded repo memory instructions block with the server-provided storeInstructionsPrompt. The issue is that the server has no awareness of the local memory tool, so when enableCopilotMemory is true, all guidance about local repo memory files (/memories/repo/) disappears from the prompt.
The MemoryContextPrompt above still shows local repo files in context (the L85 fix is good!), but without any instructions here, the model sees those files with no guidance on what they are or how to interact with them.
Could we keep the local repo memory instructions alongside the server-provided ones when both flags are on? Something like rendering the local instructions when enableMemoryTool is true, and the server instructions when enableCopilotMemory is true, since they're not mutually exclusive.
|
|
||
| constructor( | ||
| @ILogService private readonly logService: ILogService | ||
| @ILogService protected readonly logService: ILogService |
There was a problem hiding this comment.
Copilot's findings
Files not reviewed (1)
- extensions/copilot/package-lock.json: Language not supported
Comments suppressed due to low confidence (2)
extensions/copilot/src/extension/tools/node/memoryContextPrompt.tsx:90
- This text says local repo memory files can be read via the
memorytool, but when Copilot Memory is enabledmemoryToolcurrently rejectsviewfor/memories/repo/*(onlycreateis supported). Either adjust the messaging to avoid suggesting an unsupported operation, or change the routing so local repo files remain viewable/editable when both systems are enabled.
<Tag name='repoMemory'>
{localRepoMemoryFiles && localRepoMemoryFiles.length > 0
? <>The following files exist in your repository memory (/memories/repo/). These are scoped to the current workspace. Use the {ToolName.Memory} tool to read them if needed.<br /><br />{localRepoMemoryFiles.join('\n')}</>
: <>Repository memory (/memories/repo/) is empty. No workspace-scoped notes have been created yet.</>
}
extensions/copilot/src/extension/tools/node/memoryContextPrompt.tsx:285
- These instructions say the
memorytool canview/str_replace/delete/renamelocal/memories/repo/*files while Copilot Memory is enabled, butMemoryToolcurrently rejects non-createcommands for repo paths when Copilot Memory is on. Either update the routing to keep local repo files manageable in this mode, or adjust the instructions to match current behavior.
{enableCopilotMemory && <>Guidelines for local repository memory (`/memories/repo/`):<br />
- Use the `{ToolName.Memory}` tool to `view`, `str_replace`, `delete`, or `rename` existing local repo memory files.<br />
- New repository facts should be stored via Copilot Memory (see instructions below) rather than creating new local files.<br />
</>}
- Files reviewed: 18/19 changed files
- Comments generated: 6
| const zodSchema = resolveStoreMemorySchema(toolDef.definitionVersion); | ||
| const inputSchema = zodToJsonSchema(zodSchema, { target: 'openApi3' }) as { [key: string]: unknown }; | ||
| return { ...tool, description: toolDef.description, inputSchema }; | ||
| } |
There was a problem hiding this comment.
alternativeDefinition() converts a Zod schema to JSON schema on every call. Since the schema is determined solely by definitionVersion, consider memoizing the generated inputSchema per version (or caching the final tool definition) to avoid repeated work during tool list construction.
| // Route /memories/repo/* to CAPI if enabled, otherwise local storage | ||
| if (isRepoPath(path)) { | ||
| const capiEnabled = await this.agentMemoryService.checkMemoryEnabled(); | ||
| const capiEnabled = this.configurationService.getExperimentBasedConfig(ConfigKey.CopilotMemoryEnabled, this.experimentationService); | ||
| if (capiEnabled) { | ||
| const result = await this._dispatchRepoCAPI(params, path); |
There was a problem hiding this comment.
When Copilot Memory is enabled, repo paths are routed into _dispatchRepoCAPI, which only supports create. This prevents viewing/editing existing local /memories/repo/* files in the combined-flags scenario, even though the prompt UI now lists those files and suggests using the memory tool to read/edit them. Consider routing only create through CAPI and handling the other commands via the local repo store (or stop listing local repo files / adjust instructions).
See below for a potential fix:
// Route repo creates to CAPI if enabled; other repo operations continue to use local storage
if (isRepoPath(path)) {
const capiEnabled = this.configurationService.getExperimentBasedConfig(ConfigKey.CopilotMemoryEnabled, this.experimentationService);
if (capiEnabled && params.command === 'create') {
| const sessionMemoryFiles = enableMemoryTool ? await this.getSessionMemoryFiles(this.props.sessionResource) : undefined; | ||
| const localRepoMemoryFiles = enableMemoryTool ? await this.getLocalRepoMemoryFiles() : undefined; | ||
|
|
||
| // When CAPI memory is enabled, read from the cache primed by AgentMemoryToolRegistrar |
There was a problem hiding this comment.
Comment references AgentMemoryToolRegistrar, but the cache priming service was renamed to AgentMemoryCachePrimer. Update the comment to match the current class/service name to avoid confusion when debugging cache behavior.
This issue also appears in the following locations of the same file:
- line 86
- line 282
| // When CAPI memory is enabled, read from the cache primed by AgentMemoryToolRegistrar | |
| // When CAPI memory is enabled, read from the cache primed by AgentMemoryCachePrimer |
| } | ||
| // Call with sessionId but let service auto-determine repoNwo to match original working behavior | ||
| const response = await this.agentMemoryService.getMemoryPrompt(undefined, sessionId); | ||
| this.logService.info(`[AgentMemoryToolRegistrar] primed memory prompt cache for session ${sessionId || 'default'}, definitionVersion=${response?.storeToolDefinition?.definitionVersion ?? 'none'}`); |
There was a problem hiding this comment.
Log prefix still says AgentMemoryToolRegistrar even though this service/class is AgentMemoryCachePrimer. Align the log tag with the new name so log filtering/searching continues to work as expected.
| this.logService.info(`[AgentMemoryToolRegistrar] primed memory prompt cache for session ${sessionId || 'default'}, definitionVersion=${response?.storeToolDefinition?.definitionVersion ?? 'none'}`); | |
| this.logService.info(`[AgentMemoryCachePrimer] primed memory prompt cache for session ${sessionId || 'default'}, definitionVersion=${response?.storeToolDefinition?.definitionVersion ?? 'none'}`); |
| const result = tools | ||
| .filter(tool => { | ||
| // 0. If the tool was a model specific tool with an override, it'll be mixed in in the 'map' later. |
There was a problem hiding this comment.
This intermediate result variable is immediately returned and doesn’t add value vs returning the filter/map chain directly. Consider reverting to a direct return to keep the change minimal (and avoid leaving trailing whitespace in the hunk).
| const omitBaseAgentInstructions = this.configurationService.getConfig(ConfigKey.Advanced.OmitBaseAgentInstructions); | ||
| // TODO:@bhavyau find a better way to extract session resource | ||
| const sessionResourceStr = (this.props.promptContext.tools?.toolInvocationToken as any)?.sessionResource as string | undefined; | ||
| const baseAgentInstructions = <> |
There was a problem hiding this comment.
sessionResourceStr is extracted from toolInvocationToken via as any, even though a typed promptContext.request?.sessionResource is already available a few lines later. Consider using this.props.promptContext.request?.sessionResource?.toString() (or passing sessionId/sessionResource through props explicitly) to avoid relying on an untyped token shape.
User-Scoped Memories + Unified
/promptEndpoint MigrationWhat this PR does
This PR extends Copilot Memory (CAPI) support in four areas:
scopeparameter to thestore_memorytool so facts can be stored per-user rather than only per-repo/promptendpoint — migrates away from a bespoke memory fetch API to the shared/promptendpoint, which returns context, store instructions, and tool schema in one callstore_memorywas always missing from the tool list on the first turn of every conversation; this is now fixedMemoryToolEnabledandCopilotMemoryEnabledare both on, instructions and context for both storage systems are shown together rather than one replacing the otherChanges in detail
1. User-scoped memory
store_memorynow accepts an optionalscopefield ('repo'|'user', defaulting to'repo'). Routing:scope: 'repo'→AgentMemoryService.storeRepoMemory()→storeMemory({ scope: 'repository' })scope: 'user'→AgentMemoryService.storeUserMemory()→storeMemory({ scope: 'user' })The legacy
memorytool also mirrors usercreateoperations to CAPI via_syncUserMemoryCAPI()when the config flag is on. This is intentional one-way sync: the localmemorytool remains the source of truth for in-session CRUD (view/str_replace/delete), and CAPI provides ambient context injection at conversation start via the/promptresponse. The two systems are complementary.2. Unified
/promptendpointBefore:
MemoryContextPromptfetched repo memories via a separategetRepoMemories()call and formatted them locally.MemoryInstructionsPromptused a large hardcoded<repoMemoryInstructions>block.After: Both components read from
AgentMemoryService.getMemoryPrompt(), which callsfetchMemoryPrompts()from@github/copilot-agentic-tools/memory. The response provides:memoriesContext.prompt— server-formatted memory context, rendered in<memory_context>(replaces<repository_memories>and the localformatMemories()method)storeInstructions.prompt— server-authored store guidance, rendered in<storeMemoryInstructions>(replaces the hardcoded<repoMemoryInstructions>block)storeToolDefinition— server-provided schema version, used byStoreMemoryTool.alternativeDefinition()to resolve the correct Zod schema viaresolveStoreMemorySchema(definitionVersion)and convert it to JSON Schema at runtimeThis means Copilot Memory's instructions and context evolve server-side without requiring a client ship.
3. Turn-0 tool availability fix
Before:
getAgentTools()calledgetCachedMemoryPrompt()synchronously. The cache was never populated at that point (it was primed later inbuildPrompt()), sostore_memorywas excluded from the tool list on every first turn.After:
getAgentTools()awaitsgetMemoryPrompt()on a cache miss when the config is enabled, priming the cache itself.AgentMemoryCachePrimer.primeCache()(called frombuildPrompt()on turn 0) then hits the already-warm cache and is a no-op — no duplicate network call.4. Local memory tool coexistence (
memoryContextPrompt.tsx)When both
MemoryToolEnabledandCopilotMemoryEnabledare on, the model sees local repo files listed in<repoMemory>(from the localmemorytool's storage). Previously, the instructions section only showed the CAPI description ("onlycreatesupported") with no guidance for those local files.The scope description and guidelines now handle three cases explicitly:
create/viewusagecreatesupportedstoreMemoryInstructionsmemorytool to view/edit existing local filesview/str_replace/delete/rename) alongside serverstoreMemoryInstructionsCaching architecture
The
/promptresponse is cached per session inAgentMemoryService._conversationMemoryCache: Map<sessionId, MemoryPromptResponse>. An_inflightPromptsmap prevents duplicate network calls if the same session triggers concurrent fetches. Cache is cleared when the session is disposed viachatSessionService.onDidDisposeChatSession.Turn-by-turn flow:
AgentMemoryCachePrimeris a cache-warming service, not a VS Code tool registrar. It has no tool registration state to clean up when the config is disabled. Tool availability is re-evaluated each turn ingetAgentTools()by reading the config flag at call time.Other changes
getSessionIdFromTokenhelper — replaced two duplicate(x as any)?.sessionResourceextractions inagentIntent.tswith a single typed helper, reducing theas anysurface to one placepromptContext.history?.length === 0would silently skip cache priming whenhistoryisundefined; corrected to!promptContext.history || promptContext.history.length === 0_inflightPromptsmap inAgentMemoryServicededuplicates concurrent calls for the same session; previously two simultaneous callers would each make a full network round tripAgentMemoryToolRegistrar→AgentMemoryCachePrimer— the old name implied VS Code tool registration; the class only primes the prompt cacheerror.messagein twomemoryTool.tsxcatch blocks narrowed toerror instanceof Error ? error.message : String(error)What is NOT in this PR
vote_memory/vote_memorytool: Not implemented. TheIAgentMemoryServiceinterface has novoteOnMemorymethod and novote_memorytool is registered anywhere.memorytool'sview,str_replace,delete, andrenameoperations for user-scoped paths remain local-only. CAPI memories surface as ambient context at conversation start viamemoriesContext.prompt; they are not a CRUD-accessible store in this PR.definitionVersionruntime assertion: If the shared package and server ever drift on schema version, the client will silently use the wrong schema. A runtime check is a recommended follow-up.Tests
storeRepoMemory,storeUserMemory,getMemoryPrompt(cache hit / miss / clear / concurrent),getBaseUrl, config guard, error handlingalternativeDefinition(empty cache, missingstoreToolDefinition, schema override)_dispatchRepoCAPI), user CAPI sync (_syncUserMemoryCAPI) for JSON and plain-text formats, session-scope exclusion, telemetryTested locally, and was able to store memories using
store_memoryThen after asking it to remember a few things about me ->