Add support for skills/hooks sync by pwang347 · Pull Request #299363 · microsoft/vscode

pwang347 · 2026-03-05T02:35:24Z

No description provided.

Copilot

Pull request overview

Adds User Data Sync support for two new profile-scoped areas—Skills and Hooks—by extending the sync resource model, wiring new synchronisers into the sync service, and surfacing the resources in Workbench enablement/UI and profile import/export flows.

Changes:

Introduces SyncResource.Skills and SyncResource.Hooks and integrates them into sync resource lists, labels, enablement defaults, and workbench UI.
Adds new platform synchronisers for syncing Skills (recursive folder content) and Hooks (flat files).
Extends user data profile shape + related tests/import-export plumbing to include skillsHome and hooksHome.

Reviewed changes

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

Show a summary per file

File	Description
src/vs/workbench/services/workingCopy/test/electron-browser/workingCopyBackupService.test.ts	Updates test profile fixture to include `skillsHome`/`hooksHome`.
src/vs/workbench/services/userDataSync/common/userDataSync.ts	Adds localized sync-area labels for Skills/Hooks.
src/vs/workbench/services/userDataProfile/browser/userDataProfileImportExportService.ts	Includes Skills/Hooks homes in export scheme mapping.
src/vs/workbench/services/storage/test/browser/storageService.test.ts	Updates test profile fixture to include `skillsHome`/`hooksHome`.
src/vs/workbench/contrib/userDataSync/browser/userDataSync.ts	Adds Skills/Hooks to sync enablement UI and disable handling.
src/vs/workbench/contrib/editSessions/test/browser/editSessions.test.ts	Updates test profile fixture to include `skillsHome`/`hooksHome`.
src/vs/platform/userDataSync/common/userDataSyncService.ts	Registers Skills/Hooks synchronisers and assigns sync priority ordering.
src/vs/platform/userDataSync/common/userDataSyncResourceProvider.ts	Adds resource association + node-content resolution for Skills/Hooks.
src/vs/platform/userDataSync/common/userDataSyncEnablementService.ts	Defaults Skills/Hooks enablement to off (like Prompts).
src/vs/platform/userDataSync/common/userDataSync.ts	Adds new `SyncResource` values and includes them in `ALL_SYNC_RESOURCES`.
src/vs/platform/userDataSync/common/skillsSync/skillsSync.ts	New synchroniser to sync skill folders/files (recursive).
src/vs/platform/userDataSync/common/skillsSync/skillsMerge.ts	New merge logic for Skills content map.
src/vs/platform/userDataSync/common/hooksSync/hooksSync.ts	New synchroniser to sync hook files.
src/vs/platform/userDataSync/common/hooksSync/hooksMerge.ts	New merge logic for Hooks content map.
src/vs/platform/userDataProfile/common/userDataProfile.ts	Extends profile resources/types with `Skills`/`Hooks` and adds homes.
src/vs/platform/storage/test/electron-main/storageMainService.test.ts	Updates test profile fixture to include `skillsHome`/`hooksHome`.

Comments suppressed due to low confidence (1)

src/vs/platform/userDataSync/common/skillsSync/skillsSync.ts:479

