-
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
Add contextSummary
variable to AI system
#14971
base: master
Are you sure you want to change the base?
Add contextSummary
variable to AI system
#14971
Conversation
388129d
to
604cadb
Compare
Great, thank you for this integration!! The whole context story really significantly improves the work with Coder. I did not look at the code yet, but tested a few things. My observations so far: Case 1: Context added in chatThis worked before and the additional context summary does nothing bad (as expected). Case 2: Context in the chat + some files only in the contextWhen I add additional files without any further information, and it was not related to the task, the LLM did not even look at it. When I provided an additional file that was actually relevant for the task, the LLM looked at it, it probably guessed that from the file name and the context. Looks definitely good enough to merge this to me, but we have to evaluate in practise. Case 3: Context only in the context, not in the chatIt looked at the file(s) most of the time. Only if the prompt does not imply any implementation task, it did not. Example: So from case 2 and 3 we identify a Potential issue 1 that the LLM does not look at some files only in the context, but the user expects it to. I found only one prompt though, where this was really an issue. We should evaluate this in practice. If it is a problem, we can augment the prompt in iterations (during internal test) , e.g.:
So issue 1 is not a blocker for this PR. Case 4: Agent builds the context by itselfIssue 2: If a file is added to the context multiple times, it appears there multiple times (@planger I believe this is an issue we should solve in the context layer, WDYT?) Issue 3 With my prompts, the LLM rather rarely "remembered" a file by itself. I tried two options how to fix this: Option A: Add the context add function to a sub bullet in the Context Retrieval section "If you find a file interesting for the task, remember it" => The issue here was that the LLM tended to add irrelevant files, but this needs to be evaluated. Option B: Add the context function to a sub bullet in the apply change area "- Remember changed file: Use using~{context_addFile} to remember all files you change and files that were needed to create the changes." => Seemed to have better results than A as it does that in a phase, were it implements. Option C: We add all modified files to the context in the modify functions ourselves (did not try). This very likely makes sense anyways. Option A and B can be tried "on the go" as they are prompt modifications. Option C we have to implement. However, for all options, it might help to let the LLM specify a description for what is in the file or that is has been changed. My current option would be to go for Option C as a default.
All described issues IMHO do not block this PR, we can handle them in follow-ups Thanks again for this great addition! |
A pretty 'satisfying' user prompt for me to test this was the following, works well with 4o, o3-mini and Sonnet 3.5. it is actually the follow-up of this PR, so the PR follow-ups itself :-) " Note that you should type the file variables out, as they are not added to the context when pasting (the prompt of course still works, but for the test this is important) I literately adapted the system prompt to the one below. This worked pretty nicely, but it did not add files to the context by itself, so I still believe we should go with the following prompt: " Context RetrievalUse the following functions to interact with the workspace files if you require context:
Propose Code ChangesTo propose code changes or any file changes to the user, never print code or new file content in your response. Instead, for each file you want to propose changes for:
Additional ContextThe following files have been provided for additional context. Some of them may also be referred to above. Always look at the relevant files to understand your task using getFileContent |
Thought about it a bit more and I believe instead of option C, we should add a resolved version of the changeset (paths and state) to the prompt with another variable. This is now very easy to do, as we have the infrastructure in place. reasoning: the previously changed files are always interesting. I can add this in a follow-up PR. Then we can go for option A with this PR and optimized from there. @planger WDYT? |
604cadb
to
3803ed9
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.
Thank you very much! Code looks great to me and seems to work fine.
My main concern is how we make the context summary as interpretable as possible for the LLM and give the context variable provider more control over it (see my inline comment).
Thought about it a bit more and I believe instead of option C, we should add a resolved version of the changeset (paths and state) to the prompt with another variable. This is now very easy to do, as we have the infrastructure in place. reasoning: the previously changed files are always interesting. I can add this in a follow-up PR.
Then we can go for option A with this PR and optimized from there.
I would agree. I think for now keeping those separated is simpler and more flexible. If we observe that it confuses the LLMs (which might be the case), we can reconsider. But we'll need more evaluation to confirm this even is a problem we would need to solve. :-)
if (!ChatSessionContext.is(context) || request.variable.name !== CONTEXT_SUMMARY_VARIABLE.name) { return undefined; } | ||
return { | ||
variable: CONTEXT_SUMMARY_VARIABLE, | ||
value: context.session.context.getVariables().filter(variable => !!variable.arg).map(variable => `- ${variable.arg}`).join('\n') |
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.
As there might be other types of variables, we should probably also mention the variable name to facilitate the interpretation of the args for the LLM, and also (or even instead of the arg) show the variable.value
which is intended to be the identifier of the context variable element (e.g. the workspace relative path). The variable.value
is also what's added to the user request instead of the variable, if the user uses this variable in the user request. So it is already a value that is intended to be understood by the LLM. (Edit: while the args is more like a technical id, imho)
I think it might even be worth introducing another optional property in the ResolvedAIContextVariable
, something like contextSummary
, with which the context variable provider can specify an LLM-optimized summary of this context element which can be printed in the context summary to the LLM.
For instance, we may have a variable that represents symbols in a source code file, e.g. #symbol:<file-uri>@<symbol-name>
. Here it'd be helpful for the LLM to (a) know that it is a Symbol and (b) we may want to add additional infos, like it is a property of a specific Typescript class.
Based on the contextSummary
the CONTEXT_SUMMARY_VARIABLE
could output the context variables like this:
variable.name
:variable.description?
// for each variable-type for which we have a variable requestvariable.contextSummary ?? variable.value
// for each variable request of the variable type- ...
- ...
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.
@planger, thanks for the suggestion. While I like the idea, and, I've basically implemented it for the new contextDetails
variable for use in agents other than Coder, it has the downside that the value
field isn't available unless the variable has been resolved, and I don't think we have access to resolved variables at this point - after all, we're busy resolving one ourselves. Or is there somewhere where resolved values are cached (and would it be safe to use that cache)?
We could use the static description
value associated with the variable itself, something like the file variable's 'Resolves the contents of a file'
. That's not really directed at the LLM, but we could add a field that would be.
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.
Please see my suggestion on the variable dependencies.
Regarding the description, I totally agree. This was a misconception on my end. The description is not targeted at the LLM and therefore is probably in most cases not a great fit.
Let's go with this prompt then: #14971 (comment) |
for #14945 we should directly add a second function to this PR, something like the follewing (not compiling semi-pseudo code)
|
@colin-grant-work Please test whether declining a changeste element and clearing the changeset works on this branch, I believe it did not when I tested, it works on master. But I am not sure |
- Adds context to end of Coder agent's prompt - Adds a function to allow agents to supplement the context
f4cda2c
to
cf12b28
Compare
@planger, @JonasHelming, I've pushed a new version of the code that addresses some of the comments above, but there is likely still some work to do re: formatting things as intelligibly as possible.
|
cf12b28
to
5213a9e
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.
Thank you very much for this iteration! I think this goes into the right direction.
I didn't consider the variable dependency issue before (sorry for that), but I feel this is a valid general use case. So I suggest to explicitly support dependent variables.
We need to ensure that we
- Don't inject the variable service into variable resolvers (cyclic DI)
- Aim at taking care of cyclic variable dependencies
- Avoid duplicate resolution
We might be able to address those constraints by handing in a resolveDependency
callback to resolvers and enhance the variable resolution method.
Extend the variable resolver
export interface AIVariableResolverWithVariableDependencies extends AIVariableResolver {
resolve(
request: AIVariableResolutionRequest,
context: AIVariableContext,
resolveDependency: (req: AIVariableResolutionRequest) => Promise<ResolvedAIVariable | undefined>
): Promise<ResolvedAIVariable | undefined>;
}
function isResolverWithDependencies(resolver: AIVariableResolver | undefined): resolver is AIVariableResolverWithVariableDependencies {
return resolver !== undefined && resolver.resolve.length >= 3;
}
Enhance the resolveVariable method of the variable service
interface CacheEntry {
promise: Promise<ResolvedAIVariable | undefined>;
inProgress: boolean;
}
export class DefaultAIVariableService implements AIVariableService {
// ... existing properties and methods
async resolveVariable(
request: AIVariableArg,
context: AIVariableContext,
cache: Map<string, CacheEntry> = new Map()
): Promise<ResolvedAIVariable | undefined> {
const variableName = typeof request === 'string'
? request
: typeof request.variable === 'string'
? request.variable
: request.variable.name;
if (cache.has(variableName)) {
const entry = cache.get(variableName)!;
if (entry.inProgress) {
this.logger.warn(`Cycle detected for variable: ${variableName}. Skipping resolution.`);
return undefined;
}
return entry.promise;
}
const entry: CacheEntry = { promise: Promise.resolve(undefined), inProgress: true };
cache.set(variableName, entry);
const promise = (async () => {
const variable = this.getVariable(variableName);
if (!variable) {
return undefined;
}
const arg = typeof request === 'string' ? undefined : request.arg;
const resolver = await this.getResolver(variableName, arg, context);
let resolved: ResolvedAIVariable | undefined;
if (isResolverWithDependencies(resolver)) {
resolved = await resolver.resolve(
{ variable, arg },
context,
async (depRequest: AIVariableResolutionRequest) =>
this.resolveVariable(depRequest, context, cache)
);
} else {
resolved = await resolver?.resolve({ variable, arg }, context);
}
return resolved ? { ...resolved, arg } : undefined;
})();
entry.promise = promise;
promise.finally(() => {
entry.inProgress = false;
});
return promise;
}
}
Summary
- AIVariableResolverWithVariableDependencies lets a resolver access dependencies through a callback during its resolution.
- The updated
resolveVariable
method uses a promise-based cache with aninProgress
flag for cycle detection. This way, if a variable is already being resolved, it’s skipped—avoiding cycles. - With this approach, resolved variables are accessible during the resolution of dependent ones. This could be leveraged for both contextSummary (letting Coder resolve details) and contextDetails, reducing redundant resolution.
What do you think about this approach?
Maybe I'm over-complicating this, but I feel like letting variables access other variables might be a useful thing in general, and if it is, we should explicitly support it.
If you or others disagree that we should support that, I'm happily follow your lead and think your approach is fine, given it is rather local and puts the responsibility of using this approach carefully into the hands of a specific variable resolver.
}; | ||
|
||
@injectable() | ||
export class ContextSummaryVariableContribution implements AIVariableContribution, AIVariableResolver { |
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 name this ChangeSetSummaryVariableContribution
right?
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 this variable is targeted at files, even though the context may contain arbitrary context variables, like symbols or even domain-specific things.
If we want to keep it that way, we should also reflect that in the naming. It is already reflected in the description.
In general, I tend to believe we can generalize that, because the only file specific thing is currently the description and the word "file" in line 60. Both can be avoided by using the contextSummary
field or the variable name.
const data = values.filter((candidate): candidate is ResolvedAIVariable => !!candidate) | ||
.map(resolved => ({ | ||
variableKind: resolved.variable.name, | ||
variableKindDescription: resolved.variable.description, |
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.
You are probably right to remove this.
variableKind: resolved.variable.name, | ||
variableKindDescription: resolved.variable.description, | ||
variableInstanceData: resolved.arg, | ||
value: resolved.value |
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 still think it might be worth introducing a dedicated summary field here, but I'm certainly not insisting on it if you feel it isn't necessary or if we should rather come back to it at a later point, if the requirement manifests.
What it does
Fixes #14895.
How to test
Follow-ups
Breaking changes
Attribution
Review checklist
Reminder for reviewers