Skip to content

feat: add text based tool updates for agentic-chat #984

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 14 commits into from
Apr 17, 2025
Merged

Conversation

Hweinstock
Copy link
Contributor

@Hweinstock Hweinstock commented Apr 17, 2025

Problem

There is no way to tell what tools the agent is running and what their results are.

Additionally, streaming content to the chat is currently done through lsp.sendProgress which does not provide meaningful interface to build up a response. For example, each call overwrites all previous data, and doesn't provide any guardrails to prevent this from happening accidentally.

Solution

  • Add an abstraction that wraps lsp.sendProgress for streaming and writing result "blocks" back to the user.
  • Add a simple text based ui to stream these results as they are computed.
  • add emojis and markdown to be replaced by proper mynah UI components.
  • Add the moment we just dump all results to the chat. We could add hard-coded custom formatting options for known tools, but that raises the question what we should do for unknown tools (MCP).

Demo

  • executeBash is enabled for the demo.
textUIDemo.mov

Known Issues

When the request to Q endpoint fails, the chat hangs.

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@kmile
Copy link
Contributor

kmile commented Apr 17, 2025

This looks amazing! 🕺

@Hweinstock Hweinstock marked this pull request as ready for review April 17, 2025 16:49
@Hweinstock Hweinstock requested a review from a team as a code owner April 17, 2025 16:49
@Hweinstock Hweinstock changed the title feat: add text based tool updates for agentic-chat (WIP) feat: add text based tool updates for agentic-chat Apr 17, 2025
const results: ToolResult[] = []

for (const toolUse of toolUses) {
if (!toolUse.name || !toolUse.toolUseId) continue

try {
this.#debug(`Running tool ${toolUse.name} with input:`, JSON.stringify(toolUse.input))
await chatResultStream.writeResultBlock({ body: `${executeToolMessage(toolUse)}` })
Copy link
Contributor

@kmile kmile Apr 17, 2025

Choose a reason for hiding this comment

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

Not needed for this PR: Is it also possible to update a previous block? I imagine the UX showing 'pending' .. 'in progress' ... 'done', or hiding confirmation buttons, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once a block is added, its committed. This was an intentional choice to minimize interacting with state, and treat the block store as persistent.

However, you could use the stream writer to implement a progress block that renders 'in progress', 'pending', etc. until the actual resulting block is committed. The streaming writer doesn't commit any blocks to the store until it is closed (in this example, the result is resolved).

Comment on lines +127 to +131
async #sendProgressToClient(chunk: ChatResult | string, partialResultToken?: string | number) {
if (!isNullish(partialResultToken)) {
await this.#features.lsp.sendProgress(chatRequestType, partialResultToken, chunk)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I love how you've been able to keep updating the client a separate concern from the agent loop and from the tool execution itself. Really flexible approach!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, that way my goal!

Copy link
Contributor

@kmile kmile left a comment

Choose a reason for hiding this comment

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

Awesome PR!

@zixlin7 zixlin7 merged commit 12dc8d7 into aws:main Apr 17, 2025
7 checks passed
@Hweinstock Hweinstock deleted the text branch April 18, 2025 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants