Support for adding, editing, and deleting comments#195
Conversation
|
This is a lot to add to add to the protocol. Why do it this way vs as attachments? |
|
@roblourens Good question. I looked at modelling this as attachments, and I don't think they fit. (AI helped me generate this message as I didn't find it easy to explain) The feature needs a shared, mutable review surface:
I don't think we need to model "accepted by the user" in the protocol, that can stay client-side. The protocol piece is the shared mutable comment state. Attachments are message-scoped input payloads. They live on To make attachments satisfy the requirements above, we'd effectively have to add stable ids, server-owned/mutable attachment state, add/edit/delete/re-anchor commands, threading, and an attachment-specific subscription. At that point we've turned attachments into a second channel while also overloading Requirements 2 and 4: agent-authored entries and cross-client sync need a server-owned, subscribable state surface. Attachments are the right shape for the submitted payload, not for the editing/sharing surface. So the two models can still compose: at submission time, the client can turn the accepted comments into attachments on the outgoing message. The comments channel is the editing and sharing surface, attachments remain the submitted payload surface. The proposed channel follows the same state-bearing pattern as |
Pull request was converted to draft
There was a problem hiding this comment.
I don't think attachements make sense for this use case. But I do agree that the AHP shouldn't be talking about "comments" but rather some more generalised primitive. Maybe something like a "anchored coordination item" which has some message and an anchor like for exampe:
- session
- session turn
- file
- file range
- tool call
These items could be created, read, (edited ?) and removed by both client and agents
currently discussing with connor
|
Oh ok, the feature is more sophisticated than I thought. What's the scenario for the agent adding comments? |
|
The most basic scenario would likely be a code review agent. |
| * interpreted by the protocol. | ||
| */ | ||
| _meta?: Record<string, unknown>; | ||
| } |
There was a problem hiding this comment.
I'd suggest we can also use this surface area to surface GH review comments as well, in the future. Or at least GH-review like experiences. To that end I suggest we have:
- A notion of
author?: CommentAgentAuthor | CommentUserAuthoron the Comment. Doesn't need to be in this PR, an optional thing here could be added as non-breaking in the future. - A notion of resolved threads / thread state
- Thread/comment
severity?: - Would be only allow comments on ranges or also files? Maybe the text range is optional.
| * thread / comment counts without subscribing. Absent when the session | ||
| * does not expose a comments channel. | ||
| */ | ||
| comments?: CommentsSummary; |
There was a problem hiding this comment.
As a user how do I say "I made all these comments, now action on them."?
Imo a natural thing to do would be a new comment attachment type, which could reference either all comments by URI or a comments URI + an array of thread IDs to send to the model and have them address. Especially thinking in an orchestrating/multi-chat world, I may not necessarily want comments implicitly attached to whatever message I send next.
We can then say that an agent host SHOULD provide tools to the harness for the harness to respond to or resolve comments associated with a chat message.
| * @category Comments Actions | ||
| * @version 3 | ||
| */ | ||
| export interface CommentsThreadSetAction { |
There was a problem hiding this comment.
I wonder if "comments" is the right terminology for this. With a tiny bit creativity and a kind of 'comment' I could imagine this could be used for delivering things like diagnostics and unit test failures too, annotating the problematic range or unit test in the code. (Mostly thinking if their failures are delivered via an external system like CI where there is human-in-the-loop, otherwise I would expect the agent generally iterates on the data autonomously without any specific UI integration.)
We don't need to go that far at the outset, but maybe Annotation is the word we use to give us some leeway in the future, what do you think?
btw, in iterating about the terminology my agent brough up the W3C Annotation specification which is interesting. I would not use that spec literally but it might be a point of inspiration for you.
|
Is this the point where we should maybe add a server capability to make this an optional part of the spec? Maybe per-agent? It just seems like a lot to require every host to implement this for every agent. I don't think there would be a "client capability" for this because it can ignore messages and the server is going to independently either have this data or not. |
|
@roblourens I think an AH that doesn't support this would not advertise |
microsoft/vscode#320067