-
Notifications
You must be signed in to change notification settings - Fork 83
feat(amazonq): support context commands in agentic chat #948
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
server/aws-lsp-codewhisperer/src/language-server/agenticChat/context/contextCommandsProvider.ts
Outdated
Show resolved
Hide resolved
74ba40b to
0c40706
Compare
...r/aws-lsp-codewhisperer/src/language-server/agenticChat/context/agenticChatTriggerContext.ts
Show resolved
Hide resolved
8f8ead4 to
4762369
Compare
9f8c9f7 to
7613273
Compare
| }, | ||
| }, | ||
| } | ||
| testFeatures.lsp.window.showDocument = sinon.stub() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this stub missing in TestFeatures?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes I think so
|
|
||
| getContextType(prompt: AdditionalContextPrompt): string { | ||
| if (prompt.filePath.endsWith(promptFileExtension)) { | ||
| if (pathUtils.isInDirectory(path.join('.amazonq', 'rules'), prompt.relativePath)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Why does promptsDirectory get its own handler ahd rulesDirectory does not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because prompts are dynamically updated in the chat context menu, rules are just sent along with api requests
| if (prompt.filePath.endsWith(promptFileExtension)) { | ||
| if (pathUtils.isInDirectory(path.join('.amazonq', 'rules'), prompt.relativePath)) { | ||
| return 'rule' | ||
| } else if (pathUtils.isInDirectory(getUserPromptsDirectory(), prompt.filePath)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we use relativePath for rules but filePath for prompts? Is there a meaningful distinction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because rules are in the workspace but prompts are in the .aws folder
...er/aws-lsp-codewhisperer/src/language-server/agenticChat/context/addtionalContextProvider.ts
Outdated
Show resolved
Hide resolved
|
|
||
| async getAdditionalContext( | ||
| triggerContext: TriggerContext, | ||
| context?: any[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this have to be any[]? Is there no more specific type we can use here? It looks like we're using context in a pretty specific way inside mapToContextCommandItems, is that a type we can extract and use here?
| return fileList | ||
| } | ||
|
|
||
| mapToContextCommandItems(context: any[], workspaceFolderPath: string): ContextCommandItem[] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really get this method, instead of using any and converting it using this method, can't we use ContextCommandItem directly where we call this method?
The context: any always seems to have this type:
{ icon: 'file' | 'folder' | 'code-block' | string, command: string | undefined, id: string | undefined }
which combined with workspaceFolderPath is essentially the same as ContextCommandItem already.
This method maps the any to the specific type and filters out the known icons?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the main reason we have to do this is because context is missing type which is used in the call to get prompt from the local context server, but makes sense to create a type for it instead of using any, will follow up
| @@ -0,0 +1,171 @@ | |||
| import * as path from 'path' | |||
| import { FSWatcher, watch } from 'chokidar' | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're using workspace.fs to access the filesystem, but in this case I think chokidar uses the underlying fs directly.
Could this use https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_didChangeWatchedFiles instead?
This could be a follow up, but it does render the agenticChat unusable in browser environments in the meantime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested in a lsp server-client example and by configuring the following code on the server only (no changes on the client) I was able to get it to work
connection.onInitialized(() => {
// 1.a) build the folder URI
const folderUri = URI.file(
path.join(os.homedir(), '.aws', 'amazonq', 'prompts')
).toString();
// 1.b) register a watcher on “**/*” under that URI
const options: DidChangeWatchedFilesRegistrationOptions = {
watchers: [
{
globPattern: {
baseUri: folderUri,
pattern: '**/*',
},
} as FileSystemWatcher,
],
};
// tell the client “please watch this!”
connection.client.register(DidChangeWatchedFilesNotification.type, options);
});
connection.onDidChangeWatchedFiles((params) => {
for (const evt of params.changes) {
console.log(
`File ${evt.uri} was ${['', 'created', 'changed', 'deleted'][evt.type]}`
);
}
});
| this.contextCommandsProvider = new ContextCommandsProvider(logging, chat, workspace) | ||
| } | ||
|
|
||
| public static getInstance() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we use this anywhere? It seems like we're just instantiating the class, and calling it in the Server (which is great, I much prefer that over singletons)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I forgot to dispose it, fixed
|
What is the actual mechanism for sending context items to the client? I don't think I saw it in |
feat(amazonq): integrate with local context server fix(amazonq): fixes and refactor
it is send in ContextCommandsProvider here: Line 71 in 98aa254
|
| import { getUserHomeDir } from '@aws/lsp-core/out/util/path' | ||
| import * as path from 'path' | ||
|
|
||
| export const promptFileExtension = '.prompt.md' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the prompt file extension is .md, not .prompt.md
Problem
We need to support files/folders/code symbols/prompts as context command in agentic chat, and also show user the context referenced in chat response
Solution
note telemetry update will be followup in next PR
related VS Code change: aws/aws-toolkit-vscode#6999
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.