fix(pi-agents-tmux): the timeline reads the nested toolCall end-event carrier - #1444
Conversation
…or name, id, status, and failure Claude-Session: https://claude.ai/code/session_012epxJEzGqT7q3qcFhdZUt5
ApprovabilityVerdict: Approved 62168ff Self-contained bug fix for transcript timeline display formatting. Adds fallback property reads to handle nested event shapes that were previously causing display issues. Changes are limited to display logic with good test coverage. You can customize Macroscope's approvability policy. Learn more. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds support for Pi’s nested toolCall/tool_call “carrier” shape so tool executions are correctly paired and failures are reflected in the timeline output.
Changes:
- Extend
formatTranscriptForDisplayto derive tool name/id/args/status from nestedtoolCallobjects. - Propagate nested error/status/result fields when rendering
tool_execution_end. - Add a regression test covering nested
toolCallpairing and failure display.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pi-extensions/pi-agents-tmux/tests/transcript-timeline.test.ts | Adds a test ensuring nested toolCall end events pair with their starts and propagate failure. |
| pi-extensions/pi-agents-tmux/extensions/subagent/transcript-timeline.ts | Enhances parsing of tool execution events to support nested toolCall fields for pairing and status rendering. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1bb30e1187
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
…airing, 2.8.5 Claude-Session: https://claude.ai/code/session_012epxJEzGqT7q3qcFhdZUt5
Dismissing prior approval to re-evaluate 62168ff
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (3)
pi-extensions/pi-agents-tmux/extensions/subagent/transcript-timeline.ts:230
- The nested-carrier extraction logic (
call,name,id, plus related fields) is now duplicated acrosstool_execution_startandtool_execution_end. Consider factoring this into a small helper (e.g.,extractToolCallCarrier(event)) that returns{ call, name, id, target }(or{ name, id }for end) to reduce drift risk as additional shapes/fields are supported.
// Pi also emits the nested carrier shape { toolCall: { name, id, … } }.
const call = (event.toolCall ?? event.tool_call) as Record<string, unknown> | undefined;
const name = stringValue(event.toolName ?? event.tool_name) ?? stringValue(event.name) ?? stringValue(call?.name) ?? "tool";
const target = primaryToolArgument(event.args ?? event.arguments ?? event.input ?? event.params ?? call?.arguments ?? call?.input);
const label = target ? `tool ${name} (${oneLine(target, 60)})` : `tool ${name}`;
const id = stringValue(event.toolCallId ?? event.tool_call_id ?? event.toolUseId ?? event.tool_use_id ?? call?.id) ?? `name:${name.toLowerCase()}`;
pi-extensions/pi-agents-tmux/extensions/subagent/transcript-timeline.ts:247
- The nested-carrier extraction logic (
call,name,id, plus related fields) is now duplicated acrosstool_execution_startandtool_execution_end. Consider factoring this into a small helper (e.g.,extractToolCallCarrier(event)) that returns{ call, name, id, target }(or{ name, id }for end) to reduce drift risk as additional shapes/fields are supported.
const call = (event.toolCall ?? event.tool_call) as Record<string, unknown> | undefined;
const name = stringValue(event.toolName ?? event.tool_name) ?? stringValue(event.name) ?? stringValue(call?.name) ?? "tool";
const id = stringValue(event.toolCallId ?? event.tool_call_id ?? event.toolUseId ?? event.tool_use_id ?? call?.id) ?? `name:${name.toLowerCase()}`;
const open = openTools.get(id)?.shift();
const failed = event.isError === true || event.is_error === true || call?.isError === true || call?.is_error === true || stringValue(event.status ?? call?.status) === "error";
const status = stringValue(event.status ?? call?.status) ?? (failed ? "error" : "ok");
const resultSize = payloadByteSize(event.result ?? event.output ?? event.content ?? call?.result);
pi-extensions/pi-agents-tmux/CHANGELOG.md:7
- This changelog entry is a single very long line, which makes diffs and release notes harder to read. Consider wrapping it across multiple indented lines (keeping the same bullet) so it stays readable in terminals and typical markdown viewers.
### 2.8.5
- The Transcript timeline reads the nested `toolCall`/`tool_call` end-event carrier — name, id, status, `isError`/`is_error`, arguments, result (the pi-session-bridge event-sanitizer shape). Previously a nested end fell back to `name:tool`, leaving its start marked `✖ no result recorded` beside a separate neutral `tool tool · ok` row, and a nested snake-case error flag rendered as success. Id-less fallback pairing is case-insensitive on the tool name.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (3)
pi-extensions/pi-agents-tmux/extensions/subagent/transcript-timeline.ts:247
resultSizeonly checkscall?.resultfor nested carriers, but the top-level code path supportsoutput/contentas result payload keys. If the nested carrier usestoolCall.outputortoolCall.content(common in some tool result shapes), the timeline will omit the size (and potentially under-report “has result”). Consider expanding the nested lookup to includecall?.output/call?.content(and any other supported keys) to match the robustness of the top-level payload handling.
const failed = event.isError === true || event.is_error === true || call?.isError === true || call?.is_error === true || stringValue(event.status ?? call?.status) === "error";
const status = stringValue(event.status ?? call?.status) ?? (failed ? "error" : "ok");
const resultSize = payloadByteSize(event.result ?? event.output ?? event.content ?? call?.result);
pi-extensions/pi-agents-tmux/extensions/subagent/transcript-timeline.ts:230
- The nested-carrier extraction and the
name/idderivation logic is duplicated acrosstool_execution_startandtool_execution_end. To reduce the chance of start/end drifting (e.g., adding a new key on one side but not the other), consider factoring this into a small helper that returns{ call, name, id }(and optionallytarget) used by both cases.
// Pi also emits the nested carrier shape { toolCall: { name, id, … } }.
const call = (event.toolCall ?? event.tool_call) as Record<string, unknown> | undefined;
const name = stringValue(event.toolName ?? event.tool_name) ?? stringValue(event.name) ?? stringValue(call?.name) ?? "tool";
const target = primaryToolArgument(event.args ?? event.arguments ?? event.input ?? event.params ?? call?.arguments ?? call?.input);
const label = target ? `tool ${name} (${oneLine(target, 60)})` : `tool ${name}`;
const id = stringValue(event.toolCallId ?? event.tool_call_id ?? event.toolUseId ?? event.tool_use_id ?? call?.id) ?? `name:${name.toLowerCase()}`;
pi-extensions/pi-agents-tmux/extensions/subagent/transcript-timeline.ts:243
- The nested-carrier extraction and the
name/idderivation logic is duplicated acrosstool_execution_startandtool_execution_end. To reduce the chance of start/end drifting (e.g., adding a new key on one side but not the other), consider factoring this into a small helper that returns{ call, name, id }(and optionallytarget) used by both cases.
const call = (event.toolCall ?? event.tool_call) as Record<string, unknown> | undefined;
const name = stringValue(event.toolName ?? event.tool_name) ?? stringValue(event.name) ?? stringValue(call?.name) ?? "tool";
const id = stringValue(event.toolCallId ?? event.tool_call_id ?? event.toolUseId ?? event.tool_use_id ?? call?.id) ?? `name:${name.toLowerCase()}`;
|
Merge queue ejected this PR ( Ejecting merge-group run: https://github.com/vanillagreencom/vstack/actions/runs/32054807496 ( Failing job(s):
No usable same-named comparison on the PR head (checks absent, skipped, or still running) — no flake-vs-genuine call is available; inspect the failing run before re-arming. Automated by merge-queue-ejection-alert (VST-196). This alert never re-arms auto-merge. |
Follow-up to #1442 for the one finding deferred past its push budget (thread PRRT_kwDORvss6s6Z05wL): the timeline's tool branches now normalize the nested
toolCall/tool_callcarrier — name, id, status, isError, arguments, result — the shape pinned in pi-session-bridge's event-sanitizer tests. Previously a nested end fell back toname:tool, leaving the paired start marked✖ no result recordedand emitting a separate neutraltool tool · okrow. One new pin (378 green): a nested end pairs with its plain start and carries the failure tone.https://claude.ai/code/session_012epxJEzGqT7q3qcFhdZUt5