feat(acp): model the stabilized tool call name - #235
Merged
Merged
Conversation
The Tool Call Name RFD moved to Completed on 2026-09-17, stabilizing the optional `name` field on tool calls in ACP v1 and v2. Neither tool call shape carried a typed property for it. The value was not being lost: SessionUpdate's JsonExtensionData binder already preserved it verbatim as an unknown field, so a proxy round-trip kept it. What was missing is any typed way to read it, which is the entire point of the field — clients cannot tell `read_file` from `grep` when both report kind `read`, and eval tooling cannot count tools at all. Add `Name` to ToolCallUpdate (v1 `tool_call`) and ToolCallStatusUpdate (the upsert shape both versions share). Presence is deliberately not tracked: the v1/v2 divergence over an explicit null only becomes observable at a merge point, and nothing in the SDK merges this field. Name is not added to the published ToolCallUpdate constructor; adding an optional parameter is binary-breaking for the shipped package (CP0002). Set it through the init-only property, as ClientCapabilities does for Elicitation. Verified by deserializing each shape and asserting both the typed read and that the field is no longer routed to ExtensionData, plus a proxy round-trip per version. Reverse-verified by breaking only the tool_call_update binding: exactly the two cases covering that type went red while the v1 tool_call case stayed green. Co-Authored-By: Claude Code <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
The stabilized tool-call `name` was unreadable downstream, so wire it from the projection all the way to the chat pill. The chain has more links than it looks. AcpClient hands the view model a projection-rebuilt ToolCallStatusUpdate (AcpSessionUpdateView.ToolCall re-deserializes JSON that AcpSessionProjection.StoreTool wrote by hand), not the DTO the parser produced. Adding the property to the DTO alone changed nothing on screen — a probe at the parse site showed `name` bound correctly while the snapshot arriving downstream still had it null. StoreTool is therefore where the field had to be threaded, alongside ToolState and AcpToolCallSnapshot. Past the SDK, seven sites rebuild ConversationMessageSnapshot field by field. They compile fine when a field is missing and silently drop it, so each one is updated: the reducer, the workspace writer, the outgoing projector, the workspace clone, the transcript clone, and the two snapshot comparisons. Rendered per the RFD's own guidance: `title` stays the primary line and the programmatic name becomes a caption beneath it. It is an opaque token with no ACP naming scheme, so it is rendered verbatim and gets no resource string. AutomationName now carries it too — otherwise a screen reader user still could not tell read_file from grep, which is the entire point of the field. Not done: the permission card. ACP lets an agent include the name in a session/request_permission tool call, so it would fit there too, but that view model carries only the raw tool payload and plumbing it is a separate change. Co-Authored-By: Claude Code <noreply@anthropic.com>
ACP already carries the programmatic tool name into the permission request — v1's session/request_permission has a required `toolCall: ToolCallUpdate`, and v2's permission subject has a `tool_call` variant of the same shape — so the field is in scope for approvals with no permission-specific addition required. It matters here more than on the tool row: an agent asks for permission *before* it reports the tool call, so the pill header may not exist yet. The only thing on screen while deciding is the option list, and "Run tests" does not say whether the agent wants run_command or something else that produces the same copy. The name renders as a caption above the options, verbatim and without a resource, matching the tool row. It is cleared by the cancellation-retry path, which replaces the request wholesale and must not leave a stale tool name behind. Resolve the name next to the existing toolCallId resolution, over both subject shapes: a typed ToolCallUpdate and the raw JsonElement. Co-Authored-By: Claude Code <noreply@anthropic.com>
Review found six real defects in the three tool-call-name commits. Each was verified against the code before fixing, and two of the reported items turned out to be wrong or only half right — those are noted below. **The name rendered twice.** The header TextBlock was pasted twice verbatim, so every tool call with a name showed it on two consecutive lines. Not caught by anything: both copies share one x:Bind, neither has an x:Name, so the binding generator sees one valid binding and the smoke test's text match still hits the first occurrence. **The label was set in a PUA icon font.** SymbolThemeFontFamily is Segoe Fluent Icons, which has no Latin glyphs — "read_file" would render as tofu or blank, and the font is absent entirely on Skia. Every other use of that resource in the repo is a FontIcon carrying a private-use codepoint, which is the only correct use. Dropped. **The permission card was blank on every v2 request.** v2 moved the tool call off the permission request and onto a subject union, so the v1 extraction never sees it and the label stayed unset — the one case where naming the tool matters most, because the title is the only other thing on screen. The event args now extract the name the same way they already extract the title, and the v2 handler passes the subject's tool call in. **The permission name could not be cleared.** It was a plain property on an ObservableObject, so the cancellation-retry reset raised nothing: the InfoBar kept showing a tool the card no longer offered. Now observable, matching the sibling Title. **A name-only change did not mark the conversation dirty.** WorkspaceWriter.MessageEquals compared every other tool-call field but not this one, so a patch that only added a name left LastUpdatedAt stale — which also makes the cloud-sync merge reject the newer document. **v2 could drop an entire update over a bad name.** Every optional patch field on these shapes carries a default-on-error converter; name was the sole exception, so a peer sending "name":123 lost the status and title too. Scoped to v2 deliberately — that is the only surface the recovery contract applies to, and v1 stays strict on purpose. Also: a duplicated <remarks> block on the published ToolCallUpdate constructor, and a [NotifyPropertyChangedFor] on _toolCallName claiming a coupling to ShouldShowToolCallPill that does not exist — a name alone with no id, kind or title is not a renderable pill. Not fixed, reported but wrong: two test names cited by the review no longer exist, and the v2 recovery grant was claimed for v1 too. Reverse-verified: removing the v2 subject plumbing turns the new draft permission test red (Actual: null), confirming it reaches the path rather than passing vacuously. Co-Authored-By: Claude Code <noreply@anthropic.com>
This branch was successfully deployed
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
1 participant
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.
跟进 ACP 上游 2026-09-17 的 Tool Call Name RFD（转 Completed）。上次动 ACP 是 9/16，之后上游只出了这一条。三个形状已经有的能力我们逐条核过：Terminal Authentication、Boolean Config Option、
model_config分类、Message IDs、usage_update、session/delete、logout、session/close、session/resume、additionalDirectories、session_info、session/list —— 全都已在。唯一的缺口就是工具调用的name，两个提交补齐它并一路显示到聊天界面。先纠正一个容易误判的点：这个值本来并没有丢。
SessionUpdate基类带[JsonExtensionData]，name作为未知字段被原样保留，代理转发、往返都不受影响。缺的是类型化入口——而这正是该字段的全部意义：当read_file和grep都报kind: "read"时客户端无法区分，eval 场景更是一个工具都统计不出来。提交一 ·
4b22f759SDK 建模ToolCallUpdate（v1tool_call）与ToolCallStatusUpdate（v1+v2 共用的 upsert 形状）各加一个Name。null语义不同（v2 视为清除，v1 根本清不掉），但这个差异只有在把字段合并进累积状态时才可观测，而当时没有任何地方合并它。ToolCallUpdate构造函数：加可选参数对已发布包是二进制破坏（CP0002）。走 init-only 属性，与ClientCapabilities.Elicitation同样处理。提交二 ·
bf8c3909显示到聊天界面链路比看上去长，只加 DTO 属性对屏幕没有任何影响——这一点是探针查出来的，不是推理出来的。
AcpClient交给 ViewModel 的是投影重建的ToolCallStatusUpdate（AcpSessionUpdateView.ToolCall反序列化的是AcpSessionProjection.StoreTool手工拼的 JSON），不是解析器产出的那个 DTO。所以StoreTool才是必须打通的地方，连带ToolState和AcpToolCallSnapshot。过了 SDK 之后，有七处逐字段重建
ConversationMessageSnapshot。少一个字段它们照常编译、照常静默丢弃——所以逐一补齐：reducer、workspace writer、outgoing projector、workspace clone、transcript clone，以及两处快照比较。渲染遵循 RFD 自己的指引：
title保持主行，程序化工具名作为其下的 caption。它是不透明 token、ACP 没有定义命名方案，因此原样渲染、不占用资源字符串。AutomationName也带上它——否则读屏用户依然分不出read_file和grep，而这正是该字段存在的理由。验证
tool_call_update线名绑定 → 恰好覆盖该类型的 2 条转红，v1tool_call那条保持绿SalmonEgg.Acp.Tests全量SalmonEgg.Presentation.Core.Tests全量dotnet format --verify-no-changes测试里特意加了一条断言：字段不再出现在
ExtensionData里。只断言"能读出read_file"是不够的——保留但不可读的情况下这条也会绿，属于假绿。提交三 ·
df2e4232权限审批name一直在权限请求的标准里，不需要额外字段：v1 的session/request_permission带 requiredtoolCall: ToolCallUpdate，v2 的权限主体联合也有一个tool_call变体装同一类型——第一个提交给ToolCallUpdate加的Name自动就在里面。这里比工具行更要紧：agent 是先请求权限、后上报工具调用的，所以真要你点"允许"的那一刻，pill 标题栏可能根本还不存在。屏幕上唯一的线索就是选项列表，而 "Run tests" 这句话分不出
run_command和另一个恰好生成同样文案的工具。工具名渲染在选项上方的 caption，同样原样、不占资源字符串。取消重试路径会一并清掉它——那条路径整个换掉了请求内容，留个陈旧的工具名是错的。
名称解析与既有的 toolCallId 解析放在一起，两种主体形态都覆盖：类型化的
ToolCallUpdate和原始JsonElement。提交四 ·
ffba74ae评审修复自审不可靠（本次会话里已经反复重复插入、误删过属性），所以走了独立评审。找到 6 个真缺陷，
逐条实测确认后修复：
read_file。两个副本共享同一个x:Bind、都没有x:Name，所以绑定生成器看到的是"一个合法绑定"，冒烟测试的文本匹配也仍命中第一处——任何门禁都抓不到SymbolThemeFontFamily是 Segoe Fluent Icons，没有拉丁字形，read_file会渲染成豆腐块/空白，Skia 上字体根本不存在。仓库里其他 40 多处该资源全是带私有区码位的FontIcon，那才是唯一正确用法ObservableObject上的普通属性，取消重试路径清空它时什么都不通知：InfoBar 继续显示这张卡已经不提供的工具WorkspaceWriter.MessageEquals比对了其他每一个工具调用字段，唯独漏了这个 →LastUpdatedAt不刷新 → 云同步会把更新的文档判为陈旧而丢弃name是唯一例外 → agent 发"name":123会连 status 和 title 一起丢另修：已发布构造函数上重复的
<remarks>；_toolCallName上一个指向不存在耦合的[NotifyPropertyChangedFor(ShouldShowToolCallPill)]（光有名字、没有 id/kind/title 的调用不是可渲染的 pill）。评审报了两条不对的，照单全收就会引入错误：
SessionProjectionWireContract按设计只作用于 v2，v1 保持严格。测试按实际契约收窄到 v2。反向验证：拆掉 v2 subject 管道 → 新增的 draft 权限测试转红（
Actual: null），证明它真走到那条路径，不是空过。
验证（四提交合计）
SalmonEgg.Acp.Tests全量SalmonEgg.Presentation.Core.Tests全量dotnet format --verify-no-changes🤖 Generated with Claude Code