-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
feat: show variables and function on ai agent configuration #14177
feat: show variables and function on ai agent configuration #14177
Conversation
a431a3a
to
4a4c035
Compare
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 "Ai Terminal Agent" uses the cwd
variable, however in its list it shows up as "not used". I guess that's because it's also a global variable? Is it one?
Variables which couldn't be mapped are shown as agent-specific:
Does this make sense or should we rather show them in an own optional section which only exists if there is at least one? e.g. "Unmapped variables used in prompt(s)"
I think we should render a replacement text like "None" into empty sections:
Agent specific variables should not be clickable / show a different pointer as nothing happens when you click them
{ name: 'file', usedInPrompt: true, description: 'The uri of the file being edited.' }, | ||
{ name: 'language', usedInPrompt: true, description: 'The languageId of the file being edited.' }, | ||
{ name: 'snippet', usedInPrompt: true, description: 'The code snippet to be completed.' }, | ||
{ name: 'MARKER', usedInPrompt: true, description: 'The position where the completion should be inserted.' } |
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 MARKER
is not a variable but a hard coded segment we refer to in the snippet. It should be removed from the list.
Also note that I adapted the prompt in this PR. So whoever is merged first should adapt to the other PR.
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.
Currently it has the same identifier as a variable thus it is discovered by our parser and needs to be added here.
With your PR this is not an issue anymore and can be removed from the list.
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 other PR was merged, so we need to adjust this PR for the new prompt variables. Otherwise the prompt and variables will not align on master
<div style={{ paddingBottom: 10 }}> | ||
<span style={{ marginRight: '0.5rem' }}>Used Global Variables:</span> |
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 we should not add hardcoded styles here. We already use and consume the style/index.css
so the styles should be added there.
<div style={{ paddingBottom: 10 }}> | ||
<span style={{ marginRight: '0.5rem' }}>Used agent-specific Variables:</span> |
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.
same here
<div style={{ paddingBottom: 10 }}> | ||
<span style={{ marginRight: '0.5rem' }}>Used Functions:</span> |
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.
and here
const agentSpecificVariable = agent.agentSpecificVariables.find(asv => asv.name === variableId); | ||
const undeclared = agentSpecificVariable === undefined; | ||
const notUsed = !parsedPromptParts.agentSpecificVariables.includes(variableId) && agentSpecificVariable?.usedInPrompt === true; |
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.
Minor: In general I'm not a huge fan of large inline calculations. Feels to me that this should either be calculated before, or be encapsulated in an own React component.
const storedPrompt = this.promptService.getRawPrompt(template.id); | ||
const prompt = storedPrompt?.template ?? template.template; |
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.
Seems weird that we have to replicate the logic here. I would have expected that we can just ask the promptService for the correct prompt instead of manually comparing the return value of the prompt service and our template.
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.
this probably never happens, but the storedPrompt can be undefined so I wanted to have a fallback in place.
"Does this make sense or should we rather show them in an own optional section which only exists if there is at least one? e.g. "Unmapped variables used in prompt(s)" I think it does. The most likely case for this is that an agent is using an agent-specific variable and has not declared its usage. All other cases (not mapped to a global) are error cases. |
For the cwd case the current logic checks that if a prompt variable is part of the global variables list => then it goes to the global section @JonasHelming what is your take? |
In the case of |
Yes, there is also a second case of the same behavior somewhere. |
I think there is no reason to not believe an agent. If they want to mislead, they can do that in any way they want. |
4a4c035
to
28cf1f2
Compare
The agent configuration now shows also the variables (global and agent specific) as well as the functions that the agents uses. If a variable or function is used in the prompt but is not declared by the agent then this is also marked in the view. All agents are updated to declare the variables and functions they use. fixes eclipse-theia#14133
28cf1f2
to
ff221b8
Compare
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.
Works for me! Thanks!
I noticed one more issue which could be tackled here or in a follow up: Adapting the prompt template directory does not update the AI configuration view. |
thank you for the test, the found issue should be fixed now |
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.
Works for me! Nice improvements!
What it does
The agent configuration now shows also the variables (global and agent specific) as well as the functions that the agents uses. If a variable or function is used in the prompt but is not declared by the agent then this is also marked in the view.
All agents are updated to declare the variables and functions they use.
fixes #14133
How to test
Open the AI Configuration View and see that variables and functions are shown.
If you modify the prompt the list is dynamically updated.
Review checklist
Reminder for reviewers