Skip to content

Agent prompt suggestions & chat summary #15427

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

Merged
merged 5 commits into from
Apr 28, 2025

Conversation

colin-grant-work
Copy link
Contributor

@colin-grant-work colin-grant-work commented Apr 8, 2025

What it does

This PR adds a number of capabilities to the AI chat system discussed in #15094:

  • The ability for particular agents to contribute suggestions to the chat, which are rendered above the input.

At the moment, these are a static part of the agent's declaration. They could be implemented as a push system (agent fires event when suggestions changed) or pull system (agent can implement a method that is passed, e.g. a ChatSession and returns suggestions).

  • A new context variable type: session summaries, along with facilities to generate summaries, resolve summary variables, and resolve a set of summaries as part of the system prompt of a given agent.
  • The ability to open editors to view the values of context variables.

I'm not 100% sold on this feature. For one thing, it's a bit redundant for files. For another, at the moment, the content would be basically static and reflect the value of the variable at the time that it was resolved (the time at which the editor was opened). It might be possible to use the resolution context to provide updates when the variable service resolves a variable - e.g. if the same session is involved - but the way things are declared at the moment, it's hard to use the resolution context in a generic way.

Closes #15094

To do:

  • 2.1: I would introduce this in the default renderer, i.e. actions in the text plus optionally additional actions that are shown under "more" in a popup. Follow-up created: Theia AI - Epic - next #15068
  • show progress when creating the summary (or create it earlier).
  • I think I'm watching for file changes but not doing anything when they happen - implement updates if files are deleted / modified on disk.
  • Maybe hook up response rendering system instead of current (almost) direct markdown?

How to test

  1. Have a conversation with an agent.
  2. Start a new chat.

If your first chat was with Coder, it should give you two suggestions about starting a new chat, or starting a new chat with a summary of the current chat. If you choose the latter, it should automatically add a context variable.

  1. You can add a summary of the first chat to the current chat in a couple of ways:
  • it can be added to the context using the + in the bottom left of the input. Choose 'Session Summary' and then the session you're interested in.
  • it can be added to a message using #session-summary:<id> - you should get auto-completion.
  1. If you add a session summary, it should be included in the message to the LLM - check the history.

There's bit of a UX hiccup here. It takes several seconds to get the AI-generated summary for the first time (or to generate a new one if the session to be summarized adds messages). Only once that interaction is complete do we initiate the request that required the summary. Perhaps we should show the user message in the UI before we've parsed all of its variables, and then show the progress message until we start getting content back for the main request?

  1. Separately, if you add any kind of variable to the context, you should be able to click on the element in the context display and see the resolved value of that variable, e.g. the summary it provides of a given chat session.

Follow-ups

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

@github-project-automation github-project-automation bot moved this to Waiting on reviewers in PR Backlog Apr 8, 2025
@colin-grant-work colin-grant-work force-pushed the feature/agent-suggestions branch 4 times, most recently from d58e8c2 to b249a03 Compare April 8, 2025 21:13
@eneufeld
Copy link
Contributor

eneufeld commented Apr 9, 2025

I tested this. The ui looks nice and I think this is a great improvement.

This was my test:

  • initial prompt: @Coder suggets improvement for #currentRelativeFilePath
  • After the answer (which contained reading the file and a changeset) I then started a new chat with a summary (the load took a bit without a progress what was concerning)
  • Looking at the context, it did not contain any of the file references
  • Then I prompted: how do we continue? and it searched the workspace to find the file from the first prompt but only based on the class name it had in the summary.

Based on this I would suggest:

  • resolve variables in the summary
  • show progress when creating the summary

@JonasHelming
Copy link
Contributor

Quick tested this, looks cool, but i want to dig into it more. We should collect all feedback before iterating on it, but thanks Colin for preparing this!

@planger planger self-requested a review April 14, 2025 16:12
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.

First of all, I just want to say how much I like these powerful features added here—really great work!

A couple of thoughts from my side:

Suggestions Handling: I feel the suggestions are conceptually quite close to the chat session’s change set in their lifecycle, how they are created and updated, and also how they are shown to the user (as a dedicated area near the input, in reference to a chat session). So I feel like it might be better to manage these suggestions in a similar manner, i.e. directly within the chat session itself. That way, the agent or tools could update them dynamically based on the current context—like proposing follow-up questions or suggesting the user start a new task once the session only after it reached a certain length. That flexibility could really open up a lot of additional use cases.

UI Representation: I agree with Jonas—it’d be great to support going beyond markdown and support richer UI in suggestions area. That said, this raises some interesting questions. If we keep suggestions with the chat session and have them filled by agents or tools, it might feel awkward to directly create React nodes. So far, agents rather produce chat response nodes, and those are transferred to the UI using dedicated renderers. Should we stick with that model or would that be a misuse of the chat response infrastructure we already have?

TaskContextService: I also like the idea of pulling out the chat task management logic and persistence into a separate service and the summarization into a specific agent. That abstraction would make creating summaries and managing task contexts accessible not only through variables but also through a more direct API to agents and potentially other consumers. This feels like a good move for configurability of the summarization (LLMs and prompts) and reusability of reading or creating tasks.

