Skip to content

Conversation

@pwang347
Copy link
Member

@pwang347 pwang347 commented Jan 8, 2026

No description provided.

Copilot AI review requested due to automatic review settings January 8, 2026 01:33
@pwang347 pwang347 changed the base branch from main to pawang/providerV2 January 8, 2026 01:33
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds local caching support for prompt file provider APIs, allowing VS Code to display extension-provided prompt files (agents, instructions, and prompt files) before the extension activates. The implementation introduces an isCacheable boolean property that defaults to true, enabling extensions to opt out of caching for dynamic content.

Key Changes:

  • Added isCacheable property to prompt file resource APIs for controlling cache behavior
  • Made isEditable property readonly in API definitions for consistency
  • Implemented persistent storage-based caching of provider resources using profile-scoped storage
  • Added cache invalidation when extensions are uninstalled

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
src/vscode-dts/vscode.proposed.chatPromptFiles.d.ts Adds isCacheable property to CustomAgentOptions, InstructionsOptions, and PromptFileOptions interfaces; makes isEditable readonly
src/vs/workbench/contrib/chat/common/promptSyntax/service/promptsService.ts Adds isCacheable property to IPromptFileResource interface
src/vs/workbench/contrib/chat/common/promptSyntax/service/promptsServiceImpl.ts Implements caching logic with storage persistence, cache retrieval for unactivated extensions, and cache invalidation
src/vs/workbench/api/common/extHostTypes.ts Adds isCacheable property to CustomAgentChatResource, InstructionsChatResource, and PromptFileChatResource classes
src/vs/workbench/contrib/chat/test/common/promptSyntax/service/promptsService.test.ts Adds comprehensive test suite covering caching, non-caching, cache updates, cache clearing, and pre-activation scenarios

Comment on lines +1899 to +1902
if (cachedValue) {
const cached = JSON.parse(cachedValue);
assert.strictEqual(cached.length, 0, 'Should not have cached non-cacheable resources');
}
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test assertion logic is incomplete. When a provider returns only non-cacheable resources, the cache should either not exist or be an empty array. The current implementation writes an empty array when all resources are non-cacheable (line 176 in promptsServiceImpl.ts stores an empty array). The test should assert that the cache is undefined OR contains an empty array, not just check if it's an empty array when it exists. Consider changing to: "assert.ok(!cachedValue || JSON.parse(cachedValue).length === 0, 'Should not have cached non-cacheable resources');"

Suggested change
if (cachedValue) {
const cached = JSON.parse(cachedValue);
assert.strictEqual(cached.length, 0, 'Should not have cached non-cacheable resources');
}
assert.ok(!cachedValue || JSON.parse(cachedValue).length === 0, 'Should not have cached non-cacheable resources');

Copilot uses AI. Check for mistakes.
Comment on lines +472 to +489
// Filter out cached resources from extensions that now have active providers
// (the provider's resources replace the cached ones, even if URIs differ due to dynamic content)
const cachedExtensionIds = new Set(cachedResources.map(c => (c as IExtensionPromptPath).extension.identifier.value));
return result.filter(r => {
if (r.storage !== PromptsStorage.extension) {
return true;
}
const extensionPath = r as IExtensionPromptPath;
const extensionId = extensionPath.extension.identifier.value;
// If this is a cached resource and the extension now has an active provider, filter it out
if (cachedExtensionIds.has(extensionId) && activeProviderExtensionIds.has(extensionId)) {
// Keep only if it came from the provider (not from cache)
// Cached resources were added first, provider resources added after
// We can identify cached ones by checking if they're in the original cachedResources array
return !cachedResources.some(c => c.uri.toString() === r.uri.toString());
}
return true;
});
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The filtering logic at line 486 uses a nested loop (filter + some) which results in O(n*m) complexity where n is the number of results and m is the number of cached resources. For extensions with many resources, this could be inefficient. Consider creating a Set of cached URIs before the filter operation to make the lookup O(1): "const cachedUris = new Set(cachedResources.map(c => c.uri.toString()));" and then use "cachedUris.has(r.uri.toString())" instead of the some() call.

