Add user-scoped memory store and retrieval via unified /prompt endpoint#5064
Add user-scoped memory store and retrieval via unified /prompt endpoint#5064sneefyyy wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds user-scoped Copilot Memory support to the VS Code Copilot Chat extension, including writing user memories to a backend endpoint and retrieving a unified “repo + user” memory prompt for injection into the model context.
Changes:
- Extend
IAgentMemoryServicewith user-memory store + unified memory prompt retrieval APIs. - Route
/memories/*user-scopecreateoperations through CAPI when Copilot Memory is enabled (with local fallback). - Update memory context prompt construction to prefer a unified backend-provided prompt, with fallback to the existing repo-memories fetch path.
Show a summary per file
| File | Description |
|---|---|
| src/extension/tools/common/agentMemoryService.ts | Adds UserMemoryEntry / MemoryPromptResponse and implements user-memory store + unified prompt fetch. |
| src/extension/tools/node/memoryTool.tsx | Routes user-scope create through CAPI via new _userCreate() path. |
| src/extension/tools/node/memoryContextPrompt.tsx | Fetches unified backend memory prompt and injects it into prompt context; adds repo NWO resolution. |
| src/extension/tools/node/test/memoryTool.spec.tsx | Updates mocks to satisfy new IAgentMemoryService interface methods/types. |
| src/extension/tools/common/test/agentMemoryService.spec.ts | Adds type-level tests for new interfaces. |
Copilot's findings
Comments suppressed due to low confidence (4)
src/extension/tools/node/memoryContextPrompt.tsx:97
- When the unified
/promptendpoint is used (memoryPromptText), repo memory telemetry is still reported asrepoMemoryCount=0/repoMemoryLength=0, so thememoryContextReadevent no longer reflects how much backend memory context was actually injected. Consider adding measurements for the unified prompt path (e.g.,memoryPromptLengthand a flag likeusedUnifiedPrompt) and/or mapping those values into the existing fields whenmemoryPromptTextis present.
this._sendContextReadTelemetry(
!!userMemoryContent,
userMemoryContent?.length ?? 0,
sessionMemoryFiles?.length ?? 0,
sessionMemoryFiles?.join('\n').length ?? 0,
repoMemories?.length ?? 0,
repoMemories ? this.formatMemories(repoMemories).length : 0,
);
src/extension/tools/common/agentMemoryService.ts:435
- Same base-URL derivation issue as in
storeUserMemory():capiPingURL.replace(/\/_ping$/, '')won’t strip a/pingsuffix, which can yield an invalid endpoint like.../copilot_internal/ping/agents/.... Use a URL-safe base derivation (strip the last segment regardless of whether it ispingor_ping) or a stable base property (e.g.proxyBaseURL) rather than relying on regex replacement.
const username = session.account.label;
const capiBase = this.capiClientService.capiPingURL.replace(/\/_ping$/, '');
const params = new URLSearchParams({ user: username });
if (repoNwo) {
params.set('repo', repoNwo);
}
const url = `${capiBase}/agents/swe/internal/memory/v0/prompt?${params.toString()}`;
src/extension/tools/node/memoryTool.tsx:306
- When CAPI is enabled,
createunder/memories/now stores user memory remotely and does not create a local file, but the tool still supports localview/str_replace/insert/delete/renameoperations for/memories/*. That makes the path behave like a file in responses while subsequent reads/updates will still hit local storage and won’t see the just-created remote memory. Consider either (1) implementing the other user-scope operations against the user-memory API, (2) explicitly rejecting non-createoperations for user memories when CAPI is enabled (similar to repo scope), or (3) writing a local shadow file so follow-up operations remain consistent.
// 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
const result = await this._dispatchLocal(params, 'user', sessionResource);
this._sendLocalTelemetry(params.command, 'user', result.outcome, requestId, model);
return result.text;
}
src/extension/tools/node/memoryTool.tsx:391
- New user-scope CAPI path (
_userCreate+ routing in_dispatch) isn’t covered by assertions in the existing Vitest suite (e.g., thatstoreUserMemory()is called when CAPI is enabled and that local file fallback occurs when disabled). Adding targeted tests would prevent regressions and make the routing behavior explicit.
// 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
const result = await this._dispatchLocal(params, 'user', sessionResource);
this._sendLocalTelemetry(params.command, 'user', result.outcome, requestId, model);
return result.text;
}
private async _dispatchRepoCAPI(params: MemoryToolParams, path: string): Promise<MemoryToolResult> {
switch (params.command) {
case 'create':
return this._repoCreate(params);
default:
return { text: `Error: The '${params.command}' operation is not supported for repository memories at ${path}. Only 'create' is allowed for /memories/repo/.`, outcome: 'error' };
}
}
private async _repoCreate(params: ICreateParams): Promise<MemoryToolResult> {
try {
// Derive subject/category hint from the path (e.g. /memories/repo/testing.json → "testing")
const filename = params.path.split('/').pop() || 'memory';
const pathHint = filename.replace(/\.\w+$/, '');
// Parse the file_text as a memory entry.
// Accept either a JSON-formatted entry or a plain text fact.
let entry: RepoMemoryEntry;
try {
const parsed = JSON.parse(params.file_text);
entry = {
subject: parsed.subject || pathHint,
fact: parsed.fact || '',
citations: parsed.citations || '',
reason: parsed.reason || '',
category: parsed.category || pathHint,
};
} catch {
// Plain text: treat the whole content as a fact, use path as subject
entry = {
subject: pathHint,
fact: params.file_text,
citations: '',
reason: 'Stored from memory tool create command.',
category: pathHint,
};
}
const success = await this.agentMemoryService.storeRepoMemory(entry);
if (success) {
return { text: `File created successfully at: ${params.path}`, outcome: 'success' };
} else {
return { text: 'Error: Failed to store repository memory entry.', outcome: 'error' };
}
} catch (error) {
this.logService.error('[MemoryTool] Error creating repo memory:', error);
return { text: `Error: Cannot create repository memory: ${error.message}`, outcome: 'error' };
}
}
private async _userCreate(params: ICreateParams): Promise<MemoryToolResult> {
try {
const filename = params.path.split('/').pop() || 'memory';
const pathHint = filename.replace(/\.\w+$/, '');
let entry: UserMemoryEntry;
try {
const parsed = JSON.parse(params.file_text);
entry = {
subject: parsed.subject || pathHint,
fact: parsed.fact || '',
citations: parsed.citations || '',
reason: parsed.reason || '',
};
} catch {
entry = {
subject: pathHint,
fact: params.file_text,
citations: '',
reason: 'Stored from memory tool create command.',
};
}
const success = await this.agentMemoryService.storeUserMemory(entry);
if (success) {
return { text: `File created successfully at: ${params.path}`, outcome: 'success' };
} else {
return { text: 'Error: Failed to store user memory entry.', outcome: 'error' };
}
} catch (error) {
this.logService.error('[MemoryTool] Error creating user memory:', error);
return { text: `Error: Cannot create user memory: ${error.message}`, outcome: 'error' };
}
}
- Files reviewed: 5/5 changed files
- Comments generated: 5
| 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.
getRepoNwo() duplicates the repo-NWO resolution logic that already exists in AgentMemoryService (src/extension/tools/common/agentMemoryService.ts:157-182). Duplicating this makes it easy for the two implementations to drift (e.g., multi-root handling, remote selection). Consider extracting a shared helper (or exposing a repo-NWO resolver service method) so both call sites stay consistent.
| // Derive base URL from capiPingURL (strips /_ping suffix) | ||
| const capiBase = this.capiClientService.capiPingURL.replace(/\/_ping$/, ''); |
There was a problem hiding this comment.
Deriving capiBase from capiPingURL by stripping only a /_ping suffix is likely incorrect. In existing tests/mocks capiPingURL is .../copilot_internal/ping, so this replace(/\/_ping$/, '') will not strip anything and the request will go to .../copilot_internal/ping/agents/... (invalid path). Consider deriving the base URL via new URL() (e.g., remove the last path segment for both /ping and /_ping) or using a dedicated base like capiClientService.proxyBaseURL if available.
This issue also appears on line 427 of the same file.
| // Derive base URL from capiPingURL (strips /_ping suffix) | |
| const capiBase = this.capiClientService.capiPingURL.replace(/\/_ping$/, ''); | |
| // Derive base URL from capiPingURL by removing the final path segment (for both /ping and /_ping). | |
| const capiBaseUrl = new URL(this.capiClientService.capiPingURL); | |
| capiBaseUrl.pathname = capiBaseUrl.pathname.replace(/\/[^\/]+$/, ''); | |
| const capiBase = capiBaseUrl.toString().replace(/\/$/, ''); |
| const response = await fetch(url, { | ||
| method: 'PUT', | ||
| headers: { | ||
| 'Authorization': `Bearer ${session.accessToken}`, | ||
| 'Content-Type': 'application/json', | ||
| 'X-GitHub-Api-Version': '2025-10-01', | ||
| }, | ||
| body: JSON.stringify({ | ||
| subject: memory.subject, | ||
| fact: memory.fact, | ||
| citations, | ||
| reason: memory.reason, | ||
| source: { agent: 'vscode' }, | ||
| }), | ||
| }); |
There was a problem hiding this comment.
These user-memory requests use the global fetch() directly, which bypasses the extension's networking stack (proxy handling, user-agent, AB-exp headers, fetch telemetry, retry/backoff, etc.) that capiClientService.makeRequest() provides (and that repo memory methods already use). This can cause failures behind proxies and inconsistent request headers. Prefer routing these calls through ICAPIClientService.makeRequest (or the repo-standard fetcher service) so they behave consistently with other CAPI traffic.
| // 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; | ||
| } |
There was a problem hiding this comment.
User-scope routing is currently gated by agentMemoryService.checkMemoryEnabled(), but that method is repository-scoped (it returns false when there’s no repo NWO / memory not enabled for the repo). This means /memories/* creates won’t be routed to user CAPI memory in non-git workspaces or repos without memory enabled, even if Copilot Memory is enabled/configured. Consider using a config-level check for user memory (or just attempt storeUserMemory() and fall back to local only when it returns false).
This issue also appears in the following locations of the same file:
- line 294
- line 294
| const userMemoryContent = enableMemoryTool ? await this.getUserMemoryContent() : undefined; | ||
| const sessionMemoryFiles = enableMemoryTool ? await this.getSessionMemoryFiles(this.props.sessionResource) : undefined; | ||
| const repoMemories = enableCopilotMemory ? await this.agentMemoryService.getRepoMemories() : undefined; | ||
| const localRepoMemoryFiles = (enableMemoryTool && !enableCopilotMemory) ? await this.getLocalRepoMemoryFiles() : undefined; | ||
|
|
||
| if (!enableMemoryTool && !enableCopilotMemory) { | ||
| return null; | ||
| // When CAPI memory is enabled, fetch from the unified /prompt endpoint | ||
| let memoryPromptText: string | undefined; | ||
| let repoMemories: RepoMemoryEntry[] | undefined; | ||
| if (enableCopilotMemory) { | ||
| const repoNwo = await this.getRepoNwo(); | ||
| const promptResponse = await this.agentMemoryService.getMemoryPrompt(repoNwo); | ||
| memoryPromptText = promptResponse?.prompt; | ||
| // Fall back to individual repo memories if /prompt endpoint is unavailable | ||
| if (!memoryPromptText) { | ||
| repoMemories = await this.agentMemoryService.getRepoMemories(); | ||
| } |
There was a problem hiding this comment.
With Copilot Memory enabled, user memory writes are routed to CAPI, but this prompt still loads user memory content from local files (getUserMemoryContent()). That can lead to showing “No user preferences…” even when the backend has user-scoped memories, and it also means newly stored user memories won’t round-trip via /memories/* local reads. Consider suppressing local user-memory loading when the unified /prompt endpoint is used, or switching the user-memory section to be sourced from the unified prompt response (with local files only as fallback).
|
@sneefyyy please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds user-scoped memory support and consolidates memory context retrieval via a unified /prompt-backed endpoint, including dynamic registration of memory tools.
Changes:
- Adds model-specific
store_memoryandvote_memorytools plus a registrar that builds tool definitions from/prompt. - Routes user-scope memory writes through CAPI when enabled, while keeping local fallbacks.
- Updates memory context prompt rendering to prefer unified backend-provided prompt text and registers tools dynamically.
Show a summary per file
| File | Description |
|---|---|
| src/extension/tools/node/voteMemoryTool.tsx | Introduces vote_memory tool and schema builder. |
| src/extension/tools/node/storeMemoryTool.tsx | Introduces store_memory tool and schema builder derived from /prompt. |
| src/extension/tools/node/memoryTool.tsx | Routes user-scope creates to CAPI when enabled; updates memory entry parsing. |
| src/extension/tools/node/memoryContextPrompt.tsx | Fetches unified memory prompt and registers tools; resolves repo NWO via git/workspace services. |
| src/extension/tools/node/agentMemoryToolRegistrar.ts | New registrar that fetches /prompt and registers tools accordingly. |
| src/extension/tools/common/agentMemoryService.ts | Migrates memory API calls to @github/copilot-agentic-tools/memory; adds user store, voting, and prompt fetch. |
| src/extension/tools/node/test/memoryTool.spec.tsx | Updates mocks/types to new agent memory service interface. |
| src/extension/tools/common/test/agentMemoryService.spec.ts | Adds tests for MemoryPromptResponse typing/shape. |
| src/extension/extension/vscode-node/services.ts | Registers the new IAgentMemoryToolRegistrar service. |
| package.json | Adds a local file dependency for @github/copilot-agentic-tools. |
Copilot's findings
- Files reviewed: 10/11 changed files
- Comments generated: 7
| "@anthropic-ai/sdk": "^0.82.0", | ||
| "@github/blackbird-external-ingest-utils": "^0.3.0", | ||
| "@github/copilot": "^1.0.17", | ||
| "@github/copilot-agentic-tools": "file:../copilot-agentic-tools", |
There was a problem hiding this comment.
Using a file: dependency to ../copilot-agentic-tools will break installs/builds in CI and for consumers who don't have that sibling directory (e.g., packaged releases, clean checkouts). Replace this with a published semver dependency (or a workspace/monorepo-managed reference if this repo supports that) so dependency resolution is reproducible.
| "@github/copilot-agentic-tools": "file:../copilot-agentic-tools", | |
| "@github/copilot-agentic-tools": "^0.44.0", |
| // 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 triggers two /prompt calls per render when Copilot Memory is enabled: one via getMemoryPrompt(repoNwo) and another inside registerMemoryTools(). This adds avoidable latency and doubles backend load. Consider changing the registrar API to accept a pre-fetched promptResponse (or have the registrar return the fetched response) so both prompt injection and tool registration share a single network call.
| } | ||
| } 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.
error is unknown in a catch block; accessing error.message can throw if a non-Error is thrown. Use a safe extraction (e.g., error instanceof Error ? error.message : String(error)) so the error path can't crash while handling failures.
| 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 votes with scope 'repository'. Either update the comment to match behavior, or (if intended) perform votes for both 'repository' and 'user' (or accept a scope in tool input and route accordingly) so the tool does what it claims.
| // Vote on both scopes; the service will route to the right endpoint | |
| // Vote using the repository scope. |
| 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.
This replaces capiClientService.makeRequest(...) with a direct fetch(...). In VS Code extensions, routing network requests through the existing client service often ensures consistent proxy handling, request classification/telemetry, and shared retry/timeouts. Consider switching this enablement check back to ICAPIClientService (or adding equivalent proxy/telemetry behavior here) to avoid environment-specific failures and missing request instrumentation.
| // Check if this is the scoped schema by inspecting its shape keys | ||
| const shape = (schema as { shape?: Record<string, unknown> }).shape; | ||
| if (shape && 'scope' in shape) { |
There was a problem hiding this comment.
This relies on a non-standard shape property via type assertion, which is fragile across zod versions (and may not exist depending on how the schema is constructed). A more robust approach is to decide whether to include scope based on definitionVersion (since the PR already threads it through), or to use zod's public APIs/types for ZodObject shape access rather than asserting internal structure.
| // Check if this is the scoped schema by inspecting its shape keys | |
| const shape = (schema as { shape?: Record<string, unknown> }).shape; | |
| if (shape && 'scope' in shape) { | |
| // Check if this is the scoped schema using zod's public validation API. | |
| // Parsing an empty object will surface missing required fields; the scoped | |
| // variant additionally reports a top-level missing `scope` field. | |
| const emptyObjectValidation = schema.safeParse({}); | |
| const hasRequiredScope = !emptyObjectValidation.success && emptyObjectValidation.error.issues.some((issue) => issue.path.length === 1 && issue.path[0] === 'scope'); | |
| if (hasRequiredScope) { |
| 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 that affects tool availability at runtime (dynamic registration based on /prompt, including legacy fallback to toolDefinition). Given the repo already has vitest coverage for the memory service/tooling, add focused unit tests for AgentMemoryToolRegistrar.registerMemoryTools() covering: (1) disabled config short-circuit, (2) missing prompt response, (3) legacy toolDefinition fallback, and (4) vote tool registration conditional on voteToolDefinition.
Summary
Adds support for user-scoped memories in VSCode Copilot Chat, covering both the store and retrieve paths.
/memories/) are routed to the sweagentd API (PUT /agents/swe/internal/memory/v0/users/{username}) instead of local file storageGET /memory/v0/prompt?user={username}&repo={nwo}endpoint that returns combined repo and user memory context in a single response. Falls back to the existinggetRepoMemories()path if the endpoint is unavailable.Changes
agentMemoryService.ts— addsUserMemoryEntryandMemoryPromptResponsetypes; addsstoreUserMemory()andgetMemoryPrompt()toIAgentMemoryServiceandAgentMemoryServicememoryTool.tsx— routes user-scopecreatecommands through CAPI when enabled via new_userCreate()methodmemoryContextPrompt.tsx— fetches unified memory prompt from the backend and injects it as-is rather than constructing it client-side; injectsIGitServiceandIWorkspaceServiceto resolve the repo NWO