-
Notifications
You must be signed in to change notification settings - Fork 3.2k
fix: include Editor Content description with request #6706
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
base: main
Are you sure you want to change the base?
Conversation
All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
@dan0v can you share more about the workflows that make this important to you? So far as I understand this was a purposeful simplification of prompting. I think we would be happy to revert, but only if there are compelling benefits |
@sestinj For any tool which reads or modifies the codebase, such information is crucial in order to properly identify what code the user is querying. It is also counter-intuitive that in the UI, it looks to the user as if the code snippet which is provided to the model includes the line number information, since it is displayed above the code snippet, when that is no longer the case. The alternative for any tool trying to interact with code based on user queries is to rely on the file path (which is still included in the request) and do a string comparison of the whole file to hope for one of many possible matches on the code snippet. That is a terrible option, when the simple solution of providing that contextual information already existed previously and worked well. If a similar simplification vein were further pursued, the file path could also be removed from the request, but I would also not recommend such a move, as it would break even more tooling. In terms of cost/benefit, very little is gained by the removal (I don't know exactly what, if anything to be honest), but existing workflows like my own will be broken by it. The additional context provided in the description is minimal, so won't be consuming a relevant portion of the available tokens and should just be included again as before in my opinion. See xkcd-1172 -- I would say my use-case is less niche 😄 : |
Description
In version v1.0.15-vscode, the LLM was informed about various extra information about user selections in the editor. By v1.0.16-vscode, this extra information was removed, breaking workflows which rely on the LLM being aware of details like user selection line number from code snippets. This regression occurred in this commit.
The simple and minimal fix is to replicate what existed before this change, by including the context item description field for codeblocks, as before, in the information passed to the LLM. I can't see a good reason why this information should have been removed in the linked commit - maybe it was simply forgotten.
Checklist
Screenshots
Before fix: (unsuccessfully tries to use a tool, since it has lost the information it had in prior versions, regarding line numbers)

After fix: (doesn't need to call a tool, since the information is provided directly by the codeblock context item)

Tests
Two small tests were modified to accept the new (old) codeblock format, which includes the description as well.
Summary by cubic
Restored the editor content description in requests so the LLM receives selection details like line numbers, fixing a regression that broke related workflows.