AgentHost - nicer display messages for the feedback comment tools#323228
Draft
roblourens wants to merge 4 commits into
Draft
AgentHost - nicer display messages for the feedback comment tools#323228roblourens wants to merge 4 commits into
roblourens wants to merge 4 commits into
Conversation
The feedback comment server tools (addComment, listComments, deleteComments, resolveComments, viewUnreviewedComments) had no special handling in the Copilot agent host display layer, so they fell through to the generic `Using/Used "<toolName>"` fallback. Add dedicated invocation and past-tense messages for these tools in copilotToolDisplay.ts. listComments reports the number of comments parsed from the tool result (e.g. "Checked 3 comments"), falling back to "Checked comments" when the result is missing or malformed. The result text is threaded through getPastTenseMessage from both the live (copilotAgentSession) and history-replay (mapSessionEvents) paths so the message is identical on reload. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The feedback comment tools (addComment, listComments, deleteComments, resolveComments, viewUnreviewedComments) are one provider-agnostic server-tool group, but their display strings were authored per-provider, so only Copilot rendered them nicely; Claude showed `Used "listComments"` and Codex showed `Called listComments`. Add a generic, optional `getDisplay(toolName, args, result?)` to `IServerToolGroup` and let `feedbackServerToolGroup` implement it once (including the listComments "Checked N comments" count). A new pure `getServerToolDisplay` dispatcher in `serverToolGroups.ts` matches a tool by bare name or transport-prefixed form (Claude's `mcp__host__<name>`), so each provider's display layer (copilotToolDisplay, claudeToolDisplay, codexMapAppServerEvents) consults it first and falls back to its generic message otherwise. `serverToolGroups` is now the single source of truth wired into AgentServerToolHost. Comment-tool display strings move out of copilotToolDisplay.ts; exhaustive coverage moves to serverToolGroups.test.ts with a delegation smoke check left in copilotToolDisplay.test.ts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR centralizes friendly display strings (display name, invocation, and past-tense messages) for the agent host’s feedback “comment” server tools so Copilot/Claude/Codex all render consistent tool-call UI (including listComments → “Checked N comments” when the count can be parsed).
Changes:
- Extend
IServerToolGroupwith optionalgetDisplay(...)and defineIServerToolDisplay*types for provider-agnostic display metadata. - Add shared
getServerToolDisplay(...)dispatcher and wire it into the agent host plus each provider’s display/replay mapping. - Add/adjust unit tests to cover shared behavior (including transport-prefixed tool names) and keep a small Copilot delegation smoke test.
Show a summary per file
| File | Description |
|---|---|
| src/vs/platform/agentHost/test/node/serverToolGroups.test.ts | Adds unit coverage for shared server-tool display dispatch + listComments count parsing behaviors. |
| src/vs/platform/agentHost/test/node/copilotToolDisplay.test.ts | Adds a smoke test ensuring Copilot delegates feedback-tool display to the shared dispatcher. |
| src/vs/platform/agentHost/node/shared/serverToolGroups.ts | Introduces the shared registry + getServerToolDisplay dispatcher (including suffix match for transport-prefixed names). |
| src/vs/platform/agentHost/node/shared/agentServerToolHost.ts | Adds display-related interfaces and extends IServerToolGroup with optional getDisplay. |
| src/vs/platform/agentHost/node/shared/agentFeedbackServerTools.ts | Implements localized display strings for feedback comment tools, including count-aware listComments past tense. |
| src/vs/platform/agentHost/node/copilot/mapSessionEvents.ts | Passes successful tool result text into Copilot past-tense message generation for count-aware display. |
| src/vs/platform/agentHost/node/copilot/copilotToolDisplay.ts | Consults getServerToolDisplay first for feedback tools and plumbs result text into past-tense message resolution. |
| src/vs/platform/agentHost/node/copilot/copilotAgentSession.ts | Passes successful tool result text into Copilot past-tense message generation. |
| src/vs/platform/agentHost/node/codex/codexMapAppServerEvents.ts | Uses shared server-tool display strings for Codex dynamic tool call start/complete rendering. |
| src/vs/platform/agentHost/node/claude/claudeToolDisplay.ts | Consults shared server-tool display strings for Claude (name/invocation/past tense) and supports optional result text. |
| src/vs/platform/agentHost/node/claude/claudeReplayMapper.ts | Extracts result text from tool content during replay to enable count-aware past-tense messages. |
| src/vs/platform/agentHost/node/claude/claudeMapSessionEvents.ts | Extracts result text from tool content during live mapping to enable count-aware past-tense messages. |
| src/vs/platform/agentHost/node/agentService.ts | Switches agent host startup to contribute the shared serverToolGroups registry. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Low
- Codex: pass the tool call's real arguments (params.item.arguments) to getServerToolDisplay at completion instead of undefined, matching the start branch and enabling args-dependent past-tense messages. - Test: drop the unused `count` parameter from the listComments past-tense helper; the result text fully determines the expected message. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…omments-message # Conflicts: # src/vs/platform/agentHost/node/shared/agentFeedbackServerTools.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The agent host's feedback "comment" server tools —
addComment,listComments,deleteComments,resolveComments,viewUnreviewedComments— had no display handling, so they fell through to the genericUsing/Used "<toolName>"fallback (andCalling/Called listCommentson Codex). This gives them real invocation/past-tense messages across all three agent harnesses, e.g. "Checking comments" → "Checked 3 comments".How
These are provider-agnostic server tools (one
feedbackServerToolGroup, executed against the annotations channel and advertised onSessionState.serverTools), but their display was being authored per-provider — so a first pass that only touched Copilot would still leave Claude showingUsed "listComments"and Codex showingCalled listComments.Instead of triplicating, the display is hoisted into the group that owns the tools:
IServerToolGroupgains an optionalgetDisplay(toolName, args, result?)returning{ displayName?, invocationMessage?, pastTenseMessage? }. The host module stays feature-agnostic.feedbackServerToolGroupimplements it once, including thelistComments"Checked N comments" count parsed from the tool result.getServerToolDisplaydispatcher (serverToolGroups.ts, now the single source of truth wired intoAgentServerToolHost) matches a tool by bare name or transport-prefixed form (Claude'smcp__host__<name>), so it works on the providers' history-replay paths too.Tests
serverToolGroups.test.tscovers all 5 tools, the count plural/singular/missing/malformed cases, themcp__host__listCommentsprefix match, and unknown →undefined.copilotToolDisplay.test.tskeeps a delegation smoke check; exhaustive cases moved to the shared test.(Written by Copilot)