Thanks again for the great work!

@colin-grant-work
Copy link
Contributor Author

Should we stick with that model or would that be a misuse of the chat response infrastructure we already have?

I think that that is the closest analogue we have if we decide we want to allow LLM's to contribute suggestions of their own, rather than hardcoding them on specific agents. Then it wouldn't be an abuse of the system, but just the system: if the LLM is generating them, then they are just responses :-). For now, though, I haven't provided a mechanism for the LLM's to suggest things, but only for agents to provide suggestions. How they get them is left open, though.

@JonasHelming
Copy link
Contributor

You can click now on any context variable, e.g. also instances of the file variable. However, for the file variable, it opens an empty read only editor for me. This is unexpected, it should open the underlying file instead.
As this behavior is not really part of this feature, I am also perfectly fine with restricting the click to the new taskContext variable, only.

@JonasHelming
Copy link
Contributor

Is there a reason why we make the editor for tastContexts read only for the case that there is no underlying file? Could we easily allow the user to modify it in memory instead? This can for sure be a follow-up, but we should capture it.

@JonasHelming
Copy link
Contributor

If persistence for taskContexts is set-up, the in memory version is still kept, so you have two versions of the same taskContex then.

  • Create a task context (via Coder suggestion click)
  • Resolve it (e.g. via saying something in a new chat
  • type "#session-summary:" => You see two version of the tastContext, the file and the in-memory one

=> If persistence is active, we should fully get rid of the in memory ones I think

@JonasHelming
Copy link
Contributor

When I click on a taskContext variable in the chat which points to an underlying file, it opens a read only editor. This is unexpected, it should open the underlying file.

}
async suggest(context: ChatSession | ChatRequestModel): Promise<void> {
const model = ChatRequestModel.is(context) ? context.session : context.model;
const session = this.chatService.getSessions().find(candidate => candidate.model.id === model.id);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@planger Is this the expected way to get sessions now?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@planger Ping

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 in this case we don't even need to retrieve the actual chat session, because model.id should always be equivalent to the ChatSession.id (see chat-serivce.ts:187).

In general, we do have a distinction there:

  • The ChatModel encompasses all information on a conversation (also referred to as session in the chat model, e.g. ChatRequestModel.session).
  • The Chat Service wraps each ChatModel into another object, called ChatSession, which also adds more UI-related state relevant for the chat service, such as whether the session is currently active in the chat view, the currently pinned agent, last interaction, etc.

But the IDs should always be the same.

Copy link
Contributor Author

@colin-grant-work colin-grant-work Apr 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because model.id should always be equivalent to the ChatSession.id (see chat-serivce.ts:187).
... But the IDs should always be the same.

This is (currently) true (for our implementation of ChatSessions), but not guaranteed by the interfaces / API, so I'd prefer to keep the structure that matches ID's on elements that are supposed to be the same.

The alternative is the getSession API, which does the same iteration, so this isn't much less efficient:

getSession(id: string): ChatSessionInternal | undefined {
return this._sessions.find(session => session.id === id);
}

Alternatively, we could add a reference to the session on the model, or an API on the chat service to retrieve a session given a model?

@colin-grant-work
Copy link
Contributor Author

colin-grant-work commented Apr 18, 2025

When I click on a taskContext variable in the chat which points to an underlying file, it opens a read only editor. This is unexpected, it should open the underlying file.

We can do this, but it would require different services. At the moment, we create one kind of resource and then populate that using the variable resolution system, which returns text. The editor is readonly (and not hooked up to language services, etc.) because the variable resolution system doesn't know anything except that it can get a string. If we want to open the underlying file, then we have to add to the variable resolver interface the possibility of opening a variable so that resolver itself can decide whether to use a variable-specific resource or a standard URI. Would you like that as part of this PR?

Is there a reason why we make the editor for tastContexts read only for the case that there is no underlying file? Could we easily allow the user to modify it in memory instead? This can for sure be a follow-up, but we should capture it.

The answer is the same for this: we're not opening the file, we're retrieving the value of a variable.

@JonasHelming
Copy link
Contributor

When I click on a taskContext variable in the chat which points to an underlying file, it opens a read only editor. This is unexpected, it should open the underlying file.

We can do this, but it would require different services. At the moment, we create one kind of resource and then populate that using the variable resolution system, which returns text. The editor is readonly (and not hooked up to language services, etc.) because the variable resolution system doesn't know anything except that it can get a string. If we want to open the underlying file, then we have to add to the variable resolver interface the possibility of opening a variable so that resolver itself can decide whether to use a variable-specific resource or a standard URI. Would you like that as part of this PR?

Is there a reason why we make the editor for tastContexts read only for the case that there is no underlying file? Could we easily allow the user to modify it in memory instead? This can for sure be a follow-up, but we should capture it.

The answer is the same for this: we're not opening the file, we're retrieving the value of a variable.

I see! @planger WDYT about this? I think we will want this anyways, also for the file variable, right?

@JonasHelming JonasHelming mentioned this pull request Apr 19, 2025
68 tasks
@colin-grant-work colin-grant-work force-pushed the feature/agent-suggestions branch 2 times, most recently from d5ef8c5 to 36f3b5e Compare April 23, 2025 17:51
@@ -224,29 +224,54 @@ export class DefaultResourceProvider {

}

export type ResourceInitializationOptions = Pick<Resource, 'autosaveable' | 'initiallyDirty' | 'readOnly'>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is hard to understand as opposed to a simple interface type.

Copy link
Contributor Author

@colin-grant-work colin-grant-work Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also guaranteed to maintain the correct type if anything changes in Resource. For example, readOnly had changed from boolean to boolean | MarkdownString since the original declaration was written, and the declaration hadn't kept up.

protected contents: string = '';
protected contents: string | Promise<string>;

constructor(readonly uri: URI, protected options?: ResourceInitializationOptions) { }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes all of the fields in this class effectively mutable.

Copy link
Contributor Author

@colin-grant-work colin-grant-work Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I consider that an advantage, given that it does have Mutable in its name. You could consider these changes an answer to the objection that previously, the only thing 'mutable' about a MutableResource was its content and not any of the other fields on a Resource, when there are good UX grounds for wishing for an in-memory resource that can do whatever other resources can do. Why not be able to mark it readonly? Why not be able to say why it's readonly? Why not be able to say that it's initially dirty? Why not be able to customize its save behavior without having to create a new resource class (and so new resource resolver) for every particular use case?

@colin-grant-work colin-grant-work force-pushed the feature/agent-suggestions branch 2 times, most recently from a09abf6 to 393d311 Compare April 25, 2025 17:10
Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the great work. All comments from mine besides the regression mentioned below could be handled in follow ups if the time until release runs out.


I had a quick look through the code however I did not check every line in detail. I tried breaking the functionality but was not able too.


Architecturally I understand that each agent can add own suggestions. However it's a bit weird to me that such a generic suggestion like we have now for the Coder is not handled generically too.

Could be a good follow up.


Blocker regression: The edit chat button UI is broken because the input is not shown anymore.
image

Minor: The pointer is shown over the whole text instead of only the actions:
image

import { CHAT_SESSION_SUMMARY_PROMPT } from './chat-session-summary-agent-prompt';

@injectable()
export class ChatSessionSummaryAgent extends AbstractStreamParsingChatAgent implements ChatAgent {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ChatSessionSummaryAgent should not be a chat agent as it is not intended to be addressed by the user within the chat. Instead it should just be a regular agent like the chat naming agent.

Screenshot from 2025-04-25 22-20-35

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed tag.

Copy link
Member

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 even implement the ChatAgent interface

@colin-grant-work colin-grant-work force-pushed the feature/agent-suggestions branch from afa557a to 2980e72 Compare April 25, 2025 23:34
@colin-grant-work
Copy link
Contributor Author

Blocker regression: The edit chat button UI is broken because the input is not shown anymore.

Fixed the initialization of the new AIChatTreeInputWidget so that the error causing the apparent disappearance wouldn't occur.

Minor: The pointer is shown over the whole text instead of only the actions:

Tweaked the check here so that it wouldn't appear on non-callback suggestions.

@colin-grant-work colin-grant-work force-pushed the feature/agent-suggestions branch 2 times, most recently from da60c8d to cb9728b Compare April 25, 2025 23:41
Copy link
Member

@sdirix sdirix left a 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 for the work!

Potential follow up:
The agent can now be successfully disabled, however the UI does not react.

  • If the user tries to trigger a summary, they do not get an error shown, so they don't know what is going wrong (maybe they forgot that they disabled the agent)
  • In case the agent is disabled, we could hide the "start new chat with summary" text in case the summary does not exist yet (as it will not be creatable)
  • In case the agent is disabled, we could hide the "summary chat" action in the toolbar (but keep the "show summary one)

import { CHAT_SESSION_SUMMARY_PROMPT } from './chat-session-summary-agent-prompt';

@injectable()
export class ChatSessionSummaryAgent extends AbstractStreamParsingChatAgent implements ChatAgent {
Copy link
Member

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 even implement the ChatAgent interface

@github-project-automation github-project-automation bot moved this from Waiting on reviewers to Needs merge in PR Backlog Apr 27, 2025
@colin-grant-work colin-grant-work force-pushed the feature/agent-suggestions branch from cb9728b to 3e0d58c Compare April 28, 2025 02:26
@colin-grant-work colin-grant-work force-pushed the feature/agent-suggestions branch from efacf04 to 140e4a3 Compare April 28, 2025 15:44
@colin-grant-work colin-grant-work force-pushed the feature/agent-suggestions branch from 140e4a3 to 05f022c Compare April 28, 2025 16:03
@colin-grant-work colin-grant-work merged commit 8366fff into master Apr 28, 2025
8 of 11 checks passed
@github-project-automation github-project-automation bot moved this from Needs merge to Done in PR Backlog Apr 28, 2025
@github-actions github-actions bot added this to the 1.61.0 milestone Apr 28, 2025
@colin-grant-work colin-grant-work deleted the feature/agent-suggestions branch April 28, 2025 22:53
@JonasHelming JonasHelming mentioned this pull request May 5, 2025
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants