Fix blocking Pi extension hook dispatch - #8693
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 Pi extension now uses asynchronous serialized dispatch with bounded feeds, explicit target resolution, acknowledged ingestion, live ownership reconciliation, deadline-based socket operations, preserved tool failure status, and expanded Swift, Python, and CI regression coverage. ChangesPi extension dispatch and Feed delivery
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (6 errors, 2 warnings)
✅ Passed checks (17 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 replaces the Pi extension's blocking
Confidence Score: 5/5Safe to merge. All six issues raised in prior review rounds have documented fixes and regression harnesses covering the delivery-lane commit/timeout mutual exclusion, semaphore bound, publication ordering, surface-detection narrowing, waiter-registration race, and relay deadline budget. The FeedIngressSynchronousResult state machine correctly makes timeout and commit mutually exclusive via stateLock, remainingIngressTime includes attosecond precision, waiter registration is inside result.commit so it cannot observe a timed-out caller, and isSurfaceResolutionFailure is narrowed to exit code 69. No new correctness or isolation issues were found after reading the full production diff. Files Needing Attention: No files require special attention. The most structurally complex new files (FeedIngressDeliveryLane.swift, FeedCoordinator.swift, CMUXCLI+PiExtensionSourceDispatch.swift) are backed by the strongest test coverage in the PR. Important Files Changed
Sequence DiagramsequenceDiagram
participant Pi as Pi Extension Node
participant Disp as PiCmuxCommandDispatcher
participant CLI as cmux CLI
participant Lane as FeedIngressDeliveryLane
participant Result as FeedIngressSynchronousResult
participant Main as Main Actor
participant Store as WorkstreamStore
Pi->>Disp: enqueueFeed(key, command)
Disp->>Disp: scheduleFeed(sessionId)
Disp->>CLI: spawn feed.push args
CLI->>Lane: performAcceptedEventDelivery(events, timeout)
Lane->>Result: create SynchronousResult
Lane-->>Lane: executionQueue.async delivery
Note over Lane,Result: Socket worker blocks on result.wait(timeout)
Lane->>Main: DispatchQueue.main.sync
Main->>Result: result.commit resolveDeliveryTarget + ingestRevalidated
Result-->>Result: "state = .committed"
Main->>Store: store.ingest(event)
Main-->>Lane: onAcceptedOnMainActor callback
Lane->>Lane: onAccepted EventBus received+completed
Lane->>Result: result.complete semaphore.signal
Result-->>CLI: wait() returns committed value
CLI-->>Pi: status acknowledged item_id
CLI->>Disp: exit 0 or surface unavailable
Disp->>Disp: rememberSurfaceTarget or discardFeedForSession
Reviews (82): Last reviewed commit: "Fix Feed callback test synchronization s..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
a69effb to
bdf76b9
Compare
a21f92e to
8df86a0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CLI/CMUXCLI+PiExtensionSourcePart2.swift (1)
210-240: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
sendFeedswallows genuine command failures without any warning.
sendHook,ensureResumeBinding, andclearResumeBindingall callwarn(...)when a dispatched command fails for a reason other thansurfaceUnavailable.sendFeed's.catch(() => {})discards every failure unconditionally, so a real, non-"not_found" feed failure (bad payload, subprocess crash, etc.) produces zero diagnostic output — unlike every other dispatch path in this file.🩹 Proposed fix to warn on genuine feed failures
void runCmux( ["hooks", "feed", "--source", "pi", "--event", eventName], cwd, JSON.stringify(payload), context, "feed", - ).catch(() => {}); + ).then((result) => { + if (!result.ok && !result.surfaceUnavailable) { + warn(context, "cmux feed command failed", { + eventName, + status: result.status, + stderr_available: result.stderr.trim().length > 0, + error_available: result.error !== undefined, + }); + } + }).catch(() => {});🤖 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 210 - 240, Update sendFeed so its runCmux rejection handler suppresses only expected surface-unavailable/not-found failures and calls warn(...) for genuine feed command failures, matching the handling used by sendHook, ensureResumeBinding, and clearResumeBinding. Preserve the existing early returns and payload/dispatch behavior.
🤖 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`+PiExtensionSourceDispatch.swift:
- Around line 46-59: Replace the broad stdout/stderr text matching used by
isSurfaceResolutionFailure with a specific structured signal from the hook
command or CLIError, and classify failures using that signal (plus any required
structured status). Update both the failure handling around surfaceState and the
corresponding logic at lines 154-160 so dispatch is disabled only for explicitly
identified surface-resolution failures, not generic CLI errors containing
matching text.
In `@tests/test_pi_extension_dispatch.py`:
- Around line 71-380: Split the four scenario blocks in main() into separate
check_*() -> int helpers: responsiveness, feed backlog, timeout serialization,
and stale surface. Move each scenario’s setup, execution, assertions, and
failure returns into its helper, then have main() retain dependency setup,
temporary-directory management, helper invocation, and final success reporting
while preserving all existing return codes and diagnostics.
- Around line 20-42: Extract the shared Pi extension installation and override
logic from install_extension in tests/test_pi_extension_dispatch.py into
tests/claude_teams_test_utils.py alongside resolve_cmux_cli, preserving its
return path and error handling; update
tests/test_pi_extension_dispatch.py#L20-L42 to call the helper, and replace the
inline override-copy block in tests/test_pi_extension_install.py#L94-L96 with
the same helper call.
---
Outside diff comments:
In `@CLI/CMUXCLI`+PiExtensionSourcePart2.swift:
- Around line 210-240: Update sendFeed so its runCmux rejection handler
suppresses only expected surface-unavailable/not-found failures and calls
warn(...) for genuine feed command failures, matching the handling used by
sendHook, ensureResumeBinding, and clearResumeBinding. Preserve the existing
early returns and payload/dispatch behavior.
🪄 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: cba1e288-e786-4efd-957b-7661f8686fda
📒 Files selected for processing (8)
.github/workflows/ci.ymlCLI/CMUXCLI+PiExtensionSource.swiftCLI/CMUXCLI+PiExtensionSourceDispatch.swiftCLI/CMUXCLI+PiExtensionSourcePart1.swiftCLI/CMUXCLI+PiExtensionSourcePart2.swiftcmux.xcodeproj/project.pbxprojtests/test_pi_extension_dispatch.pytests/test_pi_extension_install.py
8df86a0 to
3791cff
Compare
3791cff to
20f2dea
Compare
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 `@tests/test_pi_extension_dispatch.py`:
- Around line 296-299: Replace the fixed-duration waits in the cancellation and
completion harnesses around session_shutdown and the completion flow with
deadline-bounded polling of the relevant log predicate. Await a real completion
signal, such as observing the expected five “hooks feed” calls and stable
cancellation output, before allowing the process to exit and performing Python
assertions; retain a timeout so failures terminate deterministically.
🪄 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: 0f7b27c3-29d9-49d9-9b36-f5c2fe64d61e
📒 Files selected for processing (8)
.github/workflows/ci.ymlCLI/CMUXCLI+PiExtensionSource.swiftCLI/CMUXCLI+PiExtensionSourceDispatch.swiftCLI/CMUXCLI+PiExtensionSourcePart1.swiftCLI/CMUXCLI+PiExtensionSourcePart2.swiftcmux.xcodeproj/project.pbxprojtests/test_pi_extension_dispatch.pytests/test_pi_extension_install.py
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_dispatch.py (1)
233-401: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit
check_feed_lifecycleinto per-scenario helpers.This function packs two independent scenarios — queued-feed cancellation on
session_shutdownand terminal-lifecycle completion ordering/backlog-cancellation — into one ~168-line function, unlike every othercheck_*function in this file which owns exactly one scenario (a pattern this file already adopted after a prior complexity-driven refactor ofmain()). Splitting into e.g.check_feed_cancellation()andcheck_completion_order()would keep the file consistent and easier to extend.🤖 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_dispatch.py` around lines 233 - 401, Split check_feed_lifecycle into two independent helpers: check_feed_cancellation for the session_shutdown queued-feed cancellation scenario and check_completion_order for terminal completion ordering and backlog cancellation. Move each scenario’s setup, execution, assertions, and failure reporting into its respective helper, then update the caller to invoke both helpers while preserving their existing return-code behavior.
🤖 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_dispatch.py`:
- Around line 233-401: Split check_feed_lifecycle into two independent helpers:
check_feed_cancellation for the session_shutdown queued-feed cancellation
scenario and check_completion_order for terminal completion ordering and backlog
cancellation. Move each scenario’s setup, execution, assertions, and failure
reporting into its respective helper, then update the caller to invoke both
helpers while preserving their existing return-code behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ba35d01e-805d-41d5-bfc4-15ee8823bca1
📒 Files selected for processing (2)
CLI/CMUXCLI+PiExtensionSourceDispatch.swifttests/test_pi_extension_dispatch.py
|
@coderabbitai review |
✅ Action performedReview finished.
|
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 (2)
CLI/CMUXCLI+PiExtensionSourceDispatch.swift (1)
54-60: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve terminal state after promotion to the priority queue.
Line 54 only checks
pendingFeedCommands, but Line 75 removes terminal entries into an unkeyed queue and Lines 134-145 dequeue them without retaining their key. A late start for that key is then accepted and can execute after completion—even afterfinishFeedForSessionresolves. Track terminal keys while queued/active, or reject feeds for a session once finishing begins.Also applies to: 71-76, 134-145
🤖 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`+PiExtensionSourceDispatch.swift around lines 54 - 60, Preserve terminal state for commands promoted from pendingFeedCommands into the priority queue: update the feed-dispatch flow around pendingFeedCommands, finishFeedForSession, and queue dequeue handling to retain terminal keys or reject new feeds once a session begins finishing. Ensure late non-terminal events for a terminal or finishing key cannot be accepted or executed after completion, while preserving normal ordering for valid commands.tests/test_pi_extension_dispatch.py (1)
233-401: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider splitting
check_feed_lifecycleinto two focused helpers.This function still runs two logically distinct scenarios (queued-feed cancellation on shutdown, and completion-order/overlap validation on terminal lifecycle) in one ~170-line body, flagged as high complexity. This mirrors the same maintainability concern already raised and fixed for
main()in a prior review pass — splitting intocheck_feed_cancellation_on_shutdown()andcheck_feed_completion_order()would keep the same precedent consistent across the file.🤖 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_dispatch.py` around lines 233 - 401, Split check_feed_lifecycle into two focused helpers: check_feed_cancellation_on_shutdown for the cancellation scenario and check_feed_completion_order for completion ordering and overlap validation. Move each scenario’s setup, execution, assertions, and failure reporting into its helper, then have check_feed_lifecycle invoke both and return failure immediately when either fails, preserving existing behavior.
🤖 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 `@CLI/CMUXCLI`+PiExtensionSourceDispatch.swift:
- Around line 54-60: Preserve terminal state for commands promoted from
pendingFeedCommands into the priority queue: update the feed-dispatch flow
around pendingFeedCommands, finishFeedForSession, and queue dequeue handling to
retain terminal keys or reject new feeds once a session begins finishing. Ensure
late non-terminal events for a terminal or finishing key cannot be accepted or
executed after completion, while preserving normal ordering for valid commands.
In `@tests/test_pi_extension_dispatch.py`:
- Around line 233-401: Split check_feed_lifecycle into two focused helpers:
check_feed_cancellation_on_shutdown for the cancellation scenario and
check_feed_completion_order for completion ordering and overlap validation. Move
each scenario’s setup, execution, assertions, and failure reporting into its
helper, then have check_feed_lifecycle invoke both and return failure
immediately when either fails, preserving existing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d86f50d1-0787-4b50-bd70-1b13a4792c51
📒 Files selected for processing (2)
CLI/CMUXCLI+PiExtensionSourceDispatch.swifttests/test_pi_extension_dispatch.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. |
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. |
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. |
…on-spawnsync-blocking # Conflicts: # CLI/cmux.swift # Packages/iOS/CmuxMobileTerminalKit/Tests/CmuxMobileTerminalKitTests/TerminalDockKeyboardTransitionPlannerTests.swift # cmux.xcodeproj/project.pbxproj
…-8672-pi-extension-spawnsync-blocking
…-8672-pi-extension-spawnsync-blocking
1 participant
Summary
spawnSynchook path with an asynchronous, serializedspawndispatcher so Pi's Node event loop remains responsiveThis may also address the similar freeze/queue symptom in #6507, but that report's exact scenario was not reproduced, so this PR does not close it.
Concrete before/after reproduction
Using Pi 0.73.0 with the xAI
grok-3-faststreaming provider:The prior Pi extension and settings were restored byte-for-byte after the run.
Red/green regression evidence
The behavioral Bun harness fails against the original synchronous dispatcher:
The final branch passes:
Earlier hosted red proofs captured ownership and total-deadline failures:
6105ca937d: the batch-snapshot race delivered only 5 of 64 payloads.b085ba9c05: a slowly drained request exceeded one operation deadline and blank explicit Pi targets were accepted.The corrected final test-only proof run 30165033218, commit
e05f5dd077, keeps the new tests while omitting the corresponding final source fixes. It records the intended failures, including:The final red/green commit pairs are:
f2ba6498da/6b152c219a— deadline composition coverage/fix8b8083725f/24c8eecf05— stalled callback coverage/lock-scope fix8288228764/0cd0465711— authoritative handoff coverage/fixbaa6c92bc9/e4064278ed— cross-session isolation coverage/fix84c91727a1/e1796976c8— bounded fallback/publication coverage/fixc378b4fb93— correct the final test semaphore's synchronization scopeThe temporary red-proof branch was deleted after the hosted evidence was captured.
Exact-head verification
Final pushed HEAD:
c378b4fb93a591eef13742435c0d82c2c7c984d2origin/mainis an ancestor of HEAD; merge-conflict gate is cleanissue-8672-route)git diff --check: passExact-head CI run 30164877041:
github.com)tests-build-and-lag: the complete test build succeeds, then the repository-wide warning budget fails on four files untouched by this PR (AppDelegate+WindowDock.swift,ContentView.swift,MobileTerminalRenderGridAnchorRegistry.swift, andSidebarGroupHeaderRowView.swift)swift-package-tests: package compilation reaches final link, then fails because the CI Ghostty static library lacks_ghostty_surface_render_grid_json_v2The latter two failures are current-main/toolchain infrastructure signatures; they are recorded here rather than hidden or represented as green.
Review closeout
Canonical semantic autoreview on exact HEAD returned:
{ "findings": [], "overall_correctness": "patch is correct", "overall_confidence": 0.90 }Greptile's exact-head check succeeded; its substantive summary rates the patch 5/5 and safe to merge. GitHub reports zero unresolved review threads.
The canonical wrapper itself exits nonzero because its static cmux policy phase reports 12 matches. Those matches were individually triaged and intentionally retained:
This is an explicit owner-documented exception, not a claim that the static wrapper exited 0. An actor rewrite or new micro-package is intentionally outside this bug fix.
Localization audit
The changed user-facing surfaces are CLI/socket and
feed.pusherrors.Resources/Localizable.xcstringsparses successfully; all 12 added keys have translated English and Japanese values. Added localized references were cross-checked against the catalog, and the final review commits add no user-facing text.Closes #8672