fix(daemon): emit tool_use from tool_execution_start in pi-rpc by monotykamary · Pull Request #186 · nexu-io/open-design

monotykamary · 2026-04-30T23:54:17Z

Summary

The pi-rpc adapter emitted tool_use from message_end, which fires before tool execution starts. The web UI pairs tool results to prior tool_use events, so receiving a tool_result without a preceding tool_use broke tool card rendering and file auto-open behavior.

Changes

Move tool_use emission to tool_execution_start, matching the pattern in copilot-stream.ts where tool.execution_start maps to tool_use
Remove redundant tool call extraction from message_end -- tool_use is now emitted at execution time, usage is already emitted from turn_end
Extract mapPiRpcEvent as a pure exported function so tests exercise the real event mapping logic instead of an inlined copy that can diverge from production
Update e2e test to call mapPiRpcEvent directly instead of an inlined mapper
Add regression test asserting tool_use arrives before tool_result in event order

Context

Addresses review comments from @mrcfps on PR #117:

tool_execution_start should emit tool_use -- tool_use before tool_result, not from message_end
e2e test should exercise the real adapter path -- inlined mapper diverged from production

Event ordering (before vs after)

Before (broken):

message_end  -> tool_use
tool_execution_start -> status only
tool_execution_end -> tool_result   <-- UI sees result with no prior use

After (fixed):

tool_execution_start -> tool_use + status
tool_execution_end -> tool_result   <-- UI can pair result with prior use

Out of scope

Killing the pi process after agent_end loses prompt cache on providers with session affinity (Anthropic, etc.), increasing cost per request. On Anthropic Claude, prompt caching reduces cost by ~50-90% for multi-turn sessions; killing the process after each turn loses this benefit. A follow-up PR could add process reuse or a connection pool to preserve cached prefixes across requests.

The pi-rpc adapter emitted tool_use from message_end, which fires before tool execution starts. The web UI pairs tool results to prior tool_use events, so receiving tool_result without a preceding tool_use broke tool card rendering and file auto-open behavior. Move tool_use emission to tool_execution_start, matching the pattern in copilot-stream.ts. Remove redundant tool call extraction from message_end (tool_use is now emitted at execution time, usage is already emitted from turn_end). Extract mapPiRpcEvent as a pure exported function so tests exercise the real event mapping logic instead of an inlined copy that can diverge from production. Ref: mrcfps review comments on PR nexu-io#117

lefarcen · 2026-04-30T23:55:21Z

Hi @monotykamary! 🎉

Thanks for the contribution — this is exactly the kind of systematic fix we love to see: root cause identified from prior review feedback, event ordering corrected with clear before/after diagrams, and a regression test that enforces the invariant.

I will run a deep review and get back to you within 24h.

Thanks for making open-design better!
— open-design team

lefarcen

Hey @monotykamary! 🎉

This is exactly the kind of systematic fix we love — root cause identified from prior review feedback, event ordering corrected with clear before/after diagrams, and a regression test that enforces the invariant. The pure function extraction also makes the codebase much more testable.

Lens A (code correctness): ✅ Event ordering fix is correct, state management is safe, all pi RPC event types still handled. The new regression test at line 322-336 explicitly asserts tool_use arrives before tool_result and would fail on the old code.

Lens B (reasoning completeness): ✅ Bug diagnosis is clear, test drift justification is solid. Two P2 suggestions below to strengthen the rationale.

P2 suggestions (non-blocking)

