Skip to content

Treat incomplete Pi runs as errors, not completions - #9021

Open
peterthesling wants to merge 2 commits into
manaflow-ai:mainfrom
peterthesling:fix/pi-incomplete-notifications
Open

peterthesling wants to merge 2 commits into
manaflow-ai:mainfrom
peterthesling:fix/pi-incomplete-notifications

Conversation

@peterthesling

@peterthesling peterthesling commented Jul 28, 2026 •

Copy link
Copy Markdown

Summary

  • read Pi's final assistant stopReason and errorMessage instead of assuming every settled run completed successfully
  • report output-limit and provider failures as errors, with the failure notification left as the final visible lifecycle state
  • suppress completion notifications for user-aborted runs while still marking the session idle
  • preserve the existing completion path and legacy Pi compatibility

Validation

  • python3 -m py_compile tests/test_pi_extension_install.py
  • git diff --check
  • generated-extension runtime regression harness: failed before the fix with notification.type: "completed"; passed after the fix
  • tests/test_pi_extension_install.py: passed with the generated extension and a focused fake installer CLI

Tagged 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.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with 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.

  • Bug Fixes
    • Parse assistant outcome (stopReason, errorMessage) and preserve the last message.
    • For length and error: emit stop first (with terminationReason and cmux_notification_routed), then an error notification.
    • For aborted: emit only stop and mark the session idle; no completion notification.
    • Preserve legacy completion behavior for Pi versions without agent_settled.
    • Added tests for length-capped, provider error, and aborted flows, verifying hook order and payloads.

Written for commit 35036a5. Summary will update on new commits.

Review in cubic

Assisted-By: devx/29ded3f8-7889-44f9-9d2d-4f87a80c090b
Assisted-By: devx/29ded3f8-7889-44f9-9d2d-4f87a80c090b
@coderabbitai

coderabbitai Bot commented Jul 28, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The Pi extension now propagates structured assistant outcomes, routes lifecycle hooks by stop reason, and tests completed, length-capped, provider-error, and aborted sessions.

Changes

Pi completion outcome routing

Layer / File(s) Summary
Structured assistant outcome extraction
CLI/CMUXCLI+PiExtensionSourcePart1.swift
Assistant outcomes now include optional message and error text plus a resolved stop reason.
Stop-reason completion hook routing
CLI/CMUXCLI+PiExtensionSourcePart2.swift
Completion publishing emits termination-aware stop hooks and conditionally emits error or completed notifications.
Lifecycle outcome verification
tests/test_pi_extension_install.py
Tests cover hook ordering, payloads, resume operations, and notification suppression for aborted sessions.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Suggested reviewers: mrohan-sq, lawrencecchen

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
Loading

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (2 errors)

