Clean up customization discovery flow#304273
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the prompts/customizations discovery diagnostics flow by removing per-session discovery logging from IPromptsService methods and replacing it with a global “discovery changed” event plus on-demand discovery snapshots for the chat debug log.
Changes:
- Replace
onDidLogDiscovery+ session-scoped logging params withonDidDiscoveryChangeand a newgetDiscoveryInfo(type, token)API. - Update chat/debug bridge to provide an initial discovery snapshot per session and broadcast subsequent discovery changes to active debug sessions.
- Update call sites and tests to the new
IPromptsServicesurface (removingsessionResourceparams).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/contrib/chat/test/common/promptSyntax/service/mockPromptsService.ts | Updates test mock to new IPromptsService discovery APIs/events. |
| src/vs/workbench/contrib/chat/test/browser/promptsDebugContribution.test.ts | Reworks tests to validate snapshot + broadcast behavior. |
| src/vs/workbench/contrib/chat/common/tools/builtinTools/runSubagentTool.ts | Updates hooks retrieval call signature. |
| src/vs/workbench/contrib/chat/common/promptSyntax/service/promptsServiceImpl.ts | Removes session-scoped discovery logging; adds discovery change emission + getDiscoveryInfo. |
| src/vs/workbench/contrib/chat/common/promptSyntax/service/promptsService.ts | Updates IPromptsService contract for new discovery flow. |
| src/vs/workbench/contrib/chat/common/promptSyntax/computeAutomaticInstructions.ts | Adjusts prompts service calls to updated signatures. |
| src/vs/workbench/contrib/chat/common/chatService/chatServiceImpl.ts | Adjusts prompts service calls to updated signatures. |
| src/vs/workbench/contrib/chat/browser/promptsDebugContribution.ts | Implements snapshot provider + broadcasts discovery changes to active debug sessions. |
| src/vs/sessions/contrib/chat/browser/promptsService.ts | Updates override signature to match base findAgentSkills. |
| this._register(this.cachedSlashCommands.onDidChange(() => { | ||
| void this.getPromptSlashCommandDiscoveryInfo(CancellationToken.None).catch(() => undefined) | ||
| .then(discoveryInfo => { if (discoveryInfo) { this._onDidDiscoveryChange.fire({ type: PromptsType.prompt, discoveryInfo }); } }); | ||
| })); | ||
| this._register(this.cachedCustomAgents.onDidChange(() => { | ||
| void this.getAgentDiscoveryInfo(CancellationToken.None).catch(() => undefined) | ||
| .then(discoveryInfo => { if (discoveryInfo) { this._onDidDiscoveryChange.fire({ type: PromptsType.agent, discoveryInfo }); } }); | ||
| })); | ||
| this._register(this.cachedSkills.onDidChange(() => { | ||
| void this.getSkillDiscoveryInfo(CancellationToken.None).catch(() => undefined) | ||
| .then(discoveryInfo => { if (discoveryInfo) { this._onDidDiscoveryChange.fire({ type: PromptsType.skill, discoveryInfo }); } }); | ||
| })); | ||
| this._register(this.cachedHooks.onDidChange(() => { | ||
| void this.getHookDiscoveryInfo(CancellationToken.None).catch(() => undefined) | ||
| .then(discoveryInfo => { if (discoveryInfo) { this._onDidDiscoveryChange.fire({ type: PromptsType.hook, discoveryInfo }); } }); | ||
| })); | ||
| this._register(this.onDidChangeInstructions(() => { |
There was a problem hiding this comment.
The new cached*.onDidChange handlers recompute full discovery diagnostics (listPromptFiles + parseNew across all files) on every cache invalidation. Since these caches can invalidate on file/model/config changes, this can introduce significant background work even when no one is consuming discovery diagnostics. Consider debouncing per-type recomputation (e.g. Delayer + cancellation) and/or only recomputing when discovery info is actually needed (for example, when there are active debug sessions / a consumer has requested discovery data).
| this._register(this.cachedSlashCommands.onDidChange(() => { | |
| void this.getPromptSlashCommandDiscoveryInfo(CancellationToken.None).catch(() => undefined) | |
| .then(discoveryInfo => { if (discoveryInfo) { this._onDidDiscoveryChange.fire({ type: PromptsType.prompt, discoveryInfo }); } }); | |
| })); | |
| this._register(this.cachedCustomAgents.onDidChange(() => { | |
| void this.getAgentDiscoveryInfo(CancellationToken.None).catch(() => undefined) | |
| .then(discoveryInfo => { if (discoveryInfo) { this._onDidDiscoveryChange.fire({ type: PromptsType.agent, discoveryInfo }); } }); | |
| })); | |
| this._register(this.cachedSkills.onDidChange(() => { | |
| void this.getSkillDiscoveryInfo(CancellationToken.None).catch(() => undefined) | |
| .then(discoveryInfo => { if (discoveryInfo) { this._onDidDiscoveryChange.fire({ type: PromptsType.skill, discoveryInfo }); } }); | |
| })); | |
| this._register(this.cachedHooks.onDidChange(() => { | |
| void this.getHookDiscoveryInfo(CancellationToken.None).catch(() => undefined) | |
| .then(discoveryInfo => { if (discoveryInfo) { this._onDidDiscoveryChange.fire({ type: PromptsType.hook, discoveryInfo }); } }); | |
| })); | |
| this._register(this.onDidChangeInstructions(() => { | |
| this._register(this.cachedSlashCommands.onDidChange(() => { | |
| if (!this._onDidDiscoveryChange.hasListeners) { | |
| return; | |
| } | |
| void this.getPromptSlashCommandDiscoveryInfo(CancellationToken.None).catch(() => undefined) | |
| .then(discoveryInfo => { if (discoveryInfo) { this._onDidDiscoveryChange.fire({ type: PromptsType.prompt, discoveryInfo }); } }); | |
| })); | |
| this._register(this.cachedCustomAgents.onDidChange(() => { | |
| if (!this._onDidDiscoveryChange.hasListeners) { | |
| return; | |
| } | |
| void this.getAgentDiscoveryInfo(CancellationToken.None).catch(() => undefined) | |
| .then(discoveryInfo => { if (discoveryInfo) { this._onDidDiscoveryChange.fire({ type: PromptsType.agent, discoveryInfo }); } }); | |
| })); | |
| this._register(this.cachedSkills.onDidChange(() => { | |
| if (!this._onDidDiscoveryChange.hasListeners) { | |
| return; | |
| } | |
| void this.getSkillDiscoveryInfo(CancellationToken.None).catch(() => undefined) | |
| .then(discoveryInfo => { if (discoveryInfo) { this._onDidDiscoveryChange.fire({ type: PromptsType.skill, discoveryInfo }); } }); | |
| })); | |
| this._register(this.cachedHooks.onDidChange(() => { | |
| if (!this._onDidDiscoveryChange.hasListeners) { | |
| return; | |
| } | |
| void this.getHookDiscoveryInfo(CancellationToken.None).catch(() => undefined) | |
| .then(discoveryInfo => { if (discoveryInfo) { this._onDidDiscoveryChange.fire({ type: PromptsType.hook, discoveryInfo }); } }); | |
| })); | |
| this._register(this.onDidChangeInstructions(() => { | |
| if (!this._onDidDiscoveryChange.hasListeners) { | |
| return; | |
| } |
| created: new Date(), | ||
| name, | ||
| details, | ||
| level: 1 /* ChatDebugLogLevel.Info */, | ||
| category: 'discovery', |
There was a problem hiding this comment.
level: 1 /* ChatDebugLogLevel.Info */ hard-codes the enum value. Please use ChatDebugLogLevel.Info (imported from chatDebugService) to avoid a brittle magic number and keep this aligned if the enum changes.
| getInstructionFiles(_token: CancellationToken): Promise<readonly IPromptPath[]> { throw new Error('Method not implemented.'); } | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| getDiscoveryInfo(_type: any, _token: CancellationToken): Promise<any> { throw new Error('Method not implemented.'); } |
There was a problem hiding this comment.
MockPromptsService.getDiscoveryInfo is typed with any for both params and return. Since this is a mock implementing IPromptsService, it’s safer to match the real signature (type: PromptsType, returning Promise<IPromptDiscoveryInfo>) so tests fail at compile time if the interface changes.
|
Superceded by #307142 |
No description provided.