new settings: includeReferencedInstructions & includeApplyingInstructions#289731
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces two new configuration settings to control the automatic inclusion of instruction files in chat requests: INCLUDE_APPLYING_INSTRUCTIONS and INCLUDE_REFERENCED_INSTRUCTIONS. The implementation adds an IChatMode parameter to the ComputeAutomaticInstructions constructor to allow bypassing these settings in Edit mode, where instructions should always be included.
Changes:
- Added two new boolean configuration settings with defaults (
INCLUDE_APPLYING_INSTRUCTIONS: true,INCLUDE_REFERENCED_INSTRUCTIONS: false) - Updated
ComputeAutomaticInstructionsconstructor to acceptIChatModeas the first parameter - Added comprehensive test suite in a new test file
computeAutomaticInstructions.test.ts - Refactored skill search implementation in
promptFilesLocator.tsfrom search-based to file-service-based traversal - Updated all call sites to pass the chat mode parameter
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/contrib/chat/common/promptSyntax/config/config.ts | Defines the two new configuration keys for controlling instruction inclusion |
| src/vs/workbench/contrib/chat/browser/chat.contribution.ts | Registers the new settings in the configuration registry with appropriate defaults and descriptions |
| src/vs/workbench/contrib/chat/common/promptSyntax/computeAutomaticInstructions.ts | Adds chat mode parameter to constructor and implements logic to respect the new settings while bypassing them for Edit mode |
| src/vs/workbench/contrib/chat/browser/widget/chatWidget.ts | Updates constructor call to pass current chat mode |
| src/vs/workbench/contrib/chat/common/tools/builtinTools/runSubagentTool.ts | Updates constructor call to pass chat mode, with fallback to Agent mode |
| src/vs/workbench/contrib/chat/test/common/promptSyntax/service/promptsService.test.ts | Updates test calls to include the new chat mode parameter |
| src/vs/workbench/contrib/chat/test/common/promptSyntax/computeAutomaticInstructions.test.ts | New comprehensive test suite covering all instruction collection scenarios including the new settings |
| src/vs/workbench/contrib/chat/common/promptSyntax/utils/promptFilesLocator.ts | Refactors skill file search from search-service-based to file-service-based directory traversal; fixes trailing backtick in comment |
Comments suppressed due to low confidence (3)
src/vs/workbench/contrib/chat/common/promptSyntax/computeAutomaticInstructions.ts:68
- According to VS Code coding guidelines, dependency injection parameters (those prefixed with @) must come after all non-service parameters. The constructor has been changed to add
_agentas the first parameter, but it should come after the service parameters@IPromptsService,@ILogService, etc. The current parameter order violates this convention.
To fix this, move the _agent, _enabledTools, and _enabledSubagents parameters to come before the service parameters.
constructor(
private readonly _agent: IChatMode,
private readonly _enabledTools: UserSelectedTools | undefined,
private readonly _enabledSubagents: (readonly string[]) | undefined,
@IPromptsService private readonly _promptsService: IPromptsService,
@ILogService public readonly _logService: ILogService,
@ILabelService private readonly _labelService: ILabelService,
@IConfigurationService private readonly _configurationService: IConfigurationService,
@IWorkspaceContextService private readonly _workspaceService: IWorkspaceContextService,
@IFileService private readonly _fileService: IFileService,
@ITelemetryService private readonly _telemetryService: ITelemetryService,
@ILanguageModelToolsService private readonly _languageModelToolsService: ILanguageModelToolsService,
) {
src/vs/workbench/contrib/chat/test/common/promptSyntax/computeAutomaticInstructions.test.ts:265
- The assertion message is misleading. When the condition is false (copilot-instructions should NOT be included), the message says 'Should include copilot-instructions'. The assertion message should be 'Should NOT include copilot-instructions' to accurately describe what is being tested.
assert.ok(!paths.includes(`${rootFolder}/.github/copilot-instructions.md`), 'Should include copilot-instructions');
src/vs/workbench/contrib/chat/test/common/promptSyntax/computeAutomaticInstructions.test.ts:283
- The assertion message is misleading. When the condition is false (AGENTS.md should NOT be included), the message says 'Should include AGENTS.md'. The assertion message should be 'Should NOT include AGENTS.md' to accurately describe what is being tested.
assert.ok(!paths.includes(`${rootFolder}/AGENTS.md`), 'Should include AGENTS.md');
| const instructionFiles = variables.asArray().filter(v => isPromptFileVariableEntry(v)); | ||
| const paths = instructionFiles.map(i => isPromptFileVariableEntry(i) ? i.value.path : undefined); | ||
|
|
||
| assert.ok(!paths.includes(`${rootFolder}/.github/instructions/typescript.instructions.md`), 'Should include applying instruction'); |
There was a problem hiding this comment.
The assertion message is misleading. When the condition is false (the instruction should NOT be included), the message says 'Should include applying instruction'. The assertion message should be 'Should NOT include applying instruction' to accurately describe what is being tested.
This issue also appears in the following locations of the same file:
- line 265
- line 283
Fixes #282422