Copilot uses AI. Check for mistakes.
*/
private setCachedProviderResources(extensionId: string, type: PromptsType, resources: IPromptFileResource[]): void {
const key = `${this.cachedProviderResourcesStorageKeyPrefix}${extensionId}.${type}`;
this.logger.info(`[setCachedProviderResources] Setting cache for ${extensionId}/${type}, input resources:`, JSON.stringify(resources.map(r => ({ name: r.name, description: r.description, uri: r.uri?.toString(), isEditable: r.isEditable, isCacheable: r.isCacheable }))));
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logging at line 165 uses optional chaining (r.uri?.toString()) but the actual mapping at line 172 uses non-optional access (r.uri.toString()). This inconsistency could cause a runtime error if a resource is provided without a URI. Since IPromptFileResource.uri is marked as required (not optional) in the interface, the logging should match the code and use r.uri.toString() without the optional chaining operator.

Suggested change
this.logger.info(`[setCachedProviderResources] Setting cache for ${extensionId}/${type}, input resources:`, JSON.stringify(resources.map(r => ({ name: r.name, description: r.description, uri: r.uri?.toString(), isEditable: r.isEditable, isCacheable: r.isCacheable }))));
this.logger.info(`[setCachedProviderResources] Setting cache for ${extensionId}/${type}, input resources:`, JSON.stringify(resources.map(r => ({ name: r.name, description: r.description, uri: r.uri.toString(), isEditable: r.isEditable, isCacheable: r.isCacheable }))));

Copilot uses AI. Check for mistakes.
readonly isEditable?: boolean;

/**
* Indicates whether the editor should cache the URI on startup before activation. Defaults to true.
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation says "cache the URI on startup" but the implementation actually caches the entire resource metadata (name, description, uri, isEditable), not just the URI. Consider updating the documentation to say "cache the resource metadata on startup before activation" for accuracy.

Suggested change
* Indicates whether the editor should cache the URI on startup before activation. Defaults to true.
* Indicates whether the editor should cache the resource metadata on startup before activation. Defaults to true.

Copilot uses AI. Check for mistakes.
Comment on lines +163 to +179
private setCachedProviderResources(extensionId: string, type: PromptsType, resources: IPromptFileResource[]): void {
const key = `${this.cachedProviderResourcesStorageKeyPrefix}${extensionId}.${type}`;
this.logger.info(`[setCachedProviderResources] Setting cache for ${extensionId}/${type}, input resources:`, JSON.stringify(resources.map(r => ({ name: r.name, description: r.description, uri: r.uri?.toString(), isEditable: r.isEditable, isCacheable: r.isCacheable }))));
// Filter to only cacheable resources (isCacheable defaults to true)
const cacheableResources: ICachedPromptFileResource[] = resources
.filter(r => r.isCacheable !== false)
.map(r => ({
name: r.name,
description: r.description,
uri: r.uri.toString(),
isEditable: r.isEditable
}));
this.logger.info(`[setCachedProviderResources] Storing ${cacheableResources.length} cacheable resources for key=${key}:`, JSON.stringify(cacheableResources));
this.storageService.store(key, JSON.stringify(cacheableResources), StorageScope.PROFILE, StorageTarget.MACHINE);

// Track that this extension has registered a provider for this type
this.addCachedProviderExtension(extensionId, type);
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When all resources from a provider are non-cacheable (isCacheable=false), the implementation still stores an empty array in storage and tracks the extension as having cached resources. This is somewhat inefficient as it creates storage entries and tracks extensions even when there's nothing useful to cache. Consider only storing cache data and tracking extensions when there is at least one cacheable resource (e.g., check if cacheableResources.length > 0 before storing).

Copilot uses AI. Check for mistakes.
const result: IPromptPath[] = [];

// First, get cached resources for extensions that haven't activated yet
const cachedResources = await this.getCachedResourcesForUnactivatedExtensions(type);
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cached resources loaded before extension activation (line 419) are not having their readonly state applied via filesConfigService.updateReadonly(), unlike provider resources (lines 448-455). This means if a cached resource has isEditable=false, the file won't be marked as readonly until the extension activates and the provider returns it. Consider applying the readonly state to cached resources as well if they have isEditable=false stored in the cache.

Suggested change
const cachedResources = await this.getCachedResourcesForUnactivatedExtensions(type);
const cachedResources = await this.getCachedResourcesForUnactivatedExtensions(type);
for (const cached of cachedResources) {
const cachedExtensionPath = cached as IExtensionPromptPath & { isEditable?: boolean };
if (cachedExtensionPath.isEditable === false) {
try {
await this.filesConfigService.updateReadonly(cached.uri, true);
} catch (e) {
const msg = e instanceof Error ? e.message : String(e);
this.logger.error(`[listFromProviders] Failed to make cached file readonly: ${cached.uri}`, msg);
}
}
}

Copilot uses AI. Check for mistakes.
@pwang347 pwang347 closed this Jan 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants