Skip to content

fix(agent/pi): support pi v0.84.0 toolcall_end without cumulative message - #1674

Open
happyTonakai wants to merge 2 commits into
chenhg5:mainfrom
happyTonakai:fix/pi-rpc-toolcall-v0.84
Open

fix(agent/pi): support pi v0.84.0 toolcall_end without cumulative message#1674
happyTonakai wants to merge 2 commits into
chenhg5:mainfrom
happyTonakai:fix/pi-rpc-toolcall-v0.84

Conversation

@happyTonakai

Copy link
Copy Markdown
Contributor

Summary

Fixes #1659 — tool-call events are silently dropped when the user upgrades pi to ≥ 0.84.0.

pi 0.84.0 (earendil-works/pi#7290) removed the cumulative message and assistantMessageEvent.partial fields from JSON/RPC message_update events to make streaming output linear. emitToolFromMessage in agent/pi/session.go still parsed toolcall_end exclusively through those removed fields, so every toolcall_end hit msg == nil → return and no EventToolUse was ever emitted — no tool progress shown on any platform, and the engine logs tools=0 for every turn.

Changes

agent/pi/session.go — in the toolcall_end path:

  • Fast path (pi ≥ 0.84.0): read the completed toolCall object delivered directly on the event (assistantMessageEvent.toolCall), validate type == "toolCall", extract name + input via the existing extractToolInput, and emit EventToolUse.
  • Fallback (pi < 0.84.0): keep the legacy message/partial + contentIndex snapshot path unchanged.
  • Safety net: if toolCall is present but its type is not "toolCall" (cannot happen on the real wire), fall through to the legacy path instead of silently dropping the event.

agent/pi/pi_test.go — 3 regression tests:

  • TestHandleMessageUpdate_ToolcallEnd_DirectToolCall — new v0.84.0 wire format emits exactly one EventToolUse.
  • TestHandleMessageUpdate_ToolcallEnd_CoexistsWithPartial — on pre-0.84.0 wire where both toolCall and partial are present, the fast path wins and exactly one event is emitted (no double-emit).
  • TestHandleMessageUpdate_ToolcallEnd_NonToolCallType — a wrong-typed toolCall falls through to the legacy path rather than being dropped.

Backward compatibility

Fully backward compatible: pre-0.84.0 pi still sends the old fields, and the legacy path is untouched. The fast path always wins when toolCall is present (it has carried the finalized block on every released pi version).

Testing

go test ./agent/pi/ -run 'TestHandleMessageUpdate_ToolcallEnd' -v   # all 9 cases pass
go test ./agent/pi/

…sage

pi v0.84.0 (issue #7290) removed the cumulative message and
assistantMessageEvent.partial fields from JSON/RPC message_update events
to make streaming output linear. toolcall_end now carries the complete
toolCall object directly; read it first and fall back to the old
message/partial snapshots for pre-0.84.0 pi versions.

@chenhg5 chenhg5 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

结论: Approve

总体判断: 一个上游 pi CLI breaking change 的干净 fix。设计成 fast-path + legacy fallback,对新旧版本都安全,三个独立测试 case 把关键契约(无重复 emit、错类型 fallback)都钉住了。建议合入。

Review 范围:

  • 看了 agent/pi/session.goemitToolFromMessage 的 fast path 新增(22 行)+ 三个新增测试。
  • 旁路核对 extractToolInput 既有签名,确认 type assertion + 返回值契约匹配。
  • 没改 pre-0.84.0 message/partial snapshot 路径——这是 agent/pi/session.go 改动只 +22/-0 的原因。

✅ 做得好的地方:

  • Fast-path 优先 + legacy 不动:完全兼容旧版本 pi(pre-0.84.0 wire 仍走老路径),同时让 0.84.0+ 的新事件格式立刻生效。这种「不改旧路径、只插一段 fast-path」的写法最大程度降低回归面。
  • Defense-in-depth 注释明确type != "toolCall" cannot happen on the real pi wire; fall through——作者清楚知道这是不可能 case,但仍选择 fall through 而不是 silent drop,这个选择值得肯定,避免未来读到代码的人误以为是「可以删」的 dead code。
  • 测试命名自解释 + 测试 fix 真实场景_DirectToolCall(新格式)/ _CoexistsWithPartial(新旧共存,无 double-emit)/ _NonToolCallType(错类型 fallback),三个 case 覆盖了 fast path 的全部边界。第三个测试特别有价值——它显式断言「错类型 → fall through」,如果未来谁把这段改成 unconditional early return,这个测试会立刻红。
  • Issue link 准确:作者引用了上游 earendil-works/pi#7290 作为 breaking change 来源,方便 reviewer 验证 wire format。

🚨/🔴 必须处理:

  • 未发现。

🟠 建议改进:

  • 不阻塞。toolCalltype 字段断言(itemType == "toolCall")理论上可能因为 wire 协议扩展出现新值(例如 toolCallResult),但目前 pi 官方没有这类事件,未来遇到再处理。

🔵 可选优化:

  • PR title 说「toolcall_end without cumulative message」,但实际 diff 里 fast path 触发的条件是「toolCall 字段存在」而非「无 message 字段」。文档里也写「fast path always wins when toolCall is present」——与 title 措辞略有出入。建议 PR title 改为 support pi v0.84.0 toolcall_end via direct toolCall field 更准确。低优先,合完可改。

❓ 需要确认:

  • 无。

Testing / Risk:

  • 已看到的验证: CI 全绿(lint / unit / smoke / regression / performance,09:17-09:26Z 完成);新增 3 个测试 + 既有 TestHandleMessageUpdate_ToolcallEnd_* 系列保持绿(PR body 说 9 个 case 全过);mergeable=UNKNOWN 是 GitHub 计算队列延迟,base=main + 无 conflict 应该是 MERGEABLE,几分钟内会更新。
  • 未覆盖风险: 真实 pi 0.84.0 wire 上 toolCall 字段是否真的每个 toolcall_end 都有?作者没贴 raw wire log,但 test fixture 用了与上游 issue 描述一致的结构,置信度高。
  • Blast radius: 仅 agent/pi/,不影响其他 agent(claudecode / codex / cursor / qoder / opencode / iflow / copilot / kimi / antigravity / acp)。

Next step:

  • 建议 owner 直接 merge。scope 极小,fix 一个真实用户痛点(升级到 pi 0.84.0 后所有 tool call 在 IM 端看不到进度),purely additive(+22/-0),所有调用点兼容。可以现在合。

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.

pi agent: tool-call events silently dropped after pi 0.84.0 RPC protocol change (message/partial removed from message_update)

2 participants