1. "Pure function" doc comment (apps/daemon/src/pi-rpc.ts:68-69)
The function is documented as "Pure function: no side effects" but it mutates ctx.sentFirstToken.value at line 60-65. Technically this is a side effect on the context object (even though it's intentional and localized). Consider revising the doc comment to say "Pure in the sense of no I/O or child process interaction; mutates ctx.sentFirstToken to track streaming state." or similar — helps future readers understand the contract.

2. Cache-loss quantification (PR body "Out of scope" section)
The "Out of scope" section mentions cache loss from killing pi after agent_end but provides no quantification (cost increase %, latency impact, typical cache hit rate). Without numbers, it's hard to prioritize the follow-up. Could you add a rough estimate? E.g., "On Anthropic Claude, prompt caching reduces cost by ~50-90% for multi-turn sessions; killing the process after each turn loses this benefit." This helps maintainers decide urgency vs nice-to-have.

3. Edge case awareness (not blocking, just flagging)
If pi sends tool_execution_start but never sends tool_execution_end (e.g. pi crashes mid-tool), the UI will see tool_use but never tool_result. No timeout/cleanup mechanism is visible in the daemon. Is this handled upstream (e.g. in the UI layer) or should the daemon emit a synthetic error tool_result after some timeout? (Might be out of scope for this PR, just raising for awareness.)

Overall: ready to merge IMO after you consider the P2 points. Nice work! 🚀

The function mutates ctx.sentFirstToken to track streaming state. Calling it "pure" is misleading; revised the doc comment to say no I/O or child process interaction instead.

monotykamary · 2026-05-01T00:04:23Z

Thanks @lefarcen! Addressing your P2s:

The function is documented as "Pure function: no side effects" but it mutates ctx.sentFirstToken.value

Updated in 6853e6b — replaced with "No I/O or child process interaction; mutates ctx.sentFirstToken to track streaming state."

The "Out of scope" section mentions cache loss from killing pi after agent_end but provides no quantification

Added to the PR body: "On Anthropic Claude, prompt caching reduces cost by ~50-90% for multi-turn sessions; killing the process after each turn loses this benefit."

If pi sends tool_execution_start but never sends tool_execution_end (e.g. pi crashes mid-tool), the UI will see tool_use but never tool_result

If pi crashes mid-tool, agent_end won't fire but child.on("close") still ends the SSE stream. The UI handles abrupt stream termination gracefully already — not specific to pi, any agent can crash. A synthetic error tool_result on timeout is reasonable hardening but applies to all adapters, so it belongs in a separate PR.

lefarcen

Perfect! 🎉 Both P2 items addressed:

✅ Doc comment clarity (commit 6853e6b) — now accurately describes the mutation contract without claiming "pure" in the strict FP sense.

✅ Cache-loss quantification — "50-90% cost reduction" in the PR body gives maintainers the data needed to prioritize the follow-up.

✅ Edge case reasoning — your explanation on the mid-tool crash scenario is sound: child.on('close') already handles abrupt termination upstream, making this a cross-adapter hardening opportunity rather than a pi-specific gap.

Ship it. 🚀 No blocking concerns remaining. This is ready to merge IMO — deferring final approval to a human maintainer per policy.

mrcfps

@monotykamary Thanks for tightening up the Pi RPC event ordering — moving tool_use to tool_execution_start and having the tests exercise the exported mapper is a solid direction. 🙌

I found one small UI regression risk before this lands: now that the start event emits a real tool_use, the extra status: tool event can render as a visible status pill and also break consecutive tool grouping in the chat UI. Once that duplicate status emission is removed (or explicitly filtered in the renderer), this should be good to go. 😊

mrcfps · 2026-05-02T06:50:15Z

@monotykamary Quick clarification after double-checking: this is specifically a regression from the new ordering. status(tool) existed before, but now that tool_use is emitted inline, the status block sits between tool cards and prevents grouping. Since tool_use now carries the tool name/input and renders the running tool state, I think removing the status(tool) emission is the preferred fix over filtering it in the renderer. Thanks again for the thoughtful fix here — this should be a small cleanup on an otherwise solid change. 😊

…on_start Now that tool_use fires inline from tool_execution_start, the accompanying status(tool) event is redundant: tool_use already carries the tool name, and the UI renders running state from the tool card. The extra status pill breaks consecutive tool_use grouping in AssistantMessage.buildBlocks. Aligns with copilot-stream, which emits only tool_use from tool.execution_start with no status event.

monotykamary · 2026-05-02T07:40:05Z

This is a regression from the new ordering. Before, status(tool) was the only signal that a tool was running (since tool_use was deferred to message_end), but now that tool_use fires inline, the extra status event sits between tool cards and breaks grouping in buildBlocks. Removed in 6d53806, aligning pi-rpc with copilot-stream (no status event on tool start).

lefarcen

Perfect fix! 🎉

@monotykamary addressed @mrcfps's UI regression concern cleanly:

✅ **Removed emission from ** (commit 6d53806) — now only is emitted, preventing the duplicate status block that broke tool card grouping

The fix is exactly what @mrcfps requested: "removing the status(tool) emission is the preferred fix over filtering it in the renderer."

Event order is now clean:

tool_execution_start -> tool_use (no status)
tool_execution_end -> tool_result

Ready for maintainer approval IMO. Ship it! 🚀

lefarcen

Perfect fix! 🎉

@monotykamary addressed @mrcfps's UI regression concern cleanly:

✅ Removed status(tool) emission from tool_execution_start (commit 6d53806) — now only tool_use is emitted, preventing the duplicate status block that broke tool card grouping

The fix is exactly what @mrcfps requested: "removing the status(tool) emission is the preferred fix over filtering it in the renderer."

Event order is now clean:

tool_execution_start -> tool_use (no status)
tool_execution_end -> tool_result

Ready for maintainer approval IMO. Ship it! 🚀

mrcfps

LGTM. The revised mapper now emits tool_use at tool_execution_start without the duplicate tool status event, which preserves the UI's expected tool card grouping and gives tool_result a prior use event to pair with. Exporting mapPiRpcEvent also makes the regression tests exercise the production mapping path directly, including usage, TTFT, and the tool ordering invariant.

_{Generated by looper 0.3.0 · runner=reviewer · agent=opencode}

lefarcen

Approved per off-thread maintainer review.

Author addressed the UI regression concern (removed status(tool) emission in commit 6d53806). Event order now clean.

mrcfps

@monotykamary This looks good to me — approved ✅\n\nThanks for tightening up the Pi RPC event ordering here. Emitting tool_use from tool_execution_start lines up with the UI pairing model and the Copilot stream behavior, and removing the extra tool status event should keep consecutive tool cards grouped cleanly. I also appreciate that the tests now exercise the real mapper path and cover the ordering regression directly. Nice, focused fix! 🚀

lefarcen · 2026-05-02T08:26:29Z

Merged! 🎉 Thanks @monotykamary for the contribution to open-design. Looking forward to the next one!

lefarcen · 2026-05-02T09:00:07Z

Merged! 🎉 Thanks @monotykamary for the contribution to open-design. Looking forward to the next one!

Qodo-Free-For-OSS · 2026-05-02T09:58:06Z

Hi, mapPiRpcEvent emits tool_use.id/tool_use.name and tool_result.toolUseId as null when pi fields are missing, but the SSE contract and web UI translator require strings; the UI will drop these events and can end up with unpaired/missing tool cards.

Severity: action required | Category: correctness

How to fix: Validate tool ids before emitting

Agent prompt to fix - you can give this to your LLM of choice:

Issue description

mapPiRpcEvent currently emits tool_use/tool_result events with null identifiers when pi fields are missing. This violates the DaemonAgentPayload contract (requires strings) and the web UI drops those events, potentially recreating orphan tool results / missing tool cards.

Issue Context

DaemonAgentPayload requires tool_use.id and tool_use.name to be string, and tool_result.toolUseId to be string.

The web UI translator only accepts tool_use when both id and name are strings, and only accepts tool_result when toolUseId is a string.

Fix Focus Areas

apps/daemon/src/pi-rpc.ts[158-183]

Only send('agent', {type:'tool_use', ...}) when raw.toolCallId and raw.toolName are strings.

Only send('agent', {type:'tool_result', ...}) when raw.toolCallId is a string.

Decide behavior for malformed events: either drop silently or emit a raw/status event for observability.

Found by Qodo code review

…io#186) * fix(daemon): emit tool_use from tool_execution_start in pi-rpc The pi-rpc adapter emitted tool_use from message_end, which fires before tool execution starts. The web UI pairs tool results to prior tool_use events, so receiving tool_result without a preceding tool_use broke tool card rendering and file auto-open behavior. Move tool_use emission to tool_execution_start, matching the pattern in copilot-stream.ts. Remove redundant tool call extraction from message_end (tool_use is now emitted at execution time, usage is already emitted from turn_end). Extract mapPiRpcEvent as a pure exported function so tests exercise the real event mapping logic instead of an inlined copy that can diverge from production. Ref: mrcfps review comments on PR nexu-io#117 * docs(daemon): clarify mapPiRpcEvent mutability contract The function mutates ctx.sentFirstToken to track streaming state. Calling it "pure" is misleading; revised the doc comment to say no I/O or child process interaction instead. * fix(pi-rpc): remove redundant status(tool) emission from tool_execution_start Now that tool_use fires inline from tool_execution_start, the accompanying status(tool) event is redundant: tool_use already carries the tool name, and the UI renders running state from the tool card. The extra status pill breaks consecutive tool_use grouping in AssistantMessage.buildBlocks. Aligns with copilot-stream, which emits only tool_use from tool.execution_start with no status event.

lefarcen self-requested a review April 30, 2026 23:54

lefarcen added the bug Something isn't working label Apr 30, 2026

monotykamary marked this pull request as ready for review April 30, 2026 23:56

lefarcen reviewed Apr 30, 2026

View reviewed changes

docs(daemon): clarify mapPiRpcEvent mutability contract

6853e6b

The function mutates ctx.sentFirstToken to track streaming state. Calling it "pure" is misleading; revised the doc comment to say no I/O or child process interaction instead.

lefarcen reviewed May 1, 2026

View reviewed changes

mrcfps previously requested changes May 2, 2026

View reviewed changes

Comment thread apps/daemon/src/pi-rpc.ts Outdated

monotykamary requested review from lefarcen and mrcfps May 2, 2026 07:40

lefarcen reviewed May 2, 2026

View reviewed changes

mrcfps reviewed May 2, 2026

View reviewed changes

lefarcen approved these changes May 2, 2026

View reviewed changes

mrcfps approved these changes May 2, 2026

View reviewed changes

mrcfps merged commit 5a0f954 into nexu-io:main May 2, 2026

lefarcen mentioned this pull request May 2, 2026

release: Open Design 0.2.0 — 45 PRs in 24 hours, image/video/audio gen, html-ppt skill, dark mode #297

Merged

Conversation

monotykamary commented Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Context

Event ordering (before vs after)

Out of scope

Uh oh!

lefarcen commented Apr 30, 2026

Uh oh!

lefarcen left a comment

Choose a reason for hiding this comment

P2 suggestions (non-blocking)

Uh oh!

monotykamary commented May 1, 2026

Uh oh!

lefarcen left a comment

Choose a reason for hiding this comment

Uh oh!

mrcfps left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mrcfps commented May 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

monotykamary commented May 2, 2026

Uh oh!

lefarcen left a comment

Choose a reason for hiding this comment

Uh oh!

lefarcen left a comment

Choose a reason for hiding this comment

Uh oh!

mrcfps left a comment

Choose a reason for hiding this comment

Uh oh!

lefarcen left a comment

Choose a reason for hiding this comment

Uh oh!

mrcfps left a comment

Choose a reason for hiding this comment

Uh oh!

lefarcen commented May 2, 2026

Uh oh!

lefarcen commented May 2, 2026

Uh oh!

Qodo-Free-For-OSS commented May 2, 2026

Issue description

Issue Context

Fix Focus Areas

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

monotykamary commented Apr 30, 2026 •

edited

Loading

mrcfps commented May 2, 2026 •

edited

Loading