Skip to content

Commit 931ec3c

Browse files
aeschliaeschli
authored
Ignore tools when mode is set, improve diagnostic messages (microsoft#253031)
* Ignore tools when mode is set, improve diagnostic messages * fix test
1 parent 6943e61 commit 931ec3c

File tree

10 files changed

+48
-73
lines changed

10 files changed

+48
-73
lines changed

‎src/vs/workbench/contrib/chat/browser/chatWidget.ts

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@
66
import * as dom from '../../../../base/browser/dom.js';
77
import { Button } from '../../../../base/browser/ui/button/button.js';
88
import { ITreeContextMenuEvent, ITreeElement } from '../../../../base/browser/ui/tree/tree.js';
9-
import { assert } from '../../../../base/common/assert.js';
109
import { disposableTimeout, timeout } from '../../../../base/common/async.js';
1110
import { Codicon } from '../../../../base/common/codicons.js';
1211
import { toErrorMessage } from '../../../../base/common/errorMessage.js';
@@ -1894,14 +1893,7 @@ export class ChatWidget extends Disposable implements IChatWidget {
18941893
}
18951894

18961895
// if not tools to enable are present, we are done
1897-
if (tools !== undefined) {
1898-
1899-
1900-
// sanity check on the logic of the `getPromptFilesMetadata` method
1901-
// and the code above in case this block is moved around somewhere else:
1902-
// if we have some tools present, the mode must have been equal to `agent`
1903-
assert(this.input.currentModeKind === ChatModeKind.Agent, `Chat mode must be 'agent' when there are 'tools' defined, got ${this.input.currentModeKind}.`);
1904-
1896+
if (tools !== undefined && this.input.currentModeKind === ChatModeKind.Agent) {
19051897
const enablementMap = this.toolsService.toToolAndToolSetEnablementMap(new Set(tools));
19061898
this.input.selectedToolsModel.set(enablementMap, true);
19071899
}

‎src/vs/workbench/contrib/chat/common/promptSyntax/parsers/basePromptParser.ts

Lines changed: 4 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -553,30 +553,23 @@ export class BasePromptParser<TContentsProvider extends IPromptContentsProvider>
553553

554554
const { tools, mode, description, model } = this.header.metadata;
555555

556-
// compute resulting mode based on presence
557-
// of `tools` metadata in the prompt header
558-
const resultingMode = (tools !== undefined)
559-
? ChatModeKind.Agent
560-
: mode;
561-
562556
const result: Partial<TPromptMetadata> = {};
563557

564558
if (description !== undefined) {
565559
result.description = description;
566560
}
567561

568-
if (tools !== undefined) {
562+
if (tools !== undefined && mode !== ChatModeKind.Ask && mode !== ChatModeKind.Edit) {
569563
result.tools = tools;
564+
result.mode = ChatModeKind.Agent;
565+
} else if (mode !== undefined) {
566+
result.mode = mode;
570567
}
571568

572569
if (model !== undefined) {
573570
result.model = model;
574571
}
575572

576-
if (resultingMode !== undefined) {
577-
result.mode = resultingMode;
578-
}
579-
580573
return { promptType, ...result };
581574
}
582575

‎src/vs/workbench/contrib/chat/common/promptSyntax/parsers/promptHeader/headerBase.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import { type TModeMetadata } from './modeHeader.js';
77
import { localize } from '../../../../../../../nls.js';
88
import { type TPromptMetadata } from './promptHeader.js';
99
import { type IMetadataRecord } from './metadata/base/record.js';
10-
import { PromptDescriptionMetadata } from './metadata/index.js';
1110
import { type TInstructionsMetadata } from './instructionsHeader.js';
1211
import { Range } from '../../../../../../../editor/common/core/range.js';
1312
import { Disposable } from '../../../../../../../base/common/lifecycle.js';
@@ -17,6 +16,7 @@ import { SimpleToken } from '../../codecs/base/simpleCodec/tokens/tokens.js';
1716
import { FrontMatterRecord } from '../../codecs/base/frontMatterCodec/tokens/index.js';
1817
import { FrontMatterHeader } from '../../codecs/base/markdownExtensionsCodec/tokens/frontMatterHeader.js';
1918
import { FrontMatterDecoder, type TFrontMatterToken } from '../../codecs/base/frontMatterCodec/frontMatterDecoder.js';
19+
import { PromptDescriptionMetadata } from './metadata/description.js';
2020

2121
/**
2222
* A metadata utility class "dehydrated" into a plain data object with
@@ -189,7 +189,7 @@ export abstract class HeaderBase<
189189
token.range,
190190
localize(
191191
'prompt.header.metadata.diagnostics.duplicate-record',
192-
"Duplicate metadata '{0}' will be ignored.",
192+
"Duplicate property '{0}' will be ignored.",
193193
recordName,
194194
),
195195
),
@@ -222,7 +222,7 @@ export abstract class HeaderBase<
222222
token.range,
223223
localize(
224224
'prompt.header.metadata.diagnostics.unknown-record',
225-
"Unknown metadata '{0}' will be ignored.",
225+
"Unknown property '{0}' will be ignored.",
226226
recordName,
227227
),
228228
),

‎src/vs/workbench/contrib/chat/common/promptSyntax/parsers/promptHeader/metadata/applyTo.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ export class PromptApplyToMetadata extends PromptStringMetadata {
4646
this.range,
4747
localize(
4848
'prompt.header.metadata.string.diagnostics.invalid-language',
49-
"The '{0}' metadata record is only valid in instruction files.",
49+
"The '{0}' header property is only valid in instruction files.",
5050
this.recordName,
5151
),
5252
),

‎src/vs/workbench/contrib/chat/common/promptSyntax/parsers/promptHeader/metadata/index.ts

Lines changed: 0 additions & 8 deletions
This file was deleted.

‎src/vs/workbench/contrib/chat/common/promptSyntax/parsers/promptHeader/metadata/tools.ts

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -70,8 +70,7 @@ export class PromptToolsMetadata extends PromptMetadataRecord<string[]> {
7070
valueToken.range,
7171
localize(
7272
'prompt.header.metadata.tools.diagnostics.invalid-value-type',
73-
"The '{0}' metadata must be an array of tool names, got '{2}'.",
74-
RECORD_NAME,
73+
"Must be an array of tool names, got '{0}'.",
7574
valueToken.valueTypeName.toString(),
7675
),
7776
),
@@ -118,9 +117,8 @@ export class PromptToolsMetadata extends PromptMetadataRecord<string[]> {
118117
valueToken.range,
119118
localize(
120119
'prompt.header.metadata.tools.diagnostics.invalid-tool-name-type',
121-
"Unexpected tool name '{0}', expected '{1}'.",
122-
valueToken.text,
123-
'string',
120+
"Unexpected tool name '{0}', expected a string literal.",
121+
valueToken.text
124122
),
125123
),
126124
);

‎src/vs/workbench/contrib/chat/common/promptSyntax/parsers/promptHeader/modeHeader.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,11 @@
33
* Licensed under the MIT License. See License.txt in the project root for license information.
44
*--------------------------------------------------------------------------------------------*/
55

6-
import { PromptToolsMetadata } from './metadata/index.js';
76
import { HeaderBase, IHeaderMetadata, type TDehydrated } from './headerBase.js';
87
import { PromptsType } from '../../promptTypes.js';
98
import { FrontMatterRecord } from '../../codecs/base/frontMatterCodec/tokens/index.js';
109
import { PromptModelMetadata } from './metadata/model.js';
10+
import { PromptToolsMetadata } from './metadata/tools.js';
1111

1212
/**
1313
* Metadata utility object for mode files.

‎src/vs/workbench/contrib/chat/common/promptSyntax/parsers/promptHeader/promptHeader.ts

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,13 @@ import { localize } from '../../../../../../../nls.js';
88
import { PromptMetadataWarning } from './diagnostics.js';
99
import { assert } from '../../../../../../../base/common/assert.js';
1010
import { assertDefined } from '../../../../../../../base/common/types.js';
11-
import { PromptToolsMetadata, PromptModeMetadata } from './metadata/index.js';
11+
1212
import { HeaderBase, IHeaderMetadata, type TDehydrated } from './headerBase.js';
1313
import { PromptsType } from '../../promptTypes.js';
1414
import { FrontMatterRecord } from '../../codecs/base/frontMatterCodec/tokens/index.js';
1515
import { PromptModelMetadata } from './metadata/model.js';
16+
import { PromptToolsMetadata } from './metadata/tools.js';
17+
import { PromptModeMetadata } from './metadata/mode.js';
1618

1719
/**
1820
* Metadata utility object for prompt files.
@@ -134,10 +136,8 @@ export class PromptHeader extends HeaderBase<IPromptMetadata> {
134136
mode.range,
135137
localize(
136138
'prompt.header.metadata.mode.diagnostics.incompatible-with-tools',
137-
"Record '{0}' is implied to have the '{1}' value if '{2}' record is present so the specified value will be ignored.",
138-
mode.recordName,
139-
ChatModeKind.Agent,
140-
tools.recordName,
139+
"Tools can only be used when in 'agent' mode, but the mode is set to '{0}'. The tools will be ignored.",
140+
mode.value
141141
),
142142
),
143143
);

‎src/vs/workbench/contrib/chat/test/common/promptSyntax/parsers/textModelPromptParser.test.ts

Lines changed: 29 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -653,8 +653,7 @@ suite('TextModelPromptParser', () => {
653653
metadata,
654654
{
655655
promptType: PromptsType.prompt,
656-
mode: 'agent',
657-
tools: ['tool_name1', 'tool_name2'],
656+
mode: 'ask',
658657
},
659658
'Must have correct metadata.',
660659
);
@@ -666,19 +665,19 @@ suite('TextModelPromptParser', () => {
666665
),
667666
new ExpectedDiagnosticWarning(
668667
new Range(4, 2, 4, 2 + 15),
669-
'Unknown metadata \'something\' will be ignored.',
668+
'Unknown property \'something\' will be ignored.',
670669
),
671670
new ExpectedDiagnosticWarning(
672671
new Range(5, 38, 5, 38 + 12),
673672
'Duplicate tool name \'tool_name1\'.',
674673
),
675674
new ExpectedDiagnosticWarning(
676675
new Range(5, 52, 5, 52 + 4),
677-
'Unexpected tool name \'true\', expected \'string\'.',
676+
'Unexpected tool name \'true\', expected a string literal.',
678677
),
679678
new ExpectedDiagnosticWarning(
680679
new Range(5, 58, 5, 58 + 5),
681-
'Unexpected tool name \'false\', expected \'string\'.',
680+
'Unexpected tool name \'false\', expected a string literal.',
682681
),
683682
new ExpectedDiagnosticWarning(
684683
new Range(5, 65, 5, 65 + 2),
@@ -690,15 +689,15 @@ suite('TextModelPromptParser', () => {
690689
),
691690
new ExpectedDiagnosticWarning(
692691
new Range(3, 2, 3, 2 + 11),
693-
`Record 'mode' is implied to have the 'agent' value if 'tools' record is present so the specified value will be ignored.`,
692+
`Tools can only be used when in 'agent' mode, but the mode is set to 'ask'. The tools will be ignored.`,
694693
),
695694
new ExpectedDiagnosticWarning(
696695
new Range(6, 3, 6, 3 + 37),
697-
`Duplicate metadata 'tools' will be ignored.`,
696+
`Duplicate property 'tools' will be ignored.`,
698697
),
699698
new ExpectedDiagnosticWarning(
700699
new Range(7, 1, 7, 1 + 19),
701-
`Duplicate metadata 'tools' will be ignored.`,
700+
`Duplicate property 'tools' will be ignored.`,
702701
),
703702
]);
704703
});
@@ -775,7 +774,7 @@ suite('TextModelPromptParser', () => {
775774
await test.validateHeaderDiagnostics([
776775
new ExpectedDiagnosticWarning(
777776
new Range(2, 1, 2, 1 + 15),
778-
`Unknown metadata 'applyTo' will be ignored.`,
777+
`Unknown property 'applyTo' will be ignored.`,
779778
),
780779
]);
781780
});
@@ -813,7 +812,7 @@ suite('TextModelPromptParser', () => {
813812
await test.validateHeaderDiagnostics([
814813
new ExpectedDiagnosticWarning(
815814
new Range(3, 1, 3, 13),
816-
`Unknown metadata 'mode' will be ignored.`,
815+
`Unknown property 'mode' will be ignored.`,
817816
),
818817
]);
819818
});
@@ -852,7 +851,7 @@ suite('TextModelPromptParser', () => {
852851
await test.validateHeaderDiagnostics([
853852
new ExpectedDiagnosticWarning(
854853
new Range(2, 1, 2, 14),
855-
`Unknown metadata 'mode' will be ignored.`,
854+
`Unknown property 'mode' will be ignored.`,
856855
),
857856
new ExpectedDiagnosticWarning(
858857
new Range(3, 10, 3, 10 + 2),
@@ -886,7 +885,7 @@ suite('TextModelPromptParser', () => {
886885
await test.validateHeaderDiagnostics([
887886
new ExpectedDiagnosticWarning(
888887
new Range(2, 1, 2, 7 + 9),
889-
`Unknown metadata 'mode' will be ignored.`,
888+
`Unknown property 'mode' will be ignored.`,
890889
),
891890
]);
892891
});
@@ -914,7 +913,7 @@ suite('TextModelPromptParser', () => {
914913
await test.validateHeaderDiagnostics([
915914
new ExpectedDiagnosticWarning(
916915
new Range(2, 1, 2, 7 + 6),
917-
`Unknown metadata 'mode' will be ignored.`,
916+
`Unknown property 'mode' will be ignored.`,
918917
),
919918
]);
920919
});
@@ -942,7 +941,7 @@ suite('TextModelPromptParser', () => {
942941
await test.validateHeaderDiagnostics([
943942
new ExpectedDiagnosticWarning(
944943
new Range(2, 1, 2, 7 + 7),
945-
`Unknown metadata 'mode' will be ignored.`,
944+
`Unknown property 'mode' will be ignored.`,
946945
),
947946
]);
948947
});
@@ -970,7 +969,7 @@ suite('TextModelPromptParser', () => {
970969
await test.validateHeaderDiagnostics([
971970
new ExpectedDiagnosticWarning(
972971
new Range(2, 1, 2, 7 + 20),
973-
`Unknown metadata 'mode' will be ignored.`,
972+
`Unknown property 'mode' will be ignored.`,
974973
),
975974
]);
976975
});
@@ -999,7 +998,7 @@ suite('TextModelPromptParser', () => {
999998
await test.validateHeaderDiagnostics([
1000999
new ExpectedDiagnosticWarning(
10011000
new Range(3, 1, 3, 7 + 6),
1002-
`Unknown metadata 'mode' will be ignored.`,
1001+
`Unknown property 'mode' will be ignored.`,
10031002
),
10041003
]);
10051004
});
@@ -1029,7 +1028,7 @@ suite('TextModelPromptParser', () => {
10291028
await test.validateHeaderDiagnostics([
10301029
new ExpectedDiagnosticWarning(
10311030
new Range(2, 2, 2, 9 + `${booleanValue}`.length),
1032-
`Unknown metadata 'mode' will be ignored.`,
1031+
`Unknown property 'mode' will be ignored.`,
10331032
),
10341033
]);
10351034
});
@@ -1061,7 +1060,7 @@ suite('TextModelPromptParser', () => {
10611060
await test.validateHeaderDiagnostics([
10621061
new ExpectedDiagnosticWarning(
10631062
new Range(2, 3, 2, 9 + `${quotedString}`.length),
1064-
`Unknown metadata 'mode' will be ignored.`,
1063+
`Unknown property 'mode' will be ignored.`,
10651064
),
10661065
]);
10671066
});
@@ -1093,7 +1092,7 @@ suite('TextModelPromptParser', () => {
10931092
await test.validateHeaderDiagnostics([
10941093
new ExpectedDiagnosticWarning(
10951094
new Range(2, 3, 2, 9),
1096-
`Unknown metadata 'mode' will be ignored.`,
1095+
`Unknown property 'mode' will be ignored.`,
10971096
),
10981097
]);
10991098
});
@@ -1121,7 +1120,7 @@ suite('TextModelPromptParser', () => {
11211120
await test.validateHeaderDiagnostics([
11221121
new ExpectedDiagnosticWarning(
11231122
new Range(2, 2, 2, 8),
1124-
`Unknown metadata 'mode' will be ignored.`,
1123+
`Unknown property 'mode' will be ignored.`,
11251124
),
11261125
]);
11271126
});
@@ -1157,21 +1156,22 @@ suite('TextModelPromptParser', () => {
11571156
);
11581157

11591158
const { tools, mode } = metadata;
1160-
assertDefined(
1159+
assert.equal(
11611160
tools,
1162-
'Tools metadata must be defined.',
1161+
undefined,
1162+
'Tools metadata must not be defined.',
11631163
);
11641164

11651165
assert.strictEqual(
11661166
mode,
1167-
ChatModeKind.Agent,
1167+
ChatModeKind.Ask,
11681168
'Mode metadata must have correct value.',
11691169
);
11701170

11711171
await test.validateHeaderDiagnostics([
11721172
new ExpectedDiagnosticWarning(
11731173
new Range(3, 1, 3, 1 + 11),
1174-
'Record \'mode\' is implied to have the \'agent\' value if \'tools\' record is present so the specified value will be ignored.',
1174+
'Tools can only be used when in \'agent\' mode, but the mode is set to \'ask\'. The tools will be ignored.',
11751175
),
11761176
]);
11771177
});
@@ -1203,21 +1203,22 @@ suite('TextModelPromptParser', () => {
12031203
);
12041204

12051205
const { tools, mode } = metadata;
1206-
assertDefined(
1206+
assert.equal(
12071207
tools,
1208-
'Tools metadata must be defined.',
1208+
undefined,
1209+
'Tools metadata must not be defined.',
12091210
);
12101211

12111212
assert.strictEqual(
12121213
mode,
1213-
ChatModeKind.Agent,
1214+
ChatModeKind.Edit,
12141215
'Mode metadata must have correct value.',
12151216
);
12161217

12171218
await test.validateHeaderDiagnostics([
12181219
new ExpectedDiagnosticWarning(
12191220
new Range(3, 1, 3, 1 + 12),
1220-
'Record \'mode\' is implied to have the \'agent\' value if \'tools\' record is present so the specified value will be ignored.',
1221+
'Tools can only be used when in \'agent\' mode, but the mode is set to \'edit\'. The tools will be ignored.',
12211222
),
12221223
]);
12231224
});

‎src/vs/workbench/contrib/chat/test/common/promptSyntax/service/promptsService.test.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -685,8 +685,7 @@ suite('PromptsService', () => {
685685
uri: file3,
686686
metadata: {
687687
promptType: PromptsType.prompt,
688-
tools: ['my-tool1'],
689-
mode: 'agent',
688+
mode: 'edit',
690689
},
691690
topError: undefined,
692691
references: [nonExistingFolder, yetAnotherFile]

0 commit comments

Comments
 (0)