Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

colin-grant-work
Copy link
Contributor

@colin-grant-work colin-grant-work commented Feb 18, 2025

  • Adds context to end of Coder agent's prompt
  • Adds a function to allow agents to supplement the context

What it does

Fixes #14895.

image

How to test

  1. Add a file to the context following the instructions in feat(ai): Enable Context Variables for Chat Requests #14787
  2. Then ask Coder about something in the context. The agent should know what you're talking about.
  3. Ask Coder to add something to the context. It should succeed in doing so, and the file should pop up in the UI.

Follow-ups

  1. I notice that files in the context are getting labeled with a leading slash, making them appear absolute. We could address that, if we think it's confusing.
  2. I also notice that if you drag+drop a file from the explorer tree onto the AI input widget, the text input gets populated with the file information:

image

Breaking changes

  • This PR introduces breaking changes and requires careful review. If yes, the breaking changes section in the changelog has been updated.

Attribution

Review checklist

Reminder for reviewers

@colin-grant-work colin-grant-work force-pushed the feature/context-variables branch 2 times, most recently from 388129d to 604cadb Compare February 18, 2025 22:37
@JonasHelming JonasHelming requested a review from planger February 18, 2025 23:31
@JonasHelming
Copy link
Contributor

JonasHelming commented Feb 19, 2025

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 chat

This worked before and the additional context summary does nothing bad (as expected).

Case 2: Context in the chat + some files only in the context

When 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 chat

It looked at the file(s) most of the time. Only if the prompt does not imply any implementation task, it did not. Example:
"Do not display a message to the user anymore about the tools" with context mcp-command-contribution.ts => It said: OK I won't :-D

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.:

  • "Look at them if you need them for your task"
  • "Always look at them to understand your task"

So issue 1 is not a blocker for this PR.

Case 4: Agent builds the context by itself

Issue 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.
Reasoning:

  • Our current agents do not tend to look at "other relevant files" by itself, e.g. for interface declarations. So the user has to typically provide these themselves and the LLM might add irrelevant things.
  • We have the context add function in place and can evaluate it more in practice with a prompt variant.

All described issues IMHO do not block this PR, we can handle them in follow-ups

Thanks again for this great addition!

@planger
Copy link
Contributor

planger commented Feb 19, 2025

Issue 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?)

Yes, this can be considered a bug. We shouldn't add the same element twice. I'll put it on my list.

Edit: #14979

@JonasHelming
Copy link
Contributor

JonasHelming commented Feb 19, 2025

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 :-)

"
@coder Add a new variable to #file:packages/ai-chat/src/common/context-summary-variable.ts that fully resolves the elements in the context including their content. Look at #file:packages/ai-ide-agents/src/browser/context-functions.ts on how to resolve the context. Now add this variable to the prompt of the universal agent (which is define in #file:packages/ai-ide-agents/src/common/universal-chat-agent.ts). similar to how it is used in the prompt of the coder agent.
"
(packages/ai-ide-agents/src/common/coder-replace-prompt-template.ts was added manually to the context only)

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:

"
You are an AI assistant integrated into Theia IDE, designed to assist software developers with code tasks. You can interact with the code base and suggest changes.

Context Retrieval

Use the following functions to interact with the workspace files if you require context:

  • ~{getWorkspaceDirectoryStructure}: Returns the complete directory structure.
  • ~{getWorkspaceFileList}: Lists files and directories in a specific directory.
  • ~{getFileContent}: Retrieves the content of a specific file.
  • ~{context_addFile}: Remember file locations that are relevant for completing your tasks. Only add files that are really relevant to look at later.

Propose Code Changes

To 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:

  • Always Retrieve Current Content: Use getFileContent to get the latest content of the target file.
  • Remember changed file: Use ~{context_addFile} to remember all files you change and files that were needed to create the changes.
  • Change Content: Use ~{changeSet_writeChangeToFile} or ~{changeSet_replaceContentInFile} to suggest file changes to the user. Only select and call one function per file.

Additional Context

The 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
{{contextSummary}}
"

@JonasHelming
Copy link
Contributor

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?

@colin-grant-work colin-grant-work force-pushed the feature/context-variables branch from 604cadb to 3803ed9 Compare February 19, 2025 16:01
Copy link
Contributor

@planger planger left a 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')
Copy link
Contributor

@planger planger Feb 19, 2025

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 request
    • variable.contextSummary ?? variable.value // for each variable request of the variable type
    • ...
  • ...

Copy link
Contributor Author

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.

Copy link
Contributor

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.

packages/ai-chat/src/common/chat-model.ts Show resolved Hide resolved
@JonasHelming
Copy link
Contributor

Let's go with this prompt then: #14971 (comment)

@JonasHelming
Copy link
Contributor

for #14945 we should directly add a second function to this PR, something like the follewing (not compiling semi-pseudo code)

export const CONTEXT_RESOLVE_VARIABLE: AIVariable = {
    id: 'contextResolve',
    description: nls.localize('theia/ai/core/contextResolve/description', 'Resolves the full content of context elements'),
    name: 'contextResolve',
};

@injectable()
export class ContextResolvedVariableContribution implements AIVariableContribution, AIVariableResolver {
    registerVariables(service: AIVariableService): void {
        service.registerResolver(CONTEXT_RESOLVE_VARIABLE, this);
    }

    canResolve(request: AIVariableResolutionRequest, context: AIVariableContext): MaybePromise<number> {
        if (request.variable.name === CONTEXT_RESOLVE_VARIABLE.name) {
            return 50;
        }
        return 0;
    }

    async resolve(request: AIVariableResolutionRequest, context: AIVariableContext): Promise<ResolvedAIVariable | undefined> {
        if (!ChatSessionContext.is(context) || request.variable.name !== CONTEXT_RESOLVE_VARIABLE.name) { return undefined; }

        return {
            variable: CONTEXT_RESOLVE_VARIABLE,
            value: (() => {
                context.variables.map((contextElement: any) => {
                    const variable = context.session.context.getVariables().find(v => v.id === contextElement);
                    return variable ? `Content-ID: ${variable.arg}\n + Content: ${variable.contextValue}` : 'Variable not found.'
                }
                
            })()
        }
    };
}

@JonasHelming
Copy link
Contributor

@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
@colin-grant-work colin-grant-work force-pushed the feature/context-variables branch 3 times, most recently from f4cda2c to cf12b28 Compare February 20, 2025 04:37
@colin-grant-work
Copy link
Contributor Author

colin-grant-work commented Feb 20, 2025

@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.

  • I have added two new variables, one for change sets (added to Coder's system prompt), and one for context details, added to the universal agent's prompt.
  • Re: context details, @planger proposed adding various details of resolved variables to the contextSummary variable. Since that variable is intended for use with Coder, which can resolve some details on its own, that seems perhaps overkill, but I have attempted to resolve the variables in context for the contextDetails variable. Let me know what you think about that approach, but also let me know if there's some way to access resolved variables while resolving other variables; then we could use them as we see fit in both cases, and not resolve them ourselves, as I've done for contextDetails.
  • Since there's a lot of data for contextDetails, I've just JSON stringified what's available - maybe there's a better way? Maybe I should do that for the change set, as well?
  • I've also fixed the bug re: display of change set deletions caused by consolidation of the delete and remove events.

Copy link
Contributor

@planger planger left a 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 an inProgress 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 {
Copy link
Contributor

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?

Copy link
Contributor

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,
Copy link
Contributor

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Waiting on reviewers
Development

Successfully merging this pull request may close these issues.

[Theia AI] Integrate context feature with Coder
3 participants