Notify only after Pi agent settles - #8574
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe generated Pi extension stores completion data at ChangesPi completion settlement
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Pi
participant PiExtension
participant CmuxHooks
Pi->>PiExtension: agent_end with completion data
PiExtension->>PiExtension: store pendingCompletion
Pi->>PiExtension: agent_settled when idle
PiExtension->>PiExtension: settleTurn(sessionId)
PiExtension->>CmuxHooks: notification
PiExtension->>CmuxHooks: stop with last assistant message and turn id
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ 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 |
Greptile SummaryThis PR delays Pi completion notifications until the agent is fully settled. The main changes are:
Confidence Score: 5/5The latest changes look safe to merge.
Important Files Changed
Reviews (5): Last reviewed commit: "fixup! Resolve npm-linked Pi package ver..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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`+PiExtensionSourcePart1.swift:
- Around line 94-132: Update supportsAgentSettled so detectedPiVersion returning
no version or an unparseable version defaults to false rather than true.
Preserve the existing version threshold logic for reliably parsed versions,
ensuring undetectable installations follow the immediate completion-notification
path.
🪄 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
Run ID: 7294ea41-f392-4fab-be5d-54789e129cc2
📒 Files selected for processing (3)
CLI/CMUXCLI+PiExtensionSourcePart1.swiftCLI/CMUXCLI+PiExtensionSourcePart2.swifttests/test_pi_extension_install.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_pi_extension_install.py (1)
287-337: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winShutdown right after a successful settlement isn't checked for a duplicate stop hook.
Line 313 calls
session_shutdown({reason:"quit"}, ctx)immediately afteragent_settledalready published notification+stop (line 308-310) and after a duplicate-settlement no-op check (line 311-312). No assertion verifiescompletionHookCount()is unchanged by this shutdown call, and the very next session resetscompletionCount = await completionHookCount()at line 323 — silently absorbing any extra "stop" emitted here. Context snippet 3 (session_shutdowngated byif (!state.stopped)) suggests this is handled, but that's exactly the invariant this PR is meant to lock in for duplicate/early/late completion (per PR objective), and the interrupted-session flow already tests the mirror case (lateagent_settledafter shutdown, 332-333) — this symmetric case for "shutdown after already-settled" deserves the same explicit lock-in.Based on
.github/review-bot-rules/reliability-single-source-of-truth.md, which calls for consolidating to a single authority instead of leaving a fallback/duplicate path unverified.✅ Suggested assertion to add before line 314
await handlers.get("agent_settled")({}, ctx); if (await completionHookCount() !== completionCount) throw new Error("duplicate agent_settled emitted completion twice"); +const preShutdownCount = completionCount; await handlers.get("session_shutdown")({ reason: "quit" }, ctx); +if (await completionHookCount() !== preShutdownCount) throw new Error("shutdown after settlement emitted a duplicate stop");🤖 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 `@tests/test_pi_extension_install.py` around lines 287 - 337, Add an explicit count-preservation assertion around the first session_shutdown call after agent_settled in the main ctx flow: capture completionHookCount() before shutdown, invoke session_shutdown({ reason: "quit" }, ctx), and verify the count is unchanged. Keep the existing duplicate-settlement and interrupted-session checks intact so shutdown after successful settlement is covered as a no-op.Source: Path instructions
🤖 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 `@tests/test_pi_extension_install.py`:
- Around line 390-433: Refactor the repeated fallback scenarios in
tests/test_pi_extension_install.py:390-433 by adding a helper inside
check_source (such as testFallbackSession) that performs setup, handler calls,
and completion-count validation, then invoke it for legacy, unknown, and
malformed sessions; also refactor tests/test_pi_extension_install.py:549-572
with a find_stop_payload(payloads, session_id) helper and use it for all three
payload assertions.
---
Outside diff comments:
In `@tests/test_pi_extension_install.py`:
- Around line 287-337: Add an explicit count-preservation assertion around the
first session_shutdown call after agent_settled in the main ctx flow: capture
completionHookCount() before shutdown, invoke session_shutdown({ reason: "quit"
}, ctx), and verify the count is unchanged. Keep the existing
duplicate-settlement and interrupted-session checks intact so shutdown after
successful settlement is covered as a no-op.
🪄 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
Run ID: dfb99f4b-9a3e-40a5-a5fc-2bcfa702fc5a
📒 Files selected for processing (2)
CLI/CMUXCLI+PiExtensionSourcePart1.swifttests/test_pi_extension_install.py
|
🤖 Updated by Pi — added the explicit count-preservation assertion proving |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_pi_extension_install.py (1)
335-338: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert that hooks-disabled mode suppresses completion hooks.
The scenario invokes
session_startandsession_shutdownbut never checks the result, so a regression could emit completion/stop hooks whileCMUX_PI_HOOKS_DISABLED=1and still pass.Suggested assertion
process.env.CMUX_PI_HOOKS_DISABLED = "1"; +const disabledCompletionCount = await completionHookCount(); const disabledCtx = { ... await handlers.get("session_shutdown")({ reason: "disabled" }, disabledCtx); +if (await completionHookCount() !== disabledCompletionCount) { + throw new Error("hooks-disabled mode emitted completion hooks"); +} delete process.env.CMUX_PI_HOOKS_DISABLED;🤖 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 `@tests/test_pi_extension_install.py` around lines 335 - 338, Update the hooks-disabled test around disabledCtx and its session_start/session_shutdown calls to capture the emitted events and assert that no completion or stop hooks are produced when CMUX_PI_HOOKS_DISABLED is set to "1". Preserve the existing setup and lifecycle invocation while making the suppression behavior explicitly testable.
🤖 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.
Outside diff comments:
In `@tests/test_pi_extension_install.py`:
- Around line 335-338: Update the hooks-disabled test around disabledCtx and its
session_start/session_shutdown calls to capture the emitted events and assert
that no completion or stop hooks are produced when CMUX_PI_HOOKS_DISABLED is set
to "1". Preserve the existing setup and lifecycle invocation while making the
suppression behavior explicitly testable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 03ac5cfb-93e7-4aff-9a76-12455ceba937
📒 Files selected for processing (1)
tests/test_pi_extension_install.py
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
🤖 Updated by Pi — added an explicit assertion that |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_pi_extension_install.py (1)
386-392: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert
Notification→Stopfor every fallback session.The legacy, unknown, and malformed cases currently verify only that two completion commands occurred, while the later payload checks require only a
Stop. A regression that omitsNotificationor emits twoStophooks would pass despite breaking the preserved fallback contract. Filter completion payloads by session and assert the exact sequence["Notification", "Stop"].Also applies to: 408-414, 430-436, 540-575
🤖 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 `@tests/test_pi_extension_install.py` around lines 386 - 392, Update the fallback-session assertions around the legacy, unknown, and malformed cases, including the analogous later cases, to filter completion payloads by session and assert the exact command sequence ["Notification", "Stop"]. Replace count-only checks so missing Notification or duplicate Stop emissions fail while preserving the existing fallback validation.
🤖 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.
Outside diff comments:
In `@tests/test_pi_extension_install.py`:
- Around line 386-392: Update the fallback-session assertions around the legacy,
unknown, and malformed cases, including the analogous later cases, to filter
completion payloads by session and assert the exact command sequence
["Notification", "Stop"]. Replace count-only checks so missing Notification or
duplicate Stop emissions fail while preserving the existing fallback validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b763f89b-107b-455c-9447-ae01e088fc45
📒 Files selected for processing (1)
tests/test_pi_extension_install.py
|
🤖 Updated by Pi — fallback coverage now asserts the exact |
2 participants
Context
Pi's
agent_endmarks a low-level run, so cmux can announce completion before automatic retries, compaction, or follow-up work. Fixes #8568.Summary
agent_settledand idleDependencies
None.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Delay Pi completion notifications until the agent has settled and the context is idle to avoid premature “done” signals during retries. Preserves legacy behavior for Pi versions before 0.80.5 and handles npm-linked, unknown, or malformed installs. Fixes #8568.
agent_settledonly whenctx.isIdle(); de-duplicate settlements.agent_endfor legacy/unknown/malformed Pi with correct stop payload.notificationbeforestopand setcmux_notification_routedwhen delivered.stop; ignore late settlements; respect hooks-disabled mode.package.json(realpath of npm bin symlink); supports@earendil-works/pi-coding-agentand@mariozechner/pi-coding-agent; gate modern behavior to >= 0.80.5.Written for commit 2c70f14. Summary will update on new commits.
Summary by CodeRabbit