updateRemoteSkills also derives the key via extUri.basename(...). For nested skill files this will delete/update the wrong remote entry and can corrupt the remote JSON map. Use the same full relative key that was used in the merge result/resource preview map when mutating newSkills.

		for (const { acceptResult, localResource, remoteResource, remoteChange } of resourcePreviews) {
			if (remoteChange !== Change.None) {
				const key = localResource ? this.extUri.basename(localResource) : this.extUri.basename(remoteResource);
				if (remoteChange === Change.Deleted) {
					delete newSkills[key];
				} else {
					newSkills[key] = acceptResult.content!;
				}
			}

Copilot · 2026-03-05T02:41:51Z

src/vs/platform/userDataSync/common/skillsSync/skillsSync.ts

+	private async updateLocalBackup(resourcePreviews: IFileResourcePreview[]): Promise<void> {
+		const local: IStringDictionary<IFileContent> = {};
+		for (const resourcePreview of resourcePreviews) {
+			if (resourcePreview.fileContent) {
+				local[this.extUri.basename(resourcePreview.localResource)] = resourcePreview.fileContent;
+			}
+		}
+		await this.backupLocal(JSON.stringify(this.toSkillContents(local)));
+	}
+
+	private async updateLocalSkills(resourcePreviews: ISkillsAcceptedResourcePreview[], force: boolean): Promise<void> {
+		for (const { fileContent, acceptResult, localResource, remoteResource, localChange } of resourcePreviews) {
+			if (localChange !== Change.None) {
+				const key = remoteResource ? this.extUri.basename(remoteResource) : this.extUri.basename(localResource);
+				const resource = this.extUri.joinPath(this.skillsFolder, key);
+
+				if (localChange === Change.Deleted) {
+					this.logService.trace(`${this.syncResourceLogLabel}: Deleting skill file...`, key);
+					await this.fileService.del(resource);
+					this.logService.info(`${this.syncResourceLogLabel}: Deleted skill file`, key);
+				} else if (localChange === Change.Added) {
+					this.logService.trace(`${this.syncResourceLogLabel}: Creating skill file...`, key);
+					await this.fileService.createFile(resource, VSBuffer.fromString(acceptResult.content!), { overwrite: force });
+					this.logService.info(`${this.syncResourceLogLabel}: Created skill file`, key);
+				} else {
+					this.logService.trace(`${this.syncResourceLogLabel}: Updating skill file...`, key);
+					await this.fileService.writeFile(resource, VSBuffer.fromString(acceptResult.content!), force ? undefined : fileContent!);
+					this.logService.info(`${this.syncResourceLogLabel}: Updated skill file`, key);
+				}


Skills are keyed by relative paths like "my-skill/subfolder/file" (per the class doc), but this code uses extUri.basename(...) when building the backup map and when computing the target local path. basename drops directory segments, which will cause collisions (e.g. two files named SKILL.md in different folders) and will write to the wrong location when applying changes. Preserve the full relative key (e.g. by carrying the merge key on the preview object, or by computing a relative path from the syncPreviewFolder) when backing up and applying local changes.

This issue also appears on line 471 of the same file.

Copilot · 2026-03-05T02:41:52Z

src/vs/platform/userDataSync/common/skillsSync/skillsSync.ts

+			const skillName = this.extUri.relativePath(this.skillsFolder, entry.resource)!;
+			const skillFiles = await this.readFolderRecursively(entry.resource, skillName);
+
+			// Check size limit for this skill
+			let totalSize = 0;
+			for (const content of Object.values(skillFiles)) {
+				totalSize += content.value.byteLength;
+			}
+
+			if (totalSize > MAX_SKILL_FOLDER_SIZE) {
+				this.logService.warn(`${this.syncResourceLogLabel}: Skipping skill "${skillName}" because its total size (${Math.round(totalSize / 1024)}KB) exceeds the ${Math.round(MAX_SKILL_FOLDER_SIZE / 1024)}KB limit`);
+				continue;
+			}
+
+			Object.assign(skills, skillFiles);


The size limit is checked only after reading all files in a skill into memory (readFolderRecursively reads every file, then you sum byteLength). For oversized skills this defeats the purpose of the limit and can cause large I/O + memory spikes. Consider tracking accumulated size while walking/reading and abort early once the threshold is exceeded (or using stat sizes where available).

Copilot · 2026-03-05T02:41:52Z

src/vs/platform/userDataSync/common/skillsSync/skillsSync.ts

+export class SkillsSynchroniser extends AbstractSynchroniser implements IUserDataSynchroniser {
+
+	protected readonly version: number = 1;
+	private readonly skillsFolder: URI;
+
+	constructor(
+		profile: IUserDataProfile,
+		collection: string | undefined,
+		@IEnvironmentService environmentService: IEnvironmentService,
+		@IFileService fileService: IFileService,
+		@IStorageService storageService: IStorageService,
+		@IUserDataSyncStoreService userDataSyncStoreService: IUserDataSyncStoreService,
+		@IUserDataSyncLocalStoreService userDataSyncLocalStoreService: IUserDataSyncLocalStoreService,
+		@IUserDataSyncLogService logService: IUserDataSyncLogService,
+		@IConfigurationService configurationService: IConfigurationService,
+		@IUserDataSyncEnablementService userDataSyncEnablementService: IUserDataSyncEnablementService,
+		@ITelemetryService telemetryService: ITelemetryService,
+		@IUriIdentityService uriIdentityService: IUriIdentityService,
+	) {
+		const syncResource = { syncResource: SyncResource.Skills, profile };
+		super(
+			syncResource,
+			collection,
+			fileService,
+			environmentService,
+			storageService,
+			userDataSyncStoreService,
+			userDataSyncLocalStoreService,
+			userDataSyncEnablementService,
+			telemetryService,
+			logService,
+			configurationService,
+			uriIdentityService,
+		);
+
+		this.skillsFolder = profile.skillsHome;
+		this._register(this.fileService.watch(environmentService.userRoamingDataHome));
+		this._register(this.fileService.watch(this.skillsFolder));
+		this._register(Event.filter(this.fileService.onDidFilesChange, e => e.affects(this.skillsFolder))(() => this.triggerLocalChange()));
+	}
+
+	protected async generateSyncPreview(remoteUserData: IRemoteUserData, lastSyncUserData: IRemoteUserData | null, isRemoteDataFromCurrentMachine: boolean): Promise<ISkillsResourcePreview[]> {
+		const local = await this.getSkillsFileContents();
+		const localSkills = this.toSkillContents(local);
+		const remoteSkills: IStringDictionary<string> | null = remoteUserData.syncData ? this.parseSkills(remoteUserData.syncData) : null;
+
+		lastSyncUserData = lastSyncUserData === null && isRemoteDataFromCurrentMachine ? remoteUserData : lastSyncUserData;
+		const lastSyncSkills: IStringDictionary<string> | null = lastSyncUserData && lastSyncUserData.syncData ? this.parseSkills(lastSyncUserData.syncData) : null;
+
+		if (remoteSkills) {
+			this.logService.trace(`${this.syncResourceLogLabel}: Merging remote skills with local skills...`);
+		} else {
+			this.logService.trace(`${this.syncResourceLogLabel}: Remote skills does not exist. Synchronizing skills for the first time.`);
+		}
+
+		const mergeResult = merge(localSkills, remoteSkills, lastSyncSkills);
+		return this.getResourcePreviews(mergeResult, local, remoteSkills || {}, lastSyncSkills || {});
+	}
+


There are sync tests for other resources (e.g. PromptsSync under src/vs/platform/userDataSync/test/common/), but this new synchroniser doesn’t appear to have corresponding coverage. Adding tests for basic scenarios (initial upload, remote->local, local->remote, conflict, and nested path handling) would help prevent regressions.

Copilot · 2026-03-05T02:41:52Z

src/vs/platform/userDataSync/common/hooksSync/hooksSync.ts

+export class HooksSynchroniser extends AbstractSynchroniser implements IUserDataSynchroniser {
+
+	protected readonly version: number = 1;
+	private readonly hooksFolder: URI;
+
+	constructor(
+		profile: IUserDataProfile,
+		collection: string | undefined,
+		@IEnvironmentService environmentService: IEnvironmentService,
+		@IFileService fileService: IFileService,
+		@IStorageService storageService: IStorageService,
+		@IUserDataSyncStoreService userDataSyncStoreService: IUserDataSyncStoreService,
+		@IUserDataSyncLocalStoreService userDataSyncLocalStoreService: IUserDataSyncLocalStoreService,
+		@IUserDataSyncLogService logService: IUserDataSyncLogService,
+		@IConfigurationService configurationService: IConfigurationService,
+		@IUserDataSyncEnablementService userDataSyncEnablementService: IUserDataSyncEnablementService,
+		@ITelemetryService telemetryService: ITelemetryService,
+		@IUriIdentityService uriIdentityService: IUriIdentityService,
+	) {
+		const syncResource = { syncResource: SyncResource.Hooks, profile };
+		super(
+			syncResource,
+			collection,
+			fileService,
+			environmentService,
+			storageService,
+			userDataSyncStoreService,
+			userDataSyncLocalStoreService,
+			userDataSyncEnablementService,
+			telemetryService,
+			logService,
+			configurationService,
+			uriIdentityService,
+		);
+
+		this.hooksFolder = profile.hooksHome;
+		this._register(this.fileService.watch(environmentService.userRoamingDataHome));
+		this._register(this.fileService.watch(this.hooksFolder));
+		this._register(Event.filter(this.fileService.onDidFilesChange, e => e.affects(this.hooksFolder))(() => this.triggerLocalChange()));
+	}
+
+	protected async generateSyncPreview(remoteUserData: IRemoteUserData, lastSyncUserData: IRemoteUserData | null, isRemoteDataFromCurrentMachine: boolean): Promise<IHooksResourcePreview[]> {
+		const local = await this.getHooksFileContents();
+		const localHooks = this.toHookContents(local);
+		const remoteHooks: IStringDictionary<string> | null = remoteUserData.syncData ? this.parseHooks(remoteUserData.syncData) : null;
+
+		lastSyncUserData = lastSyncUserData === null && isRemoteDataFromCurrentMachine ? remoteUserData : lastSyncUserData;
+		const lastSyncHooks: IStringDictionary<string> | null = lastSyncUserData && lastSyncUserData.syncData ? this.parseHooks(lastSyncUserData.syncData) : null;
+
+		if (remoteHooks) {
+			this.logService.trace(`${this.syncResourceLogLabel}: Merging remote hooks with local hooks...`);
+		} else {
+			this.logService.trace(`${this.syncResourceLogLabel}: Remote hooks does not exist. Synchronizing hooks for the first time.`);
+		}
+
+		const mergeResult = merge(localHooks, remoteHooks, lastSyncHooks);
+		return this.getResourcePreviews(mergeResult, local, remoteHooks || {}, lastSyncHooks || {});
+	}
+
+	protected async hasRemoteChanged(lastSyncUserData: IRemoteUserData): Promise<boolean> {
+		const lastSync: IStringDictionary<string> | null = lastSyncUserData.syncData ? this.parseHooks(lastSyncUserData.syncData) : null;
+		if (lastSync === null) {
+			return true;
+		}
+		const local = await this.getHooksFileContents();
+		const localHooks = this.toHookContents(local);
+		const mergeResult = merge(localHooks, lastSync, lastSync);
+		return Object.keys(mergeResult.remote.added).length > 0 || Object.keys(mergeResult.remote.updated).length > 0 || mergeResult.remote.removed.length > 0 || mergeResult.conflicts.length > 0;
+	}
+


There are sync tests for other resources (e.g. PromptsSync under src/vs/platform/userDataSync/test/common/), but this new synchroniser doesn’t appear to have corresponding coverage. Adding tests for upload/download/delete and conflict cases would help ensure hooks sync behaves consistently with prompts/snippets sync.

wip

c36ae73

Copilot AI review requested due to automatic review settings March 5, 2026 02:35

Copilot started reviewing on behalf of pwang347 March 5, 2026 02:37 View session

Copilot AI reviewed Mar 5, 2026

View reviewed changes

vs-code-engineering bot assigned pwang347 Mar 5, 2026

austindyoung mentioned this pull request Mar 5, 2026

[mirror] microsoft/vscode#299363 Add support for skills/hooks sync austindyoung/vscode-driftfence-mirror-fork#556

Open

wip

7adf2ee

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for skills/hooks sync#299363

Add support for skills/hooks sync#299363
pwang347 wants to merge 2 commits intomainfrom
pawang/skillsAndHooksSync

pwang347 commented Mar 5, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 5, 2026

Uh oh!

Copilot AI Mar 5, 2026

Uh oh!

Copilot AI Mar 5, 2026

Uh oh!

Copilot AI Mar 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

pwang347 commented Mar 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants