Keep Pi hooks and unread updates off UI-critical paths - #9289
Merged
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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 PR refreshes stale managed Pi extensions, dispatches Pi lifecycle hooks asynchronously with shutdown draining, and consolidates sidebar unread state and titlebar layout calculations into shared models. ChangesPi lifecycle handling
Unread state and titlebar layout
Identity and argument normalization
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (22 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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`+PiExtension.swift:
- Around line 29-39: Update refreshManagedPiExtensionIfNeeded and its atomic
write path to revalidate that extensionURL still refers to the same regular file
observed by existingPiExtensionContents before replacing it; fail closed without
writing if the file was replaced or changed. Add a regression test that
simulates replacement between the read and write and verifies newer user-owned
content is preserved.
In `@tests/test_pi_extension_install.py`:
- Around line 104-130: Update the subprocess invocation in the pi extension
refresh test to capture its CompletedProcess result, then assert the expected
successful return code for the missing-socket session-start invocation. Include
captured stdout and stderr in the assertion failure message so command errors
are diagnosable.
🪄 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: c9be3e2f-6f42-4e84-9d70-7bbd3af092c6
📒 Files selected for processing (3)
CLI/CMUXCLI+PiExtension.swiftCLI/cmux.swifttests/test_pi_extension_install.py
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 369-395: Scope lifecycle task tracking to each session instead of
sharing the global lifecycleTasks set: update trackLifecycleTask and its callers
to associate tasks with the authoritative sessionId, and update
drainLifecycleTasks to drain only the shutting-down session’s tasks. Ensure
session_shutdown uses that session-specific drain so unrelated blocked or
continuous work cannot delay cleanup, and add a regression case covering one
session blocked while another shuts down.
In `@cmuxTests/UpdatePillReleaseVisibilityTests.swift`:
- Around line 250-283: The test testLayoutModelOnlyRecomputesForTitlebarInputs
must drain RunLoop.main after each notificationCenter.post call before asserting
computationCount or snapshot values. Add a small main-run-loop drain after the
unrelated UserDefaults, titlebar-style, KeyboardShortcutSettings, and
GlobalFontMagnification notifications, preserving the existing assertions and
expected counts.
In `@cmuxTests/WorkspaceContentViewVisibilityTests.swift`:
- Around line 261-343: The testUnreadChangeDoesNotReevaluateContentViewRoot test
currently applies unread state to a standalone SidebarUnreadModel while
ContentView.sidebarUnread uses TerminalNotificationStore.shared.sidebarUnread.
Update the test to apply the unread state through
TerminalNotificationStore.shared.sidebarUnread, or alias the injected model to
that shared instance, ensuring the sidebar observes the same model as
production.
In `@Sources/ContentView.swift`:
- Around line 823-828: Remove `@ObservedObject` observation of
TitlebarControlsLayoutModel from ContentView and stop deriving titlebar layout
state at the ContentView level. Keep fullscreenControlsWidth computation local
to the consumer overlay, where TitlebarControlsView already observes
TitlebarControlsLayoutModel, and refresh only that overlay on snapshot changes
using the existing sidebarUnread.$snapshot.dropFirst() pattern.
In `@tests/test_pi_extension_dispatch.py`:
- Around line 164-216: Update check_ui_lifecycle_handlers_return_immediately so
the fake cmux script blocks on a per-invocation release marker instead of using
the fixed sleep. Replace measure’s performance.now()/75 ms assertions with a
release-signal check: invoke each handler, assert its promise returns before
creating the corresponding marker, then create the marker and retain the bounded
log poll to verify all commands eventually complete.
🪄 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: d8288383-4890-40b7-a130-8bd09c181870
📒 Files selected for processing (11)
CLI/CMUXCLI+PiExtensionSourcePart2.swiftSources/ContentView.swiftSources/DockUnreadPanelProjection.swiftSources/MinimalModeSidebarTitlebarControlsOverlay.swiftSources/TerminalNotificationStore.swiftSources/Update/UpdateTitlebarAccessory.swiftcmuxTests/FileDropOverlayViewTests.swiftcmuxTests/UpdatePillReleaseVisibilityTests.swiftcmuxTests/WorkspaceContentViewVisibilityTests.swifttests/test_pi_extension_dispatch.pytests/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. |
lawrencecchen
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@cmuxTests/MobileHostIdentityTests.swift`:
- Around line 193-207: The notification observer in the
MobileHostIdentity.deviceID test must use a concurrency-safe, notification-only
counter instead of mutating the captured notificationCount directly. Update the
observer and final assertion to use the guarded or otherwise isolated counter,
preserving the expectation that deviceID does not emit
UserDefaults.didChangeNotification.
In `@Sources/Mobile/MobileHostIdentity.swift`:
- Around line 86-90: Update persistDeviceIDIfNeeded to normalize the stored
UserDefaults value with the same canonicalization used by readSharedDeviceID and
settleSharedDeviceID before comparing it with id, while preserving the early
return for equivalent IDs. Add a regression test covering an uppercase legacy
stored UUID and verify UserDefaults.set is not invoked.
In `@Sources/TerminalNotificationStore.swift`:
- Around line 2320-2324: Remove the `= false` default from
`SidebarWorkspaceUnreadSummary.hasLatestNotification`. Update every initializer
call, including the constructions in `WorkspaceContentViewVisibilityTests`, to
pass `hasLatestNotification` explicitly and preserve the intended relationship
with `latestNotificationText`.
🪄 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: f550643a-6daf-4c0c-935f-4090590e8392
📒 Files selected for processing (12)
Sources/ContentView.swiftSources/Mobile/MobileHostIdentity.swiftSources/Sidebar/AppKitList/Cells/SidebarGroupHeaderRowModel.swiftSources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowModel.swiftSources/Sidebar/AppKitList/SidebarWorkspaceTableController.swiftSources/Sidebar/AppKitList/SidebarWorkspaceTableRowConfiguration.swiftSources/Sidebar/AppKitList/SidebarWorkspaceTableView.swiftSources/TerminalNotificationStore.swiftSources/VerticalTabsSidebar+WorkspaceGroups.swiftcmuxTests/MobileHostIdentityTests.swiftcmuxTests/SidebarLazyLayoutScaleTests.swiftcmuxTests/WorkspaceContentViewVisibilityTests.swift
# Conflicts: # cmuxTests/WorkspaceContentViewVisibilityTests.swift
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. |
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 `@Sources/Update/UpdateTitlebarAccessory.swift`:
- Around line 152-231: Update Sources/Update/UpdateTitlebarAccessory.swift lines
152-231 in TitlebarControlsLayoutModel.recompute() to derive the next style and
contentSize, compare both with snapshot, and return without reassigning snapshot
or incrementing revision when unchanged. Update Sources/ContentView.swift lines
810-830 to remove the `@ObservedObject` titlebarControlsLayoutModel property;
instead maintain the fullscreenControlsWidth value used near line 2078 and
refresh it through a targeted
onReceive(titlebarControlsLayoutModel.$snapshot.dropFirst()) handler, following
the existing sidebarUnread.$snapshot.dropFirst() pattern.
🪄 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: b79c01f9-54e6-40dd-bd15-921a37402fa6
📒 Files selected for processing (3)
CLI/cmux.swiftSources/ContentView.swiftSources/Update/UpdateTitlebarAccessory.swift
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 (3)
Sources/VerticalTabsSidebar+WorkspaceGroups.swift (1)
284-303: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winReuse the snapshot predicates instead of re-implementing the unread rule.
Lines 286, 289, 299, and 302 each inline
(unreadSummariesByWorkspaceId[$0]?.unreadCount ?? 0) > 0or its negation.SidebarUnreadSnapshotalready owns that rule asworkspaceIsUnread(forWorkspaceId:),canMarkWorkspaceRead(forWorkspaceIds:), andcanMarkWorkspaceUnread(forWorkspaceIds:). Four inline copies can drift from the shared definition if the unread rule changes.Pass the
SidebarUnreadSnapshotinto this method and call its predicates.anchorIdsthen becomes unnecessary.♻️ Proposed refactor
- let anchorIds = [group.anchorWorkspaceId] - let canMarkAnchorRead = anchorIds.contains { - (unreadSummariesByWorkspaceId[$0]?.unreadCount ?? 0) > 0 - } - let canMarkAnchorUnread = anchorIds.contains { - (unreadSummariesByWorkspaceId[$0]?.unreadCount ?? 0) == 0 - } + let canMarkAnchorRead = unreadSnapshot.canMarkWorkspaceRead( + forWorkspaceIds: [group.anchorWorkspaceId] + ) + let canMarkAnchorUnread = unreadSnapshot.canMarkWorkspaceUnread( + forWorkspaceIds: [group.anchorWorkspaceId] + ) @@ let nonAnchorMemberIds = memberWorkspaceIds.filter { $0 != group.anchorWorkspaceId } - let canMarkAllRead = nonAnchorMemberIds.contains { - (unreadSummariesByWorkspaceId[$0]?.unreadCount ?? 0) > 0 - } - let canMarkAllUnread = nonAnchorMemberIds.contains { - (unreadSummariesByWorkspaceId[$0]?.unreadCount ?? 0) == 0 - } + let canMarkAllRead = unreadSnapshot.canMarkWorkspaceRead(forWorkspaceIds: nonAnchorMemberIds) + let canMarkAllUnread = unreadSnapshot.canMarkWorkspaceUnread(forWorkspaceIds: nonAnchorMemberIds)🤖 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 `@Sources/VerticalTabsSidebar`+WorkspaceGroups.swift around lines 284 - 303, Update the affected method in VerticalTabsSidebar+WorkspaceGroups to accept and use the existing SidebarUnreadSnapshot. Replace the inline unread-count checks for anchor and non-anchor workspaces with workspaceIsUnread(forWorkspaceId:), canMarkWorkspaceRead(forWorkspaceIds:), and canMarkWorkspaceUnread(forWorkspaceIds:) as appropriate, and remove the now-unnecessary anchorIds variable.CLI/cmux.swift (2)
3791-3800: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMap only retryable failures to the transient restore message.
This catch handles every socket connection failure. Permission errors, stale sockets, protocol failures, and other permanent errors can be reported as “cmux is still opening.” Classify the error before returning the retry message. Re-throw non-retryable failures.
🤖 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/cmux.swift` around lines 3791 - 3800, Update the restore socket-error handling around loggedRestoreError so the transient “cmux is still opening” message is used only for errors classified as retryable startup failures. Re-throw permission, stale-socket, protocol, and other non-retryable connection errors instead of mapping them to the retry message, while preserving the existing retry behavior and error details for eligible failures.
28327-28332: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winHandle
surface.resume.setfailures instead of swallowing them.
CLI/cmux.swift:28333stores the hook-captured restore fields inparams, but the request result is discarded with_ = try? client.sendV2(...). Ifsurface.resume.setrejects these fields or the socket call fails, hook-driven restore can continue without the newlaunch_command/permission_modedata. Use a controlled error path or omit-only-if-unsupported negotiation.🤖 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/cmux.swift` around lines 28327 - 28332, Update the surface.resume.set request flow following the launch_command and permission_mode assignments so failures from client.sendV2 are no longer discarded via try?. Handle the error through the existing controlled error path, or explicitly retry/omit the restore fields only when the server reports they are unsupported, while preserving successful hook-driven restoration.
🤖 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
`@Packages/macOS/CmuxNotifications/Tests/CmuxNotificationsTests/SidebarUnreadSnapshotTests.swift`:
- Around line 60-83: Update the suppression test around SidebarUnreadModel.apply
to observe model publication/state changes with withObservationTracking,
following the approach in WorkspaceContentViewVisibilityTests.swift, rather than
relying on the buffered snapshotChanges stream. Assert that applying an
equivalent snapshot does not publish a change, while retaining the separate
stream-delivery test for the distinct unread-state update.
In `@Sources/CMUXInstalledExtensionSidebarHostView.swift`:
- Around line 305-314: Update the unread-handling loop in the .task block to
refresh snapshotCache from snapshotProvider() immediately before
applyUnread(unreadSnapshot), ensuring unread updates use the complete current
workspace list. Preserve the existing cancellation check, unread application,
and xpcHost.sendSnapshotDidChange flow.
In `@Sources/Sidebar/AppKitList/SidebarWorkspaceTableController.swift`:
- Line 37: Document the isolation invariant beside unreadTask in
Sources/Sidebar/AppKitList/SidebarWorkspaceTableController.swift:37-37, stating
it is written only by setUnreadSource, dismantleContainerView, and deinit, and
that nonisolated(unsafe) is solely required for deinit cancellation. Add the
same note beside unreadTask in Sources/DockUnreadPanelProjection.swift:17-17,
naming init and deinit as its only write sites.
---
Outside diff comments:
In `@CLI/cmux.swift`:
- Around line 3791-3800: Update the restore socket-error handling around
loggedRestoreError so the transient “cmux is still opening” message is used only
for errors classified as retryable startup failures. Re-throw permission,
stale-socket, protocol, and other non-retryable connection errors instead of
mapping them to the retry message, while preserving the existing retry behavior
and error details for eligible failures.
- Around line 28327-28332: Update the surface.resume.set request flow following
the launch_command and permission_mode assignments so failures from
client.sendV2 are no longer discarded via try?. Handle the error through the
existing controlled error path, or explicitly retry/omit the restore fields only
when the server reports they are unsupported, while preserving successful
hook-driven restoration.
In `@Sources/VerticalTabsSidebar`+WorkspaceGroups.swift:
- Around line 284-303: Update the affected method in
VerticalTabsSidebar+WorkspaceGroups to accept and use the existing
SidebarUnreadSnapshot. Replace the inline unread-count checks for anchor and
non-anchor workspaces with workspaceIsUnread(forWorkspaceId:),
canMarkWorkspaceRead(forWorkspaceIds:), and
canMarkWorkspaceUnread(forWorkspaceIds:) as appropriate, and remove the
now-unnecessary anchorIds variable.
🪄 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: 46bfc0c7-d1ed-4a94-8861-3f9c61255e09
📒 Files selected for processing (26)
CLI/cmux.swiftPackages/macOS/CmuxNotifications/Sources/CmuxNotifications/SidebarUnreadSnapshot.swiftPackages/macOS/CmuxNotifications/Tests/CmuxNotificationsTests/SidebarUnreadSnapshotTests.swiftSources/AppDelegate.swiftSources/CMUXInstalledExtensionSidebarHostView.swiftSources/ContentView.swiftSources/DockPanelView.swiftSources/DockUnreadPanelProjection.swiftSources/MinimalModeSidebarTitlebarControlsOverlay.swiftSources/Panels/CustomSidebarPaneDataContextCache.swiftSources/Panels/CustomSidebarPanelView.swiftSources/Panels/PanelContentView.swiftSources/Sidebar/AppKitList/SidebarWorkspaceTableController.swiftSources/Sidebar/AppKitList/SidebarWorkspaceTableRowConfiguration.swiftSources/Sidebar/AppKitList/SidebarWorkspaceTableView.swiftSources/SurfaceResumeCommandCanonicalizer+PortableAgentExecutable.swiftSources/TerminalNotificationStore.swiftSources/Update/UpdateTitlebarAccessory.swiftSources/VerticalTabsSidebar+WorkspaceGroups.swiftcmuxTests/DockRuntimeParityTests.swiftcmuxTests/FileDropOverlayViewTests.swiftcmuxTests/SidebarHiddenPresentationTests.swiftcmuxTests/SidebarLazyLayoutScaleTests.swiftcmuxTests/SidebarPointerInteractionScaleTests.swiftcmuxTests/TitlebarInteractiveControlTests.swiftcmuxTests/WorkspaceContentViewVisibilityTests.swift
💤 Files with no reviewable changes (1)
- cmuxTests/SidebarHiddenPresentationTests.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
10 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/test_pi_extension_install.py">
<violation number="1" location="tests/test_pi_extension_install.py:100">
P2: This new CLI subprocess invocation runs against the real user's home, so any session-start state or identity writes land in the actual ~/.local/state/cmux instead of an isolated temp dir. Since this PR is explicitly about device/identity writes, isolate HOME for the refresh_env spawn by pointing CFFIXED_USER_HOME (and HOME) at a per-test temp directory so the test is hermetic and never touches real user state.</violation>
</file>
<file name="CLI/CMUXCLI+PiExtension.swift">
<violation number="1" location="CLI/CMUXCLI+PiExtension.swift:39">
P2: Uninstall can be undone by an in-flight `session-start`: a refresh that passed the marker check writes the file back after `uninstallPiExtensionHooks` removes it. Coordinate refresh and uninstall (or revalidate a shared generation/lock before committing) so a successful uninstall remains durable.</violation>
</file>
<file name="CLI/CMUXCLI+PiExtensionSourcePart2.swift">
<violation number="1" location="CLI/CMUXCLI+PiExtensionSourcePart2.swift:404">
P1: A prompt arriving immediately after session start can be routed before `ensureResumeBinding` completes. This loses the prior lifecycle ordering and can send the prompt to the ambient/stale surface when `session-start` resolves a moved surface; queue lifecycle work per session so prompt-submit follows the binding operation.</violation>
<violation number="2" location="CLI/CMUXCLI+PiExtensionSourcePart2.swift:415">
P2: Making the prompt-submit hook non-blocking changes its global ordering relative to the first tool-feed event of the same turn. Previously `before_agent_start` awaited prompt-submit, so cmux was guaranteed to receive prompt-submit before the first PreToolUse/PostToolUse feed for that turn. Now prompt-submit runs on the serial control queue while feed events are dispatched on a separate feed queue with its own scheduling/compaction, so a feed event can reach cmux before prompt-submit. If any UI logic assumes prompt-submit leads the turn's feed, this is a silent ordering regression. Consider extending the lifecycle-order test to assert prompt-submit precedes the first feed event, or otherwise document that cross-queue ordering is not guaranteed.</violation>
<violation number="3" location="CLI/CMUXCLI+PiExtensionSourcePart2.swift:488">
P2: Shutting down one Pi session now waits for every session's in-flight lifecycle work, so a slow or continuously active second session can indefinitely delay this session's stop and cleanup. Track/drain tasks by `sessionId` instead of using the extension-wide set.</violation>
</file>
<file name="Sources/ContentView.swift">
<violation number="1" location="Sources/ContentView.swift:830">
P2: With the AppKit sidebar enabled, unread publishes still invalidate `VerticalTabsSidebar` and reconstruct its table-row projection. Keep the initial unread snapshot and the update subscription inside the AppKit controller (or an isolated leaf) so `appKitWorkspaceTableRows()` does not read `SidebarUnreadModel` during the sidebar body.</violation>
</file>
<file name="CLI/cmux.swift">
<violation number="1" location="CLI/cmux.swift:35278">
P3: The added session-start refresh performs synchronous disk I/O — a full file read on every call and a full atomic rewrite whenever stale — on the Pi session-start hook dispatch path. It runs in the cmux CLI subprocess so it doesn't block the app UI directly, but it does add a blocking disk operation to a lifecycle path this PR is meant to keep lightweight, and the rewrite only takes effect for the *next* session (the current one already loaded the old extension). Consider deferring the write off the synchronous dispatch path (e.g., a fire-and-forget refresh) and short-circuiting on a cheap stat/size check before reading the whole file, since the per-session cost otherwise isn't free on upgrade sessions.</violation>
</file>
<file name="tests/test_pi_extension_dispatch.py">
<violation number="1" location="tests/test_pi_extension_dispatch.py:197">
P3: The 75ms hard threshold gives only a 2x margin over the 150ms stub sleep, so under heavy CI load a legitimately non-blocking handler whose call gets preempted for >75ms (e.g. first-call/JIT or scheduling jitter on a shared runner) would false-fail as 'blocking'. Consider widening the margin relative to the stub delay (e.g. threshold well below half the sleep, or assert elapsed is a small fraction of the stub latency) to make the latency regression check robust to environment noise rather than measuring absolute wall time.</violation>
</file>
<file name="Sources/CMUXInstalledExtensionSidebarHostView.swift">
<violation number="1" location="Sources/CMUXInstalledExtensionSidebarHostView.swift:222">
P1: Accepted extension actions publish the cached pre-action snapshot, so extensions can remain out of sync until a later `snapshotUpdateToken` refresh. Keep this provider authoritative for action-triggered pushes, or refresh `snapshotCache` immediately after accepted actions.</violation>
</file>
<file name="cmuxTests/WorkspaceContentViewVisibilityTests.swift">
<violation number="1" location="cmuxTests/WorkspaceContentViewVisibilityTests.swift:358">
P3: The new test asserts that the affected sidebar row receives the unread badge and that ContentView/WorkspaceContent/VerticalTabsSidebar bodies don't rebuild, but it never verifies that the unaffected workspace rows are left unchanged. Since the test is named `testUnreadChangeUpdatesOnlyAffectedSidebarRow`, the "only" is unproven — a regression that also wrote the new count into neighboring rows would still pass. Consider installing `applyModelProbeForTesting` on a second, non-targeted workspace cell and asserting its unreadCount stays 0.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| return true | ||
| } | ||
| if def.name == "pi", action == "session-start" { | ||
| refreshManagedPiExtensionIfNeeded(def) |
There was a problem hiding this comment.
P3: The added session-start refresh performs synchronous disk I/O — a full file read on every call and a full atomic rewrite whenever stale — on the Pi session-start hook dispatch path. It runs in the cmux CLI subprocess so it doesn't block the app UI directly, but it does add a blocking disk operation to a lifecycle path this PR is meant to keep lightweight, and the rewrite only takes effect for the next session (the current one already loaded the old extension). Consider deferring the write off the synchronous dispatch path (e.g., a fire-and-forget refresh) and short-circuiting on a cheap stat/size check before reading the whole file, since the per-session cost otherwise isn't free on upgrade sessions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At CLI/cmux.swift, line 35278:
<comment>The added session-start refresh performs synchronous disk I/O — a full file read on every call and a full atomic rewrite whenever stale — on the Pi session-start hook dispatch path. It runs in the cmux CLI subprocess so it doesn't block the app UI directly, but it does add a blocking disk operation to a lifecycle path this PR is meant to keep lightweight, and the rewrite only takes effect for the *next* session (the current one already loaded the old extension). Consider deferring the write off the synchronous dispatch path (e.g., a fire-and-forget refresh) and short-circuiting on a cheap stat/size check before reading the whole file, since the per-session cost otherwise isn't free on upgrade sessions.</comment>
<file context>
@@ -35274,6 +35274,9 @@ export default CMUXSessionRestore;
return true
}
+ if def.name == "pi", action == "session-start" {
+ refreshManagedPiExtensionIfNeeded(def)
+ }
let actionArgs = Array(rest.dropFirst())
</file context>
Contributor
Author
There was a problem hiding this comment.
The refresh now skips a contended lock, and Pi's handler returns before the serialized CLI child completes. The remaining steady-state file verification is bounded to the generated managed extension; an atomic rewrite occurs only after an upgrade.
| await Promise.resolve(action()); | ||
| const elapsed = performance.now() - startedAt; | ||
| console.log(`${label}_ms=${elapsed}`); | ||
| if (elapsed >= 75) throw new Error(`${label} blocked Pi for ${elapsed}ms`); |
There was a problem hiding this comment.
P3: The 75ms hard threshold gives only a 2x margin over the 150ms stub sleep, so under heavy CI load a legitimately non-blocking handler whose call gets preempted for >75ms (e.g. first-call/JIT or scheduling jitter on a shared runner) would false-fail as 'blocking'. Consider widening the margin relative to the stub delay (e.g. threshold well below half the sleep, or assert elapsed is a small fraction of the stub latency) to make the latency regression check robust to environment noise rather than measuring absolute wall time.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_pi_extension_dispatch.py, line 197:
<comment>The 75ms hard threshold gives only a 2x margin over the 150ms stub sleep, so under heavy CI load a legitimately non-blocking handler whose call gets preempted for >75ms (e.g. first-call/JIT or scheduling jitter on a shared runner) would false-fail as 'blocking'. Consider widening the margin relative to the stub delay (e.g. threshold well below half the sleep, or assert elapsed is a small fraction of the stub latency) to make the latency regression check robust to environment noise rather than measuring absolute wall time.</comment>
<file context>
@@ -161,6 +161,121 @@ def check_responsiveness(bun: str, root: Path, extension_path: Path) -> int:
+ await Promise.resolve(action());
+ const elapsed = performance.now() - startedAt;
+ console.log(`${label}_ms=${elapsed}`);
+ if (elapsed >= 75) throw new Error(`${label} blocked Pi for ${elapsed}ms`);
+}
+await measure("session_start", () => handlers.get("session_start")({}, ctx));
</file context>
There was a problem hiding this comment.
9 issues found across 38 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Packages/macOS/CmuxNotifications/Sources/CmuxNotifications/SidebarUnreadSnapshot.swift">
<violation number="1" location="Packages/macOS/CmuxNotifications/Sources/CmuxNotifications/SidebarUnreadSnapshot.swift:208">
P2: Reentrant `apply` calls can make observers regress to an older snapshot: a callback publishing B runs before this loop resumes, so later observers receive B and then stale A. Queue/reject nested publications and drain snapshots in order so every observer’s final state matches `snapshot`.</violation>
</file>
<file name="Sources/VerticalTabsSidebar+WorkspaceGroups.swift">
<violation number="1" location="Sources/VerticalTabsSidebar+WorkspaceGroups.swift:35">
P2: Unread publications will again invalidate the SwiftUI sidebar builder, rebuilding all AppKit row configurations instead of only reconfiguring affected cells. Keep this initial group model unread-neutral; the table controller supplies the current snapshot during `flushApply`.</violation>
</file>
<file name="tests/test_pi_extension_dispatch.py">
<violation number="1" location="tests/test_pi_extension_dispatch.py:391">
P3: The new cross-session isolation check polls the cmux log with an unbounded `while (true)` and no deadline. If the expected slow-session marker never appears, run_extension's subprocess.run(timeout=20) raises an unhandled TimeoutExpired that crashes the whole suite rather than failing this check cleanly. Give the loop a bounded deadline (like the sibling lifecycle test) so a regression fails as a per-check FAIL instead of a traceback.</violation>
</file>
<file name="CLI/CMUXCLI+PiExtension.swift">
<violation number="1" location="CLI/CMUXCLI+PiExtension.swift:50">
P2: Pi session-start hook delivery blocks whenever another process holds `.cmux-session.lock`, despite refresh being best-effort. Use a non-blocking acquisition for the refresh path and skip this refresh on contention; later session starts can retry.</violation>
<violation number="2" location="CLI/CMUXCLI+PiExtension.swift:78">
P2: The refresh path bails when the on-disk extension is empty (`existing.isEmpty`), so a truncated/zero-byte managed extension is never repaired on session start even though `installPiExtensionHooks` treats an empty file as a writable fresh install. Since the whole purpose of `refreshManagedPiExtensionIfNeeded` is to heal stale cmux-managed extensions, an empty-but-present file (e.g. from an interrupted write) stays broken and the module won't load. Consider letting the empty case fall through to the replacement write, consistent with install, so an empty managed file is also healed.</violation>
</file>
<file name="Sources/CMUXInstalledExtensionSidebarHostView.swift">
<violation number="1" location="Sources/CMUXInstalledExtensionSidebarHostView.swift:177">
P3: Unread observation behavior is now duplicated across two private view types, so future delivery fixes can diverge. Share the existing observer instead of maintaining a second copy.</violation>
</file>
<file name="Sources/Update/UpdateTitlebarAccessory.swift">
<violation number="1" location="Sources/Update/UpdateTitlebarAccessory.swift:196">
P3: Editing an unrelated shortcut still recomputes shortcut/font geometry and relayouts every titlebar surface. Filter action-scoped notifications to the titlebar hint actions; retain recomputation for action-less bulk reloads.</violation>
</file>
<file name="tests/test_pi_extension_install.py">
<violation number="1" location="tests/test_pi_extension_install.py:163">
P2: The new race fixtures spawn `blocked_refresh`/`blocked_uninstall` with pipes and drain them via `communicate(..., timeout=20)` without handling `TimeoutExpired`. If the in-flight refresh deadlocks (these tests deliberately block the child on a lock), the 20s deadline only raises an uncaught exception — the child processes are never terminated or SIGKILLed and the pipes are left undrained, leaking a hung cmux CLI process and potentially hanging CI. Wrap each `communicate`/`subprocess.run` in try/except `subprocess.TimeoutExpired` and, on timeout, send terminate then SIGKILL before re-raising, or add a `finally` cleanup for the spawned Popen objects.</violation>
</file>
<file name="CLI/CMUXCLI+PiExtensionSourcePart2.swift">
<violation number="1" location="CLI/CMUXCLI+PiExtensionSourcePart2.swift:384">
P3: When a deferred lifecycle task rejects, the new catch swallows the exception and warns with only `error_available: error !== undefined` — the real error message is never logged. Since these tasks are now detached (handlers return immediately), the throw site is no longer visible to the caller, so this is the only diagnostic for a failed prompt-submit / resume-binding / completion / teardown task. Consider serializing the actual error (e.g., `message`/`name`) into the warning details so a failing task is actionable.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| const healthy = context("pi-healthy-session"); | ||
| handlers.get("before_agent_start")({ prompt: "block session A" }, slow); | ||
| const logPath = process.env.CMUX_TEST_PI_CROSS_LIFECYCLE_LOG; | ||
| while (true) { |
There was a problem hiding this comment.
P3: The new cross-session isolation check polls the cmux log with an unbounded while (true) and no deadline. If the expected slow-session marker never appears, run_extension's subprocess.run(timeout=20) raises an unhandled TimeoutExpired that crashes the whole suite rather than failing this check cleanly. Give the loop a bounded deadline (like the sibling lifecycle test) so a regression fails as a per-check FAIL instead of a traceback.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_pi_extension_dispatch.py, line 391:
<comment>The new cross-session isolation check polls the cmux log with an unbounded `while (true)` and no deadline. If the expected slow-session marker never appears, run_extension's subprocess.run(timeout=20) raises an unhandled TimeoutExpired that crashes the whole suite rather than failing this check cleanly. Give the loop a bounded deadline (like the sibling lifecycle test) so a regression fails as a per-check FAIL instead of a traceback.</comment>
<file context>
@@ -161,6 +161,289 @@ def check_responsiveness(bun: str, root: Path, extension_path: Path) -> int:
+const healthy = context("pi-healthy-session");
+handlers.get("before_agent_start")({ prompt: "block session A" }, slow);
+const logPath = process.env.CMUX_TEST_PI_CROSS_LIFECYCLE_LOG;
+while (true) {
+ try {
+ if ((await Bun.file(logPath).text()).includes("pi-slow-session")) break;
</file context>
| } | ||
| } | ||
|
|
||
| private struct CMUXSidebarUnreadSnapshotObserver: View { |
There was a problem hiding this comment.
P3: Unread observation behavior is now duplicated across two private view types, so future delivery fixes can diverge. Share the existing observer instead of maintaining a second copy.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/CMUXInstalledExtensionSidebarHostView.swift, line 177:
<comment>Unread observation behavior is now duplicated across two private view types, so future delivery fixes can diverge. Share the existing observer instead of maintaining a second copy.</comment>
<file context>
@@ -132,12 +133,69 @@ private struct CMUXSidebarExtensionLimitedChoiceStore {
+ }
+}
+
+private struct CMUXSidebarUnreadSnapshotObserver: View {
+ let source: SidebarUnreadModel
+ let action: @MainActor (SidebarUnreadSnapshot) -> Void
</file context>
| } | ||
| ) | ||
| for name in [ | ||
| KeyboardShortcutSettings.didChangeNotification, |
There was a problem hiding this comment.
P3: Editing an unrelated shortcut still recomputes shortcut/font geometry and relayouts every titlebar surface. Filter action-scoped notifications to the titlebar hint actions; retain recomputation for action-less bulk reloads.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Update/UpdateTitlebarAccessory.swift, line 196:
<comment>Editing an unrelated shortcut still recomputes shortcut/font geometry and relayouts every titlebar surface. Filter action-scoped notifications to the titlebar hint actions; retain recomputation for action-less bulk reloads.</comment>
<file context>
@@ -139,6 +141,102 @@ struct TitlebarControlsStyleConfig {
+ }
+ )
+ for name in [
+ KeyboardShortcutSettings.didChangeNotification,
+ GlobalFontMagnification.didChangeNotification,
+ ] {
</file context>
There was a problem hiding this comment.
8 issues found across 38 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/VerticalTabsSidebar+WorkspaceGroups.swift">
<violation number="1" location="Sources/VerticalTabsSidebar+WorkspaceGroups.swift:224">
P2: Unread updates for multiple groups now rescan the entire workspace list per affected group. Reuse the render context's membership index or maintain a group-id-to-members index so targeted unread repaint stays proportional to changed rows instead of becoming O(groups × workspaces).</violation>
</file>
<file name="Sources/Sidebar/AppKitList/Cells/SidebarGroupHeaderRowModel.swift">
<violation number="1" location="Sources/Sidebar/AppKitList/Cells/SidebarGroupHeaderRowModel.swift:22">
P3: The render model's immutable snapshot contract is weakened without a consumer needing mutation. Keeping these fields `let` preserves the documented value-snapshot semantics and prevents accidental in-place state changes from bypassing normal rebuild/reconfigure flow.</violation>
</file>
<file name="CLI/CMUXCLI+PiExtension.swift">
<violation number="1" location="CLI/CMUXCLI+PiExtension.swift:47">
P3: Install/uninstall reports “Failed to read <extension>” when opening or locking `.cmux-session.lock` fails, obscuring permission or lock-operation failures. Use a localized mutation/lock-specific error for these paths.</violation>
</file>
<file name="CLI/cmux.swift">
<violation number="1" location="CLI/cmux.swift:35278">
P2: This session-start call performs synchronous, blocking file I/O (Darwin.flock LOCK_EX, read, and a conditional atomic write) on the live Hook delivery path — the same path this PR is trying to keep off blocking work. In steady state it degrades to a small no-op read, but immediately after a cmux update (extension content changed) or when another cmux process holds the extension lock, the first session-start blocks until the write/lock completes, adding latency on Pi's event loop. Consider dispatching the refresh to a short-lived background task so hook delivery isn't gated on the file mutation, while keeping the revalidation-under-lock in place.</violation>
</file>
<file name="Packages/macOS/CmuxNotifications/Sources/CmuxNotifications/SidebarUnreadSnapshot.swift">
<violation number="1" location="Packages/macOS/CmuxNotifications/Sources/CmuxNotifications/SidebarUnreadSnapshot.swift:108">
P2: A manually marked-unread workspace with no notification count cannot enable Mark Read. Include `hasManualUnread` so the action reflects the snapshot's explicit unread state.</violation>
<violation number="2" location="Packages/macOS/CmuxNotifications/Sources/CmuxNotifications/SidebarUnreadSnapshot.swift:208">
P2: Reentrant observer updates can deliver snapshots out of order: later observers receive a newer snapshot and then this stale one. Serialize notification draining or defer nested publications until the current observer pass completes.</violation>
</file>
<file name="Sources/CMUXInstalledExtensionSidebarHostView.swift">
<violation number="1" location="Sources/CMUXInstalledExtensionSidebarHostView.swift:177">
P3: Unread-observation behavior now has two identical private implementations, so future changes to delivery or invalidation semantics can silently diverge. A shared observation-leaf view would keep both the overlay and extension-sidebar paths on one implementation.</violation>
</file>
<file name="tests/test_pi_extension_install.py">
<violation number="1" location="tests/test_pi_extension_install.py:163">
P3: The new refresh-race fixtures call communicate(timeout=20) on the flock-blocked CLI child without any kill-on-timeout. If session-start ever hangs (e.g. wedges connecting to the intentionally missing socket) instead of exiting non-zero, TimeoutExpired propagates as an uncaught traceback and the child stays blocked on the lock. Recommend wrapping these calls so a timeout terminates (then SIGKILLs) the child before re-raising, matching the repo's established child-lifecycle pattern. The likely happy path (refresh exits quickly) is unaffected; this is defensive robustness for the concurrency test.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
Comment on lines
+22
to
+27
| var anchorUnreadCount: Int | ||
| var canMarkRead: Bool | ||
| var canMarkUnread: Bool | ||
| var hasLatestNotifications: Bool | ||
| var canMarkAllRead: Bool | ||
| var canMarkAllUnread: Bool |
There was a problem hiding this comment.
P3: The render model's immutable snapshot contract is weakened without a consumer needing mutation. Keeping these fields let preserves the documented value-snapshot semantics and prevents accidental in-place state changes from bypassing normal rebuild/reconfigure flow.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Sidebar/AppKitList/Cells/SidebarGroupHeaderRowModel.swift, line 22:
<comment>The render model's immutable snapshot contract is weakened without a consumer needing mutation. Keeping these fields `let` preserves the documented value-snapshot semantics and prevents accidental in-place state changes from bypassing normal rebuild/reconfigure flow.</comment>
<file context>
@@ -19,12 +19,12 @@ struct SidebarGroupHeaderRowModel: Equatable, Hashable {
- let hasLatestNotifications: Bool
- let canMarkAllRead: Bool
- let canMarkAllUnread: Bool
+ var anchorUnreadCount: Int
+ var canMarkRead: Bool
+ var canMarkUnread: Bool
</file context>
Suggested change
| var anchorUnreadCount: Int | |
| var canMarkRead: Bool | |
| var canMarkUnread: Bool | |
| var hasLatestNotifications: Bool | |
| var canMarkAllRead: Bool | |
| var canMarkAllUnread: Bool | |
| let anchorUnreadCount: Int | |
| let canMarkRead: Bool | |
| let canMarkUnread: Bool | |
| let hasLatestNotifications: Bool | |
| let canMarkAllRead: Bool | |
| let canMarkAllUnread: Bool |
| mode_t(S_IRUSR | S_IWUSR) | ||
| ) | ||
| guard descriptor >= 0 else { | ||
| throw piExtensionReadError(at: extensionURL) |
There was a problem hiding this comment.
P3: Install/uninstall reports “Failed to read ” when opening or locking .cmux-session.lock fails, obscuring permission or lock-operation failures. Use a localized mutation/lock-specific error for these paths.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At CLI/CMUXCLI+PiExtension.swift, line 47:
<comment>Install/uninstall reports “Failed to read <extension>” when opening or locking `.cmux-session.lock` fails, obscuring permission or lock-operation failures. Use a localized mutation/lock-specific error for these paths.</comment>
<file context>
@@ -26,6 +27,73 @@ extension CMUXCLI {
+ mode_t(S_IRUSR | S_IWUSR)
+ )
+ guard descriptor >= 0 else {
+ throw piExtensionReadError(at: extensionURL)
+ }
+ defer { Darwin.close(descriptor) }
</file context>
| } | ||
| } | ||
|
|
||
| private struct CMUXSidebarUnreadSnapshotObserver: View { |
There was a problem hiding this comment.
P3: Unread-observation behavior now has two identical private implementations, so future changes to delivery or invalidation semantics can silently diverge. A shared observation-leaf view would keep both the overlay and extension-sidebar paths on one implementation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/CMUXInstalledExtensionSidebarHostView.swift, line 177:
<comment>Unread-observation behavior now has two identical private implementations, so future changes to delivery or invalidation semantics can silently diverge. A shared observation-leaf view would keep both the overlay and extension-sidebar paths on one implementation.</comment>
<file context>
@@ -132,12 +133,69 @@ private struct CMUXSidebarExtensionLimitedChoiceStore {
+ }
+}
+
+private struct CMUXSidebarUnreadSnapshotObserver: View {
+ let source: SidebarUnreadModel
+ let action: @MainActor (SidebarUnreadSnapshot) -> Void
</file context>
| ) | ||
| extension_path.write_text(replacement, encoding="utf-8") | ||
| fcntl.flock(lock, fcntl.LOCK_UN) | ||
| blocked_refresh.communicate(input=refresh_payload, timeout=20) |
There was a problem hiding this comment.
P3: The new refresh-race fixtures call communicate(timeout=20) on the flock-blocked CLI child without any kill-on-timeout. If session-start ever hangs (e.g. wedges connecting to the intentionally missing socket) instead of exiting non-zero, TimeoutExpired propagates as an uncaught traceback and the child stays blocked on the lock. Recommend wrapping these calls so a timeout terminates (then SIGKILLs) the child before re-raising, matching the repo's established child-lifecycle pattern. The likely happy path (refresh exits quickly) is unaffected; this is defensive robustness for the concurrency test.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_pi_extension_install.py, line 163:
<comment>The new refresh-race fixtures call communicate(timeout=20) on the flock-blocked CLI child without any kill-on-timeout. If session-start ever hangs (e.g. wedges connecting to the intentionally missing socket) instead of exiting non-zero, TimeoutExpired propagates as an uncaught traceback and the child stays blocked on the lock. Recommend wrapping these calls so a timeout terminates (then SIGKILLs) the child before re-raising, matching the repo's established child-lifecycle pattern. The likely happy path (refresh exits quickly) is unaffected; this is defensive robustness for the concurrency test.</comment>
<file context>
@@ -86,6 +92,117 @@ def main() -> int:
+ )
+ extension_path.write_text(replacement, encoding="utf-8")
+ fcntl.flock(lock, fcntl.LOCK_UN)
+ blocked_refresh.communicate(input=refresh_payload, timeout=20)
+ if extension_path.read_text(encoding="utf-8") != replacement:
+ print("FAIL: in-flight Pi refresh overwrote a replacement extension")
</file context>
Suggested change
| blocked_refresh.communicate(input=refresh_payload, timeout=20) | |
| try: | |
| blocked_refresh.communicate(input=refresh_payload, timeout=20) | |
| except subprocess.TimeoutExpired: | |
| blocked_refresh.kill() | |
| blocked_refresh.communicate() | |
| raise |
# Conflicts: # Sources/Update/UpdateTitlebarAccessory.swift
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. |
There was a problem hiding this comment.
1 issue found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/CMUXInstalledExtensionSidebarHostView.swift">
<violation number="1" location="Sources/CMUXInstalledExtensionSidebarHostView.swift:314">
P3: Unread delivery still scans and allocates two full workspace-ID arrays on every notification, so large sidebars retain a main-thread cost on the path this change is meant to make cheap. A membership revision/token maintained with structural sidebar updates would let this callback avoid both maps.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| SidebarUnreadSnapshotObserver(source: unreadSource) { unreadSnapshot in | ||
| // Rebuild rich metadata only when workspace membership changed. | ||
| // The common unread-only path stays a cheap cache patch. | ||
| if !snapshotCache.containsWorkspaces(workspaceIDsProvider()) { |
There was a problem hiding this comment.
P3: Unread delivery still scans and allocates two full workspace-ID arrays on every notification, so large sidebars retain a main-thread cost on the path this change is meant to make cheap. A membership revision/token maintained with structural sidebar updates would let this callback avoid both maps.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/CMUXInstalledExtensionSidebarHostView.swift, line 314:
<comment>Unread delivery still scans and allocates two full workspace-ID arrays on every notification, so large sidebars retain a main-thread cost on the path this change is meant to make cheap. A membership revision/token maintained with structural sidebar updates would let this callback avoid both maps.</comment>
<file context>
@@ -253,14 +300,31 @@ struct CMUXInstalledExtensionSidebarHostView: View {
+ SidebarUnreadSnapshotObserver(source: unreadSource) { unreadSnapshot in
+ // Rebuild rich metadata only when workspace membership changed.
+ // The common unread-only path stays a cheap cache patch.
+ if !snapshotCache.containsWorkspaces(workspaceIDsProvider()) {
+ let snapshot = snapshotCache.replace(with: snapshotProvider())
+ xpcHost.sendSnapshotDidChange(snapshot)
</file context>
There was a problem hiding this comment.
2 issues found across 37 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/test_pi_extension_install.py">
<violation number="1" location="tests/test_pi_extension_install.py:45">
P2: In the timeout path the final process.communicate() has no timeout and will block until EOF on both pipes. If the refresh/uninstall child spawns a grandchild that inherits those pipe write-ends, EOF never arrives and the test hangs instead of failing fast. This helper is the fallback for the new lock-race tests that intentionally exercise timeouts, so a bounded, nonblocking drain (os.read on the raw fds after exit, or a finite communicate timeout + process-group kill) would keep those races from wedging CI.</violation>
</file>
<file name="Sources/CMUXInstalledExtensionSidebarHostView.swift">
<violation number="1" location="Sources/CMUXInstalledExtensionSidebarHostView.swift:147">
P2: The snapshot cache's sequence can run ahead of the authoritative provider. applyUnread(_:) bumps the cached sequence, and replace(with:) re-crafts `max(next.sequence, current.sequence &+ 1)` onto the provider snapshot. Since the unread-patched cache and the real provider snapshot have identical content but different sequences, every later extension poll (`sendSnapshotDidChange` -> snapshotProvider -> replace) replaces the cache and inflates the sequence by one with no real change. The extension therefore observes a continuously increasing sequence on every poll despite identical content, which can re-trigger extension-side work and the broad-sidebar churn this PR is meant to avoid. Consider not persisting a synthetic sequence bump when the incoming provider snapshot matches the current cached content (diff only in sequence), so ordering reflects real changes only.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
There was a problem hiding this comment.
9 issues found across 38 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/Sidebar/AppKitList/Cells/SidebarGroupHeaderRowModel.swift">
<violation number="1" location="Sources/Sidebar/AppKitList/Cells/SidebarGroupHeaderRowModel.swift:22">
P3: The model is no longer immutable: `unreadRebuild` mutates these fields in place. Update the type documentation to describe its value-snapshot semantics without promising immutability, so callers do not rely on a stale contract.</violation>
</file>
<file name="cmuxTests/FileDropOverlayViewTests.swift">
<violation number="1" location="cmuxTests/FileDropOverlayViewTests.swift:7">
P3: This newly added `import CmuxNotifications` is now unused: the only reference needing that module's types (the removed `.environmentObject(TerminalNotificationStore.shared.sidebarUnread)`) is gone, and nothing else in the file uses CmuxNotifications. Drop the import to avoid a dead dependency in the test.</violation>
</file>
<file name="cmuxTests/SidebarWorkspaceSnapshotRefreshPolicyTests.swift">
<violation number="1" location="cmuxTests/SidebarWorkspaceSnapshotRefreshPolicyTests.swift:51">
P3: This regression test only covers the dedup/no-inflation branch: it asserts that identical provider content keeps the cached sequence, but never asserts the positive path where genuinely changed content *does* bump the sequence and propagate. Because `replace(with:)` returns `current` in the dedup branch, these `#expect(...sequence == patched.sequence)` assertions would still pass even if `replace(with:)` regressed to always return the cached snapshot (swallowing all real updates). Adding a positive control — a provider whose workspace summary differs from the patched snapshot, asserting `sequence` inflates and the returned snapshot carries the new content — would fully lock in the intended "content changed → reconfigures" behavior this cache is protecting.</violation>
</file>
<file name="tests/test_pi_extension_install.py">
<violation number="1" location="tests/test_pi_extension_install.py:84">
P3: The switch from a '---' record delimiter to line-per-record parsing silently drops any cmux stdin payload whose JSON contains an embedded newline (e.g. a notification/stop payload carrying a multi-line last_assistant_message): json.loads fails on the partial line and the payload is thrown away, so the per-session completion-order and turn_id assertions can miss it. The payloads here are single-line today, but this is a regression in robustness vs the prior chunk-based split.</violation>
<violation number="2" location="tests/test_pi_extension_install.py:215">
P2: The race tests use a 1-second wall-clock bound to distinguish "correct non-blocking lock handling" from "blocks on the lock". A correctly-behaving refresher that is merely slow to spawn/start on a loaded CI host is SIGTERM'd by communicate_or_terminate and converted into a false "blocked on its advisory lock"/"blocked behind uninstall" FAIL. Consider making the lock-behavior assertion deterministic (e.g. assert the refresh subprocess exits promptly with a non-zero status while the parent still holds the flock) rather than relying on a tight timeout that conflates slowness with blocking.</violation>
</file>
<file name="cmuxTests/WorkspaceContentViewVisibilityTests.swift">
<violation number="1" location="cmuxTests/WorkspaceContentViewVisibilityTests.swift:419">
P3: The "atomic" assertion here can't actually detect multi-step publication. withObservationTracking fires onChange only once per registration, so an apply() that wrote five @Observable properties one-by-one would still yield publicationCount == 1, making the test pass while the model publishes a torn intermediate state. The meaningful coverage is really the second phase (equivalent snapshot stays silent); the first phase gives false confidence about atomicity. Consider re-arming registration inside onChange (or tracking each field separately) and asserting intermediate states are never observed if the goal is to prove a single atomic snapshot.</violation>
</file>
<file name="tests/test_pi_extension_dispatch.py">
<violation number="1" location="tests/test_pi_extension_dispatch.py:439">
P3: The `healthy_stop > slow_prompt_end` clause in this assertion can never be true: the harness writes the release file only after session B's shutdown subprocess finishes, and the slow session A prompt subprocess writes its `end` line only after that release file exists. So session B's stop `end` always precedes session A's prompt `end` by construction, and the clause is dead. The regression is actually caught by the earlier `healthy_stop is None`/non-zero-returncode paths; consider dropping the clause (or restructuring so it can actually fail) so the FAIL message's intent matches what the check can detect.</violation>
</file>
<file name="Sources/CMUXInstalledExtensionSidebarHostView.swift">
<violation number="1" location="Sources/CMUXInstalledExtensionSidebarHostView.swift:176">
P2: Unread delivery now allocates and scans the entire workspace list twice per update, so a busy notification stream still puts O(workspaces) work on the main actor. Retaining cached IDs and prior unread workspace IDs would let this path validate/update only affected rows.</violation>
</file>
<file name="CLI/CMUXCLI+PiExtension.swift">
<violation number="1" location="CLI/CMUXCLI+PiExtension.swift:45">
P2: An existing `.cmux-session.lock` symlink is followed, so a shared or untrusted `PI_CODING_AGENT_DIR` can redirect this synchronization point to another inode. Refresh can then silently skip, or install/uninstall can wait on an unrelated lock; reject symlink lock paths.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| import Testing | ||
| import WebKit | ||
| import CmuxUpdater | ||
| import CmuxNotifications |
There was a problem hiding this comment.
P3: This newly added import CmuxNotifications is now unused: the only reference needing that module's types (the removed .environmentObject(TerminalNotificationStore.shared.sidebarUnread)) is gone, and nothing else in the file uses CmuxNotifications. Drop the import to avoid a dead dependency in the test.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxTests/FileDropOverlayViewTests.swift, line 7:
<comment>This newly added `import CmuxNotifications` is now unused: the only reference needing that module's types (the removed `.environmentObject(TerminalNotificationStore.shared.sidebarUnread)`) is gone, and nothing else in the file uses CmuxNotifications. Drop the import to avoid a dead dependency in the test.</comment>
<file context>
@@ -4,6 +4,7 @@ import SwiftUI
import Testing
import WebKit
import CmuxUpdater
+import CmuxNotifications
#if canImport(cmux_DEV)
</file context>
| def payloads_from_log(text: str) -> list[dict[str, object]]: | ||
| payloads: list[dict[str, object]] = [] | ||
| for raw in text.split("\n---\n"): | ||
| for raw in text.splitlines(): |
There was a problem hiding this comment.
P3: The switch from a '---' record delimiter to line-per-record parsing silently drops any cmux stdin payload whose JSON contains an embedded newline (e.g. a notification/stop payload carrying a multi-line last_assistant_message): json.loads fails on the partial line and the payload is thrown away, so the per-session completion-order and turn_id assertions can miss it. The payloads here are single-line today, but this is a regression in robustness vs the prior chunk-based split.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_pi_extension_install.py, line 84:
<comment>The switch from a '---' record delimiter to line-per-record parsing silently drops any cmux stdin payload whose JSON contains an embedded newline (e.g. a notification/stop payload carrying a multi-line last_assistant_message): json.loads fails on the partial line and the payload is thrown away, so the per-session completion-order and turn_id assertions can miss it. The payloads here are single-line today, but this is a regression in robustness vs the prior chunk-based split.</comment>
<file context>
@@ -41,9 +81,9 @@ def wait_for_text(
def payloads_from_log(text: str) -> list[dict[str, object]]:
payloads: list[dict[str, object]] = []
- for raw in text.split("\n---\n"):
+ for raw in text.splitlines():
raw = raw.strip()
- if not raw:
</file context>
| focusedReadIndicatorByWorkspaceId: focusedIndicators, | ||
| manualUnreadWorkspaceIds: manualUnreadWorkspaceIds | ||
| ) | ||
| #expect(publicationCount == 1) |
There was a problem hiding this comment.
P3: The "atomic" assertion here can't actually detect multi-step publication. withObservationTracking fires onChange only once per registration, so an apply() that wrote five @observable properties one-by-one would still yield publicationCount == 1, making the test pass while the model publishes a torn intermediate state. The meaningful coverage is really the second phase (equivalent snapshot stays silent); the first phase gives false confidence about atomicity. Consider re-arming registration inside onChange (or tracking each field separately) and asserting intermediate states are never observed if the goal is to prove a single atomic snapshot.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxTests/WorkspaceContentViewVisibilityTests.swift, line 419:
<comment>The "atomic" assertion here can't actually detect multi-step publication. withObservationTracking fires onChange only once per registration, so an apply() that wrote five @Observable properties one-by-one would still yield publicationCount == 1, making the test pass while the model publishes a torn intermediate state. The meaningful coverage is really the second phase (equivalent snapshot stays silent); the first phase gives false confidence about atomicity. Consider re-arming registration inside onChange (or tracking each field separately) and asserting intermediate states are never observed if the goal is to prove a single atomic snapshot.</comment>
<file context>
@@ -257,6 +258,181 @@ final class WorkspaceContentViewVisibilityTests {
+ focusedReadIndicatorByWorkspaceId: focusedIndicators,
+ manualUnreadWorkspaceIds: manualUnreadWorkspaceIds
+ )
+ #expect(publicationCount == 1)
+
+ withObservationTracking {
</file context>
| any(line.startswith("blocked ") for line in calls) | ||
| or healthy_stop is None | ||
| or slow_prompt_end is None | ||
| or healthy_stop > slow_prompt_end |
There was a problem hiding this comment.
P3: The healthy_stop > slow_prompt_end clause in this assertion can never be true: the harness writes the release file only after session B's shutdown subprocess finishes, and the slow session A prompt subprocess writes its end line only after that release file exists. So session B's stop end always precedes session A's prompt end by construction, and the clause is dead. The regression is actually caught by the earlier healthy_stop is None/non-zero-returncode paths; consider dropping the clause (or restructuring so it can actually fail) so the FAIL message's intent matches what the check can detect.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_pi_extension_dispatch.py, line 439:
<comment>The `healthy_stop > slow_prompt_end` clause in this assertion can never be true: the harness writes the release file only after session B's shutdown subprocess finishes, and the slow session A prompt subprocess writes its `end` line only after that release file exists. So session B's stop `end` always precedes session A's prompt `end` by construction, and the clause is dead. The regression is actually caught by the earlier `healthy_stop is None`/non-zero-returncode paths; consider dropping the clause (or restructuring so it can actually fail) so the FAIL message's intent matches what the check can detect.</comment>
<file context>
@@ -161,6 +161,288 @@ def check_responsiveness(bun: str, root: Path, extension_path: Path) -> int:
+ any(line.startswith("blocked ") for line in calls)
+ or healthy_stop is None
+ or slow_prompt_end is None
+ or healthy_stop > slow_prompt_end
+ ):
+ print(f"FAIL: slow Pi session delayed another session's shutdown: {calls!r}")
</file context>
This was referenced Aug 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pi hooks now refresh the managed extension at session start and dispatch lifecycle work asynchronously, preserving per-session ordering without blocking Pi's event loop.
Unread delivery now publishes one atomic snapshot. The native sidebar table owns that snapshot and reconfigures only rows whose workspace summaries changed, while terminal overlays update directly. Titlebar geometry is cached until a real titlebar input changes. Presence heartbeats also avoid rewriting an unchanged device ID, which previously emitted a broad UserDefaults notification and rebuilt the full content tree on the heartbeat cadence.
Five paired trials on tagged head
pilag:pi -nepiFull
piincludes every configured extension. A separate five-trialpi -ne -e cmux-session.tsrun isolated the cmux extension at 0.875 ms median prompt acceptance, 1.162 ms maximum, and 24.236 ms median abort. The larger full-Pi shutdown delta comes from other configured extensions or shared Pi work.A 30-second Time Profiler capture processed 12 unread notifications and crossed the heartbeat interval. It found no notification-time or heartbeat-time hang. The sole 252.60 ms microhang began at 1.508 seconds, before the first injected notification at 2 seconds.
Verification:
tests/test_pi_extension_install.pytests/test_pi_extension_dispatch.pySummary by CodeRabbit
New Features
Bug Fixes