Check name Status Explanation Resolution
Cmux User-Facing Error Privacy ❌ Error The error notification uses completion.errorMessage directly, so raw upstream/provider text can reach a user-visible alert. Replace errorMessage in notification text with generic product wording; keep provider-specific details only in sanitized logs or internal telemetry.
Cmux Full Internationalization ❌ Error Part2 adds user-facing Pi failure messages as raw literals; no matching localized keys exist in Resources/Localizable.xcstrings for the catalog's en/ja locales. Use String(localized:defaultValue:) (or equivalent) for the new failure texts and add matching translated xcstrings entries for every existing locale.
✅ Passed checks (23 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly matches the main change: treating incomplete Pi runs as errors instead of completions.
Description check ✅ Passed Summary and validation are solid, but the Demo Video, Review Trigger, and Checklist sections from the template are missing.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Cmux Swift Actor Isolation ✅ Passed The touched Swift files only changed embedded TypeScript string literals; no Swift declarations, actor annotations, Sendable refs, or UI-store isolation changed.
Cmux Swift Blocking Runtime ✅ Passed PASS: the PR only adjusts embedded TypeScript lifecycle logic and Python test polling; no new Swift semaphores, waits, sleeps, syncs, or locks were introduced.
Cmux Browser Automation Off-Main ✅ Passed Diff only touches Pi extension lifecycle code; no browser.* routing, WebKit/AppKit waits, or socket-worker policy changes appear, so the browser-automation rule isn’t implicated.
Cmux Expensive Synchronous Load ✅ Passed Touched files only change Pi hook routing/test coverage; no RestorableAgentSessionIndex.load(), agent-store scans, or main-actor/interactive history loads were added.
Cmux Cache Substitution Correctness ✅ Passed Only in-memory Pi lifecycle state changed; no durable persistence/history/snapshot path replaced a fresh authoritative read with a cache.
Cmux No Hacky Sleeps ✅ Passed No production sleep/timer/polling was added; only a test-only bounded poll in tests/test_pi_extension_install.py, which the rule allows.
Cmux Algorithmic Complexity ✅ Passed The new Pi logic uses a single reverse scan over one event payload and no nested rescans, sorting, or repeated filtering in a hot path.
Cmux Swift Concurrency ✅ Passed No new DispatchQueue/Combine/Task legacy patterns were added; the async handlers are third-party callback boundaries, which the rule allows.
Cmux Swift @Concurrent ✅ Passed Touched Swift files only embed TypeScript; no Swift @concurrent, nonisolated async, actor-isolated, or UI-hop changes were introduced.
Cmux Swift Package Boundaries ✅ Passed The diff only updates generated Pi extension source in the CLI app target plus tests; this is lifecycle glue/vendored code, which the rule explicitly allows.
Cmux Swiftpm Lockfiles ✅ Passed HEAD only changes two Swift files; no Package.swift, Package.resolved, project.pbxproj, or .gitignore diffs are present.
Cmux Swift Logging ✅ Passed The diff changes Pi lifecycle logic only; no new print/debugPrint/dump/NSLog, Logger, or file/stdout diagnostics were added to production Swift.
Cmux Swiftui State Layout ✅ Passed Diff only edits embedded Pi TypeScript in Swift string literals; no SwiftUI views, state objects, GeometryReader, lazy row store refs, or render-time state writes.
Cmux Architecture Rethink ✅ Passed The PR is a small correctness fix: it refines completion/notification routing with clear owners, and only uses sleep in tests, which the rule allows.
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PASS: The PR only changes embedded Pi hook TypeScript and tests; no NSWindow/WindowGroup code, cmuxAuxiliaryWindowIdentifiers, or close-shortcut routing is introduced.
Cmux Source Artifacts ✅ Passed Only intentional source files changed; no logs, caches, build output, temp dirs, or other source-control artifacts appear in the diff.
Cmux No Test Or Debug Seam In Production Source ✅ Passed PASS: The only modified Swift files are under CLI/, not a production Sources/ path, and no new debug/test seam members were added.
Cmux No Ambient Global State ✅ Passed Touched Swift files only add/modify static let string literals inside extension CMUXCLI; no new top-level func/var/singleton or ambient state appears.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

📥 Commits

Reviewing files that changed from the base of the PR and between e50bba3 and 35036a5.

📒 Files selected for processing (3)
  • CLI/CMUXCLI+PiExtensionSourcePart1.swift
  • CLI/CMUXCLI+PiExtensionSourcePart2.swift
  • tests/test_pi_extension_install.py

Comment on lines 227 to 266
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);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 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" . || true

Repository: 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:


🏁 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)
PY

Repository: 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

Comment on lines +244 to +257
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;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 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

@teamleaderleo

Copy link
Copy Markdown
Collaborator

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 :)

@teamleaderleo teamleaderleo added S2: major A crash, hang, lost state, broken connection, or a regression on a path people use area: agents Agent integrations (Claude Code, Codex, ACP), agent chat, hooks, status area: notifications Notifications, banners, badges, the bell labels Sep 30, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: agents Agent integrations (Claude Code, Codex, ACP), agent chat, hooks, status area: notifications Notifications, banners, badges, the bell S2: major A crash, hang, lost state, broken connection, or a regression on a path people use

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants