-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Conversation
d58e8c2
to
b249a03
Compare
I tested this. The ui looks nice and I think this is a great improvement. This was my test:
Based on this I would suggest:
|
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! |
packages/ai-chat/src/browser/session-summary-variable-contribution.ts
Outdated
Show resolved
Hide resolved
packages/ai-chat/src/browser/session-summary-variable-contribution.ts
Outdated
Show resolved
Hide resolved
packages/ai-chat/src/browser/session-summary-variable-contribution.ts
Outdated
Show resolved
Hide resolved
packages/ai-chat/src/browser/session-summary-variable-contribution.ts
Outdated
Show resolved
Hide resolved
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.
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!
b249a03
to
8ffb092
Compare
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. |
packages/ai-ide/src/browser/context-session-summary-variable.ts
Outdated
Show resolved
Hide resolved
packages/ai-chat/src/browser/session-summary-variable-contribution.ts
Outdated
Show resolved
Hide resolved
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. |
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. |
If persistence for taskContexts is set-up, the in memory version is still kept, so you have two versions of the same taskContex then.
=> If persistence is active, we should fully get rid of the in memory ones I think |
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); |
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 Is this the expected way to get sessions 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.
@planger Ping
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 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, calledChatSession
, 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.
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.
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:
theia/packages/ai-chat/src/common/chat-service.ts
Lines 180 to 182 in 505c885
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?
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?
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? |
d5ef8c5
to
36f3b5e
Compare
packages/ai-chat/src/common/chat-session-summary-agent-prompt.ts
Outdated
Show resolved
Hide resolved
packages/core/src/common/resource.ts
Outdated
@@ -224,29 +224,54 @@ export class DefaultResourceProvider { | |||
|
|||
} | |||
|
|||
export type ResourceInitializationOptions = Pick<Resource, 'autosaveable' | 'initiallyDirty' | 'readOnly'> |
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 is hard to understand as opposed to a simple interface 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.
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.
packages/core/src/common/resource.ts
Outdated
protected contents: string = ''; | ||
protected contents: string | Promise<string>; | ||
|
||
constructor(readonly uri: URI, protected options?: ResourceInitializationOptions) { } |
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 makes all of the fields in this class effectively mutable.
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 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?
a09abf6
to
393d311
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.
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.
Minor: The pointer is shown over the whole text instead of only the actions:
import { CHAT_SESSION_SUMMARY_PROMPT } from './chat-session-summary-agent-prompt'; | ||
|
||
@injectable() | ||
export class ChatSessionSummaryAgent extends AbstractStreamParsingChatAgent implements ChatAgent { |
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.
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.
Removed tag.
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 even implement the ChatAgent
interface
afa557a
to
2980e72
Compare
Fixed the initialization of the new
Tweaked the check here so that it wouldn't appear on non-callback suggestions. |
da60c8d
to
cb9728b
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 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 { |
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 even implement the ChatAgent
interface
cb9728b
to
3e0d58c
Compare
efacf04
to
140e4a3
Compare
And trigger lint, maybe?
140e4a3
to
05f022c
Compare
What it does
This PR adds a number of capabilities to the AI chat system discussed in #15094:
Closes #15094
To do:
How to test
+
in the bottom left of the input. Choose 'Session Summary' and then the session you're interested in.#session-summary:<id>
- you should get auto-completion.Follow-ups
Breaking changes
Attribution
Review checklist
Reminder for reviewers