agentPlugins: normalize to user data dir storage#304977
agentPlugins: normalize to user data dir storage#304977connor4312 wants to merge 1 commit intomainfrom
Conversation
Previously we stored plugins in a very internal way that was inaccessible by other tooling. This sets up a `agent-plugins` folder beside `extensions` and creates an `installed.json` which is easy to integrate with.
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: @bpaseroMatched files:
|
There was a problem hiding this comment.
Pull request overview
This PR moves agent plugin storage to a well-known user-data location (agent-plugins beside extensions) and introduces an installed.json manifest to make installed plugins discoverable and integratable by external tooling.
Changes:
- Add
agentPluginsHometo the workbench environment andagentPluginsPathresolution (CLI/env/portable/default) for native. - Migrate plugin repositories from the legacy cache directory to the new
agent-pluginslocation and persist installs in a new file-backedinstalled.json. - Update the chat plugin marketplace/repository services and add/extend tests for migration + installed-plugin lifecycle.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/services/environment/electron-browser/environmentService.ts | Exposes native agentPluginsHome URI derived from the resolved native path. |
| src/vs/workbench/services/environment/common/environmentService.ts | Adds agentPluginsHome to IWorkbenchEnvironmentService. |
| src/vs/workbench/services/environment/browser/environmentService.ts | Implements agentPluginsHome for browser/web workbench. |
| src/vs/platform/environment/common/environmentService.ts | Adds native path resolution for agentPluginsPath via CLI/env/portable/default. |
| src/vs/platform/environment/common/argv.ts | Adds --agent-plugins-dir to native parsed args typing. |
| src/vs/workbench/contrib/chat/common/plugins/agentPluginRepositoryService.ts | Documents/exposes agentPluginsHome on the repository service API. |
| src/vs/workbench/contrib/chat/browser/agentPluginRepositoryService.ts | Switches repo cache root to agentPluginsHome and performs one-time directory migration. |
| src/vs/workbench/contrib/chat/common/plugins/fileBackedInstalledPluginsStore.ts | New file-backed observable store for installed.json plus legacy-storage migration. |
| src/vs/workbench/contrib/chat/common/plugins/pluginMarketplaceService.ts | Switches installed-plugin persistence to the file-backed store and adds metadata hydration. |
| src/vs/workbench/contrib/chat/test/browser/plugins/agentPluginRepositoryService.test.ts | Updates tests for new environment dependency and new home path. |
| src/vs/workbench/contrib/chat/test/common/plugins/pluginMarketplaceService.test.ts | Updates existing tests and adds lifecycle tests for installed plugin add/remove/metadata. |
| src/vs/workbench/contrib/chat/test/common/plugins/fileBackedInstalledPluginsStore.test.ts | New tests covering legacy migration, failure behavior, and reading existing installed.json. |
Comments suppressed due to low confidence (3)
src/vs/workbench/contrib/chat/test/common/plugins/pluginMarketplaceService.test.ts:269
- Same as above: the empty
{}IFileService stub will cause FileBackedInstalledPluginsStore initialization/write paths to throw (caught but noisy/flaky). Please stub the minimal file operations used by the store so these lifecycle tests aren’t relying on internal error handling.
}));
instantiationService.stub(IEnvironmentService, { cacheHome: URI.file('/cache') } as Partial<IEnvironmentService> as IEnvironmentService);
instantiationService.stub(IFileService, {} as unknown as IFileService);
instantiationService.stub(IAgentPluginRepositoryService, { agentPluginsHome: URI.file('/agent-plugins') } as unknown as IAgentPluginRepositoryService);
instantiationService.stub(ILogService, new NullLogService());
src/vs/workbench/contrib/chat/common/plugins/pluginMarketplaceService.ts:473
repoMayBePrivateis only updated when the GitHub request returns a non-200 status. If a marketplace.json is found (200) but declares zero plugins,plugins.lengthwill be 0 andrepoMayBePrivatewill still be true, causing an unnecessary clone-based fallback. Consider tracking “definition file found” separately (or settingrepoMayBePrivate = falseon any 200) and caching even empty plugin lists to avoid repeated network requests.
let repoMayBePrivate = true;
const plugins = await this._readPluginsFromDefinitions(reference, async (defPath) => {
if (token.isCancellationRequested) {
return undefined;
}
const url = `https://raw.githubusercontent.com/${repo}/main/${defPath}`;
try {
const context = await this._requestService.request({ type: 'GET', url, callSite: 'pluginMarketplaceService.fetchPluginList' }, token);
const statusCode = context.res.statusCode;
if (statusCode !== 200) {
repoMayBePrivate &&= statusCode !== undefined && statusCode >= 400 && statusCode < 500;
this._logService.debug(`[PluginMarketplaceService] ${url} returned status ${statusCode}, skipping`);
return undefined;
}
return await asJson<IMarketplaceJson>(context) ?? undefined;
} catch (err) {
this._logService.debug(`[PluginMarketplaceService] Failed to fetch marketplace.json from ${url}:`, err);
return undefined;
}
});
if (plugins.length > 0) {
cache.set(reference.canonicalId, {
plugins,
expiresAt: Date.now() + GITHUB_MARKETPLACE_CACHE_TTL_MS,
referenceRawValue: reference.rawValue,
});
this._savePersistedGitHubMarketplaceCache(cache);
return plugins;
}
if (repoMayBePrivate) {
this._logService.debug(`[PluginMarketplaceService] ${repo} may be private, attempting clone-based marketplace discovery`);
return this._fetchFromClonedRepo(reference, token);
}
this._logService.debug(`[PluginMarketplaceService] No marketplace.json found in ${repo}`);
return [];
src/vs/workbench/contrib/chat/common/plugins/fileBackedInstalledPluginsStore.ts:143
- Comment says “Each entry is { pluginUri: string, enabled: boolean }”, but entries are actually
{ pluginUri: string, marketplace: string }. Please fix this comment (and consider validating theversionfield while you’re here) so the file format stays clear.
const json: IInstalledJson = JSON.parse(content.value.toString());
if (!json || !Array.isArray(json.installed)) {
this._logService.warn('[FileBackedInstalledPluginsStore] installed.json has unexpected format, ignoring');
return undefined;
}
// Each entry is { pluginUri: string, enabled: boolean }.
return json.installed
.filter((entry): entry is IInstalledJsonEntry => typeof entry.pluginUri === 'string' && typeof entry.marketplace === 'string')
.map(entry => ({ pluginUri: URI.parse(entry.pluginUri), marketplace: entry.marketplace }));
} catch {
| })); | ||
| instantiationService.stub(IEnvironmentService, { cacheHome: URI.file('/cache') } as Partial<IEnvironmentService> as IEnvironmentService); | ||
| instantiationService.stub(IFileService, {} as unknown as IFileService); | ||
| instantiationService.stub(IAgentPluginRepositoryService, {} as unknown as IAgentPluginRepositoryService); | ||
| instantiationService.stub(IAgentPluginRepositoryService, { agentPluginsHome: URI.file('/agent-plugins') } as unknown as IAgentPluginRepositoryService); | ||
| instantiationService.stub(ILogService, new NullLogService()); |
There was a problem hiding this comment.
PluginMarketplaceService now constructs FileBackedInstalledPluginsStore, which calls fileService.exists/readFile/createFolder/writeFile. Stubbing IFileService with {} causes initialization failures and scheduled writes to throw (even if caught), which can make tests flaky. Provide a minimal file service stub implementing the methods the store uses (e.g. exists→false, createFolder/writeFile→no-op) so the service initializes cleanly.
This issue also appears on line 265 of the same file.
See below for a potential fix:
const fileServiceStub: Partial<IFileService> = {
exists: async () => false,
readFile: async resource => ({
resource,
value: undefined as any,
etag: ''
}),
createFolder: async () => undefined as any,
writeFile: async () => undefined as any
};
instantiationService.stub(IConfigurationService, new TestConfigurationService({
[ChatConfiguration.PluginMarketplaces]: ['microsoft/plugins'],
[ChatConfiguration.PluginsEnabled]: true,
}));
instantiationService.stub(IEnvironmentService, { cacheHome: URI.file('/cache') } as Partial<IEnvironmentService> as IEnvironmentService);
instantiationService.stub(IFileService, fileServiceStub as IFileService);
| }, | ||
| } as unknown as ICommandService); | ||
| instantiationService.stub(IEnvironmentService, { cacheHome: URI.file('/cache') } as unknown as IEnvironmentService); | ||
| instantiationService.stub(IEnvironmentService, { cacheHome: URI.file('/cache'), agentPluginsHome: URI.file('/cache/agentPlugins') } as unknown as IEnvironmentService); | ||
| instantiationService.stub(IFileService, fileService); |
There was a problem hiding this comment.
The constructor of AgentPluginRepositoryService now injects IWorkbenchEnvironmentService (not IEnvironmentService). Stubbing IEnvironmentService here will not satisfy DI, so these tests will fail at instantiation time. Stub IWorkbenchEnvironmentService instead (and update the import) with the needed properties (cacheHome, agentPluginsHome).
| 'extensions-dir'?: string; | ||
| 'extensions-download-dir'?: string; | ||
| 'builtin-extensions-dir'?: string; | ||
| 'agent-plugins-dir'?: string; | ||
| extensionDevelopmentPath?: string[]; // undefined or array of 1 or more local paths or URIs |
There was a problem hiding this comment.
--agent-plugins-dir is added to the parsed args type, but the CLI option table that drives parsing/validation (src/vs/platform/environment/node/argv.ts OPTIONS) also needs an entry for it. Without that, the parser will treat it as an unknown option and it will never reach this.args['agent-plugins-dir'].
| * On native this is `~/{dataFolderName}/agent-plugins/`; on web it | ||
| * falls back to `{cacheHome}/agentPlugins/`. |
There was a problem hiding this comment.
The JSDoc says the web implementation falls back to {cacheHome}/agentPlugins/, but the new BrowserWorkbenchEnvironmentService implementation returns a sibling of userRoamingDataHome (e.g. /agent-plugins). Please align this comment with the actual behavior, or adjust the implementation to match the documented path.
| * On native this is `~/{dataFolderName}/agent-plugins/`; on web it | |
| * falls back to `{cacheHome}/agentPlugins/`. | |
| * On native this is `~/{dataFolderName}/agent-plugins/`; on web it is | |
| * a sibling of `userRoamingDataHome` (for example `/agent-plugins`). |
| * The on-disk format stores only the plugin URI (as a string) and | ||
| * enablement state. Plugin metadata (name, description, marketplace | ||
| * provenance, etc.) is read from the plugin manifest on disk by the | ||
| * discovery layer — keeping a single source of truth. |
There was a problem hiding this comment.
The class-level docs describe storing “enablement state” in installed.json, but the on-disk schema and IInstalledJsonEntry contain pluginUri + marketplace only (no enabled flag). Please update the documentation to match the actual persisted fields to avoid confusion for external tool authors.
This issue also appears on line 133 of the same file.
| * The on-disk format stores only the plugin URI (as a string) and | |
| * enablement state. Plugin metadata (name, description, marketplace | |
| * provenance, etc.) is read from the plugin manifest on disk by the | |
| * discovery layer — keeping a single source of truth. | |
| * The on-disk format stores only the plugin URI (as a string) and the | |
| * marketplace identifier. Plugin metadata (name, description, etc.) is | |
| * read from the plugin manifest on disk by the discovery layer – | |
| * keeping a single source of truth. |
| @@ -168,7 +169,6 @@ export interface IPluginMarketplaceService { | |||
| getMarketplacePluginMetadata(pluginUri: URI): IMarketplacePlugin | undefined; | |||
| addInstalledPlugin(pluginUri: URI, plugin: IMarketplacePlugin): void; | |||
| removeInstalledPlugin(pluginUri: URI): void; | |||
| setInstalledPluginEnabled(pluginUri: URI, enabled: boolean): void; | |||
| /** Returns whether the given marketplace has been explicitly trusted by the user. */ | |||
There was a problem hiding this comment.
enabled was removed from IMarketplaceInstalledPlugin/installedPlugins, but there are still call sites that use it (e.g. src/vs/workbench/contrib/chat/browser/pluginInstallService.ts filters installedPlugins.get().filter(e => e.enabled)). As-is, this change will cause TypeScript compile errors and/or behavior changes. Either keep an enablement field in the installed plugin model (even if persisted elsewhere) or update the remaining consumers to use the new enablement source.
| try { | ||
| const repoDir = this._pluginRepositoryService.getRepositoryUri(reference); | ||
| const plugins = await this._readPluginsFromDirectory(repoDir, reference); | ||
| const match = plugins.find(p => { | ||
| const installUri = this._pluginRepositoryService.getPluginInstallUri(p); |
There was a problem hiding this comment.
_hydratePluginMetadata reads from getRepositoryUri() directly. On native startup, AgentPluginRepositoryService may still be migrating the old cache directory to agentPluginsHome (migration is only awaited in ensureRepository/ensurePluginSource). This can make hydration run against a non-existent repoDir and then never retry. Consider awaiting a repository-service “ready/migration” signal here, or using ensureRepository with a silent/no-clone mode before reading marketplace.json.
| // Touch the store to trigger the derived observable to re-evaluate | ||
| // now that _pluginMetadata has new entries. | ||
| const current = this._installedPluginsStore.get(); | ||
| this._installedPluginsStore.set([...current], undefined); |
There was a problem hiding this comment.
Touching the installed-plugins store via set([...current]) will schedule a write to installed.json, causing unnecessary disk I/O and potentially observable external churn on every startup hydration. Instead, make installedPlugins react to metadata changes (e.g., an observable version counter for _pluginMetadata) or add a store method to emit an in-memory change without scheduling a file write.
| this._installedPluginsStore.set([...current], undefined); | |
| this._installedPluginsStore.set(current, undefined); |
Previously we stored plugins in a very internal way that was inaccessible by other tooling. This sets up a
agent-pluginsfolder besideextensionsand creates aninstalled.jsonwhich is easy to integrate with.