Add General Purpose agent support behind experimental setting#306871
Add General Purpose agent support behind experimental setting#306871digitarald merged 9 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a built-in “General Purpose” subagent pathway to the runSubagent tool and surfaces it in automatic agent instructions, gated behind the chat.generalPurposeAgent experiment (resolved via IWorkbenchAssignmentService).
Changes:
- Add
GeneralPurposeAgentNameconstant and integrate GP routing inRunSubagentTool(schema + invocation behavior). - Update
ComputeAutomaticInstructionsto include GP agent first (when experiment is enabled) and append custom agents after. - Refactor
PromptsServicecaching/logging and update/extend tests for GP agent behavior and prompt parsing expectations.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/contrib/chat/common/constants.ts | Adds shared GeneralPurposeAgentName constant for GP agent references. |
| src/vs/workbench/contrib/chat/common/tools/builtinTools/runSubagentTool.ts | Introduces experiment-gated GP behavior and updates tool schema/update events accordingly. |
| src/vs/workbench/contrib/chat/common/promptSyntax/computeAutomaticInstructions.ts | Renders GP agent entry first (when enabled) in the <agents> instruction block. |
| src/vs/workbench/contrib/chat/common/promptSyntax/service/promptsServiceImpl.ts | Refactors cached results vs discovery-info logging and custom agent/skill/slash command computations. |
| src/vs/workbench/contrib/chat/test/common/tools/builtinTools/runSubagentTool.test.ts | Adds GP-path tests and updates tool construction for new DI shape. |
| src/vs/workbench/contrib/chat/test/common/promptSyntax/computeAutomaticInstructions.test.ts | Adds coverage asserting GP agent appears first when experiment is enabled. |
| src/vs/workbench/contrib/chat/test/common/promptSyntax/service/promptsService.test.ts | Updates expectations for parsed variable references and tool-reference ranges. |
Comments suppressed due to low confidence (2)
src/vs/workbench/contrib/chat/common/promptSyntax/service/promptsServiceImpl.ts:715
getAgentDiscoveryInfo()is recomputed just for discovery logging, which can significantly increase work forgetCustomAgents()(re-parsing agent files) wheneversessionResourceis provided. Please guard this behindthis._onDidLogDiscovery.hasListeners()or restructure caching so logging can reuse the already-computed results.
public async getCustomAgents(token: CancellationToken, sessionResource?: URI): Promise<readonly ICustomAgent[]> {
const sw = StopWatch.create();
const result = await this.cachedCustomAgents.get(token);
if (sessionResource) {
const elapsed = sw.elapsed();
void this.getAgentDiscoveryInfo(token).catch(() => undefined).then(discoveryInfo => {
const details = result.length === 1
? localize("promptsService.resolvedAgent", "Resolved {0} agent in {1}ms", result.length, elapsed.toFixed(1))
: localize("promptsService.resolvedAgents", "Resolved {0} agents in {1}ms", result.length, elapsed.toFixed(1));
this._onDidLogDiscovery.fire({
sessionResource,
name: localize("promptsService.loadAgents", "Load Agents"),
details,
discoveryInfo,
category: 'discovery',
});
});
src/vs/workbench/contrib/chat/common/promptSyntax/service/promptsServiceImpl.ts:1125
getSkillDiscoveryInfo()is recomputed purely for discovery logging insidefindAgentSkills(), which can double skill parsing and folder diagnostics work wheneversessionResourceis provided. Consider guarding the extra computation behindthis._onDidLogDiscovery.hasListeners()or caching the discovery info with the computed skills.
const result = await this.cachedSkills.get(token);
if (sessionResource) {
const elapsed = sw.elapsed();
void this.getSkillDiscoveryInfo(token).catch(() => undefined).then(discoveryInfo => {
const details = result.length === 1
? localize("promptsService.resolvedSkill", "Resolved {0} skill in {1}ms", result.length, elapsed.toFixed(1))
: localize("promptsService.resolvedSkills", "Resolved {0} skills in {1}ms", result.length, elapsed.toFixed(1));
this._onDidLogDiscovery.fire({
sessionResource,
name: localize("promptsService.loadSkills", "Load Skills"),
details,
discoveryInfo,
category: 'discovery',
});
});
aeschli
left a comment
There was a problem hiding this comment.
Instead of the IWorkbenchAssignmentService don't we just use settings that then are controlled by experiments?
Felt like an odd setting to add, as there is uncertain user value in toggling it on or off. I could make it a hidden internal setting, which also makes testing it easier. |
|
Yes, lets add an internal setting. From what I understand there's the convention that all experiments are now backed by a setting. |
Technically, there's no difference between current empty argument and the new general purpose. I keep getting reports that the Explorer subagent is overused, and also that the subagents are not automatically used for parallel implementation. Both issues I hope to see improve by giving the default agent a named entry. |
Add a built-in 'General Purpose' agent to the runSubagent tool, gated behind the 'chat.generalPurposeAgent' experiment treatment: - Add GeneralPurposeAgentName constant - Make agentName required and route undefined/GP names to built-in agent - Render GP agent in automatic instructions agents block - Clean up duplicate DI injection in RunSubagentTool - Add unit tests for GP agent paths
…h, deterministic tests - Replace unsafe 'configEvent as Event<void>' cast with dedicated Emitter (fixes tsgo typecheck CI failure) - Fire onDidUpdateToolData when experiment resolution changes the value - Add try/catch around getTreatment in computeAutomaticInstructions - Replace flaky setTimeout(0) in tests with Event.toPromise(onDidUpdateToolData)
…ulation The merge conflict resolution incorrectly replaced fullLength with name.length + 1 for toolReference OffsetRange calculation, and removed fullLength from variableReferences test expectations. Restore the original behavior from main.
- Add error handler to _resolveExperiment() preventing unhandled promise rejections when getTreatment fails - Decouple GP agent from SubagentToolCustomAgents config gate so experiment works independently of custom agents setting - Fix redundant parens on Event listener arrow function - Add test for GP agent rendering without custom agents config
Reset promptsServiceImpl.ts and promptsService.test.ts back to main. These files contained an unrelated refactor (method renames, type simplifications) that was accidentally carried over during the port from PR #295494.
Replace direct IWorkbenchAssignmentService.getTreatment checks with
an experimental configuration setting chat.generalPurposeAgent.enabled.
This simplifies runtime code from async experiment resolution to
synchronous configurationService.getValue() calls.
- Add GeneralPurposeAgentEnabled to ChatConfiguration enum
- Register setting with experiment: { mode: 'auto' }, default: false
- Remove IWorkbenchAssignmentService DI from RunSubagentTool and
ComputeAutomaticInstructions (zero other usages in either file)
- Make GP agent error hint conditional on setting being enabled
- Update tests to use config-based setup instead of assignment stubs
75a0c2d to
1eb1701
Compare
|
Can we remove the There's also the suggestion that we make the name of the subagentTool mandatory. We could then mention in the tool description that 'General Agent' is a good default. What do you think? Is the 'General Agent' a custom agent defined in the Copilot extension? How would core know about it? Maybe the setting could be to give the default agent's name? |
|
I approved the PR. Open issues:
|
Add a built-in "General Purpose" agent to the
runSubagenttool, gated behind thechat.generalPurposeAgent.enabledexperimental setting.Based on #295494, rebased and adapted to the current codebase.
Changes
constants.tsGeneralPurposeAgentName = 'General Purpose'constantGeneralPurposeAgentEnabled = 'chat.generalPurposeAgent.enabled'toChatConfigurationenumchat.contribution.tschat.generalPurposeAgent.enabledsetting withtype: 'boolean',default: false,tags: ['experimental', 'advanced'],experiment: { mode: 'auto' }advancedtag hides the setting from the Settings UI by default on StablerunSubagentTool.tsIWorkbenchAssignmentServiceexperiment checks with synchronousconfigurationService.getValue(ChatConfiguration.GeneralPurposeAgentEnabled)reads_generalPurposeAgentEnabledfield,_resolveExperiment()method, andonDidRefetchAssignmentslisteneragentNameparameter only appears in the tool schema whenSubagentToolCustomAgentsorGeneralPurposeAgentEnabledis enabled; required when GP is enabledundefined/empty/"General Purpose"agent names are normalized toeffectiveSubAgentName = GeneralPurposeAgentName, used consistently inagentRequest.subAgentNameandtoolMetadata.agentNamegetSubAgentByName()lookups are gated behindSubagentToolCustomAgentssetting in bothinvoke()andprepareToolInvocation()onDidUpdateToolDatafires on config changes for bothSubagentToolCustomAgentsandGeneralPurposeAgentEnabledcomputeAutomaticInstructions.tsIWorkbenchAssignmentServiceinjection (replaced asyncgetTreatmentwith synchronousconfigurationService.getValue)Tests
IWorkbenchAssignmentServicestubs andNullWorkbenchAssignmentServiceusage from GP-related testsTestConfigurationService({ [ChatConfiguration.GeneralPurposeAgentEnabled]: true })SubagentToolCustomAgentsviaTestConfigurationServiceagentNameis absent when neither GP nor custom agents is enabledawait Event.toPromise(tool.onDidUpdateToolData)waits (no longer async)Decisions
chat.generalPurposeAgentintentionally dropped — this PR has not been merged yet, so no backward compatibility neededexperiment: { mode: 'auto' }so experiments can still override the default value['experimental', 'advanced']— hidden by default on Stable, visible on InsidersagentNameschema gated behindcustomAgentsEnabled || gpEnabledto avoid exposing the parameter when neither feature is activeSubagentToolCustomAgentsto prevent resolving agents when that setting is off