Repository navigation
Treat incomplete Pi runs as errors, not completions - #9021
peterthesling wants to merge 2 commits into
Conversation
Assisted-By: devx/29ded3f8-7889-44f9-9d2d-4f87a80c090b
Assisted-By: devx/29ded3f8-7889-44f9-9d2d-4f87a80c090b
📝 WalkthroughWalkthroughThe Pi extension now propagates structured assistant outcomes, routes lifecycle hooks by stop reason, and tests completed, length-capped, provider-error, and aborted sessions. ChangesPi completion outcome routing
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant PiEvent
participant agent_end
participant publishPendingCompletion
participant LifecycleHooks
PiEvent->>agent_end: agent_end event
agent_end->>agent_end: resolve assistant outcome
agent_end->>publishPendingCompletion: store pending completion
publishPendingCompletion->>LifecycleHooks: Stop with terminationReason
publishPendingCompletion->>LifecycleHooks: error or completed Notification
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors)
✅ Passed checks (23 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/CMUXCLI`+PiExtensionSourcePart2.swift:
- Around line 244-257: The failure branch around sendHook("notification") marks
cmux_notification_routed true before confirming notification delivery, allowing
a failed notification to suppress all fallback signaling. Capture the
notification send result, log a warning when delivery fails, and only mark the
stop payload as routed after successful delivery; preserve the existing
error/length messages and stop-hook behavior while ensuring failed delivery
remains diagnosable.
- Around line 227-266: Update publishPendingCompletion so only the explicitly
supported successful stopReason value(s) use the completed notification path.
Handle every unrecognized reason, including toolUse, conservatively as an error
or non-success outcome, preserving the stop lifecycle notification and avoiding
the “Task completed” fallback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9fd040c2-d9ed-4816-a284-edc2a57de297
📒 Files selected for processing (3)
CLI/CMUXCLI+PiExtensionSourcePart1.swiftCLI/CMUXCLI+PiExtensionSourcePart2.swifttests/test_pi_extension_install.py
| function publishPendingCompletion(ctx: ExtensionContext, sessionId: string): void { | ||
| const completion = settleTurn(sessionId); | ||
| if (!completion) return; | ||
| const notificationRouted = sendHook("notification", ctx, { | ||
| message: completion.lastAssistantMessage || "Task completed", | ||
| turn_id: completion.turnId, | ||
| notification: { type: completion.notificationType }, | ||
| }); | ||
|
|
||
| const stopPayload: HookExtra = { | ||
| last_assistant_message: completion.lastAssistantMessage, | ||
| terminationReason: completion.stopReason, | ||
| turn_id: completion.turnId, | ||
| }; | ||
|
|
||
| if (completion.stopReason === "aborted") { | ||
| // User cancellation is idle, not a successful completion worth notifying about. | ||
| stopPayload.cmux_notification_routed = true; | ||
| sendHook("stop", ctx, stopPayload); | ||
| return; | ||
| } | ||
|
|
||
| if (completion.stopReason === "length" || completion.stopReason === "error") { | ||
| // Stop first so the error notification remains the final visible lifecycle state. | ||
| // Always suppress the generic Stop fallback, which would misreport this as completed. | ||
| stopPayload.cmux_notification_routed = true; | ||
| sendHook("stop", ctx, stopPayload); | ||
| sendHook("notification", ctx, { | ||
| message: completion.stopReason === "length" | ||
| ? "Model stopped because it reached the maximum output token limit. The response may be incomplete." | ||
| : completion.errorMessage || "Pi stopped with an error before completing the task.", | ||
| turn_id: completion.turnId, | ||
| notification: { type: "error" }, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| const notificationRouted = sendHook("notification", ctx, { | ||
| message: completion.lastAssistantMessage || "Task completed", | ||
| turn_id: completion.turnId, | ||
| notification: { type: "completed" }, | ||
| }); | ||
| if (notificationRouted) stopPayload.cmux_notification_routed = true; | ||
| sendHook("stop", ctx, stopPayload); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
fd -a 'CMUXCLI\+PiExtensionSourcePart2\.swift$' . || true
echo "== relevant lines =="
file="$(fd 'CMUXCLI\+PiExtensionSourcePart2\.swift$' . | head -n 1)"
if [ -n "${file:-}" ]; then
wc -l "$file"
sed -n '200,290p' "$file" | nl -ba -v200
fi
echo "== stopReason references in file =="
if [ -n "${file:-}" ]; then
rg -n "stopReason|toolUse|notificationRouted|publishPendingCompletion|settleTurn|errorMessage" "$file"
fi
echo "== broader stopReason/toolUse references =="
rg -n "stopReason|toolUse|last_assistant_message|cmux_notification_routed|Model stopped|Task completed" .Repository: manaflow-ai/cmux
Length of output: 341
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file and relevant lines =="
file="./CLI/CMUXCLI+PiExtensionSourcePart2.swift"
wc -l "$file"
sed -n '210,280p' "$file"
echo "== stopReason references in file =="
rg -n "stopReason|toolUse|notificationRouted|publishPendingCompletion|settleTurn|errorMessage" "$file" || true
echo "== broader stopReason/toolUse references =="
rg -n "stopReason|toolUse|last_assistant_message|cmux_notification_routed|Model stopped|Task completed" . || trueRepository: manaflow-ai/cmux
Length of output: 50374
🌐 Web query:
Pi Infinity stopReason toolUse length error aborted official documentation
💡 Result:
In the Pi agent framework, stopReason is a property within an AssistantMessage that indicates why a model generation finished [1][2][3]. The specific conditions you mentioned—length, toolUse, error, and aborted—are standard terminal states in the Pi event protocol [1][4][3]. Here is how these stop reasons are defined: - "length": The model stopped generating because it hit the maximum configured output token limit [1][2][3]. If this occurs during a tool call, the tool call may be incomplete and should generally not be executed or replayed [5]. - "toolUse": The model finished its generation because it is requesting a tool call and expects results from the system [1][2][3]. - "error": An error occurred during the generation process [1][2][3]. The errorMessage field typically contains details about the failure [4][3]. - "aborted": The request was explicitly cancelled by the user or system via an abort signal [1][2][3]. If you are encountering a "length error" or "aborted" state, it indicates the request terminated before completing naturally [2][5]. Recent developer discussions note that messages reaching these terminal states while in the middle of a tool call can produce incomplete data [5]; it is standard practice in robust Pi implementations to sanitize these messages to prevent the execution of partial tool calls [5]. Documentation advises checking the stopReason property directly in the final AssistantMessage to determine the appropriate handling logic for your application [2][6][3].
Citations:
- 1: https://www.pkgstats.com/pkg:`@stonega/pi-ai`
- 2: https://github.com/badlogic/pi-mono/blob/137547a4/packages/ai/README.md
- 3: https://pt-act-pi-mono.mintlify.app/api/ai/stream-and-complete
- 4: https://github.com/badlogic/pi-mono/blob/83378aad/packages/ai/src/types.ts
- 5: fix(agent-core): ignore truncated tool calls openclaw/openclaw#97140
- 6: https://github.com/badlogic/pi-mono/blob/v0.66.1/packages/ai/README.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
code = Path("CLI/CMUXCLI+PiExtensionSourcePart2.swift").read_text()
# Extract control paths for stopReason using a small text scanner.
m = re.search(r"function publishPendingCompletion[\s\S]*?(?=export default)", code)
if not m:
print("publishPendingCompletion not found")
raise SystemExit
body = m.group(0) if m else ""
print("contains aborted branch:", "completion.stopReason === \"aborted\"" in body)
print("contains length/error branch:", "completion.stopReason === \"length\"" in body and "completion.stopReason === \"error\"" in body)
print("contains catch-all notification branch:", "notification: { type: \"completed\" }" in body)
# Check whether any prior elif/else if handles stopReason generically.
branch_lines = [line.strip() for line in body.splitlines() if "completion.stopReason" in line]
print("stopReason branch conditions:")
for line in branch_lines:
print(" " + line)
print("unrecognized_stop_would_reach_completed:", "toolUse" not in str(branch_lines).lower() and body.count("notification: { type: \"completed\" }") > 0)
print()
print("nearby function:")
print(body)
PYRepository: manaflow-ai/cmux
Length of output: 2199
Handle unclassified stopReason values instead of defaulting to “Task completed”.
This only branches on "aborted", "length", and "error". Other reported Pi terminal reasons such as "toolUse" (including truncated/proxy-glitch tool calls) fall through to the final notification branch and report an incomplete turn as completed. Enumerate the known success final reason(s) explicitly and treat unmatched/unrecognized values conservatively instead of using a catch-all “completed” notification.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CLI/CMUXCLI`+PiExtensionSourcePart2.swift around lines 227 - 266, Update
publishPendingCompletion so only the explicitly supported successful stopReason
value(s) use the completed notification path. Handle every unrecognized reason,
including toolUse, conservatively as an error or non-success outcome, preserving
the stop lifecycle notification and avoiding the “Task completed” fallback.
Source: Path instructions
| if (completion.stopReason === "length" || completion.stopReason === "error") { | ||
| // Stop first so the error notification remains the final visible lifecycle state. | ||
| // Always suppress the generic Stop fallback, which would misreport this as completed. | ||
| stopPayload.cmux_notification_routed = true; | ||
| sendHook("stop", ctx, stopPayload); | ||
| sendHook("notification", ctx, { | ||
| message: completion.stopReason === "length" | ||
| ? "Model stopped because it reached the maximum output token limit. The response may be incomplete." | ||
| : completion.errorMessage || "Pi stopped with an error before completing the task.", | ||
| turn_id: completion.turnId, | ||
| notification: { type: "error" }, | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
cmux_notification_routed is forced true before the failure notification's own delivery is confirmed.
For length/error, stopPayload.cmux_notification_routed = true is set and the stop hook dispatched before sendHook("notification", ...) even runs, and its return value is discarded. If that notification call itself fails (e.g. a transient runCmux failure), the app-side generic Stop fallback is already permanently suppressed by the already-sent stop payload — the user ends up with no failure signal at all. Contrast this with the completed branch just below, which only sets the routed flag if (notificationRouted), i.e. only after confirming delivery.
At minimum, capture the notification's success and log a warning on failure so this silent-failure path is diagnosable; also consider whether "always suppress" is the right trade-off here given it can leave error runs with zero visible signal.
Based on path instructions ("...adds 'best effort' fallback branches that could misroute notifications or completion state").
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CLI/CMUXCLI`+PiExtensionSourcePart2.swift around lines 244 - 257, The failure
branch around sendHook("notification") marks cmux_notification_routed true
before confirming notification delivery, allowing a failed notification to
suppress all fallback signaling. Capture the notification send result, log a
warning when delivery fails, and only mark the stop payload as routed after
successful delivery; preserve the existing error/length messages and stop-hook
behavior while ensuring failed delivery remains diagnosable.
Source: Path instructions
|
Thank you for this, @peterthesling! You were first on the aborted-run part, which landed on main in #9451 shortly after. The length/error handling still isn't on main, but the Pi bridge has moved to the async version since #8537, so that piece would need porting there (and #12861 touches the same files). Would love to take it if you're up for it :) |
Summary
stopReasonanderrorMessageinstead of assuming every settled run completed successfullyValidation
python3 -m py_compile tests/test_pi_extension_install.pygit diff --checknotification.type: "completed"; passed after the fixtests/test_pi_extension_install.py: passed with the generated extension and a focused fake installer CLITagged app build was not run because this machine has Xcode 16.2 while the repository is pinned to Xcode 26.x. The focused test exercises the generated TypeScript extension end to end.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Treat incomplete Pi runs as errors instead of successful completions. Output-limit and provider failures now show error notifications; aborted runs no longer emit completion notifications.
stopReason,errorMessage) and preserve the last message.lengthanderror: emitstopfirst (withterminationReasonandcmux_notification_routed), then an errornotification.aborted: emit onlystopand mark the session idle; no completion notification.agent_settled.Written for commit 35036a5. Summary will update on new commits.