Gate agent notifications on background work + per-category settings - #7129
Conversation
cmux fired a "Completed" notification on every Claude turn end, even when the turn was intermediate (a background build, a Monitor, or a scheduled wakeup was still running and would re-invoke the agent). Verified against claude 2.1.197: the Stop hook payload carries background_tasks and session_crons; the Notification hook carries notification_type (permission_prompt vs idle_prompt) but not background_tasks, and idle_prompt fires ~60s after an intermediate turn even while a task runs. Mechanism: the CLI now classifies each agent notification and forwards the signal to the app as an optional c=<category>;p=<0|1> meta segment appended to the notify_target_async payload. The app gates delivery by user config (NotificationsCatalogSection), so the runtime signal (CLI-side) and the policy (app-side, where settings live) stay cleanly separated. - CLI: hasActiveClaudeBackgroundWork(_:) reads background_tasks[].status == "running" or non-empty session_crons (nil/absent => not pending, so claude < 2.1.145 behaves as before). Stop caches hadPendingBackgroundWorkAtStop on the session record and tags c=turn-complete;p=<pending>. Notification tags needs-permission / idle-reminder; idle-reminder reads the cache because its payload lacks background_tasks. - App: parseNotificationPayload gains an optional 4th meta segment, treated as meta only when it begins with c= (else folded back into the body, so legacy callers whose body contains | parse byte-identically). AgentNotificationGate decides delivery; gated in notifyTargetQueued. - Settings: notifications.agentPermissionPrompt (default on), agentTurnComplete (whenIdle default | always | never), agentIdleReminder (default on). Settings UI rows, cmux.json bridge + schema, en/ja/ko/uk localization. - Tests: ClaudeBackgroundWorkNotifyTests (CLI meta + cache) and AgentNotificationGateTests (decision table + meta parser). Principled fix: one shared background-work predicate, one CLI->app signal, policy centralized app-side. The same predicate is what the hibernation effort needs to avoid hibernating panes with live background work. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
📝 WalkthroughWalkthroughAdds cached background-work state to Claude stops, tags notification payloads with category and pending metadata, and uses new agent notification settings to gate delivery. The change also adds matching settings UI, schema/localization entries, and CLI and unit tests. ChangesAgent Notification Gating
Estimated code review effort: 4 (Complex) | ~60 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors, 2 warnings)
✅ Passed checks (20 passed)
✨ Finishing Touches📝 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae4203a24a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| #expect(handled.wait(timeout: .now() + 5) == .success) | ||
| harness.assertSuccessfulHook(result) | ||
| let snapshot = context.state.snapshot() | ||
| context.cleanup() |
There was a problem hiding this comment.
The tests that call runStopHook read cachedPending(storeURL, ...) after the helper returns, but ClaudeHookContext.cleanup() removes root, and storeURL is inside that root. As a result those cache assertions always see nil even when the CLI wrote hadPendingBackgroundWorkAtStop, so the newly added tests fail; defer cleanup until after the caller reads the store or read the cached value before deleting the fixture directory.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in be4cf8c: the helper now reads the cached value from the store before cleanup() deletes the fixture dir, and returns it to the caller. Verified on the AWS M4 Pro runner (all suite tests pass).
— Claude Code
Greptile SummaryGates "Agent Finished" and "Agent Waiting for Input" notifications on whether background tasks / scheduled crons are still running when a Claude turn ends, and adds per-category notification settings (
Confidence Score: 4/5Safe to merge with one small correctness fix noted; legacy behavior (untagged payloads always deliver) is fully preserved for non-cmux callers and older claude clients. The fallback classification for pre-2.1.145 claude clients switches on a localized subtitle string to derive the notification category, but the xcstrings catalog already provides Japanese translations for those exact keys. In a Japanese-locale app the switch always falls through to .other, meaning agentTurnComplete=never and agentIdleReminder=false have no effect on old-client notifications. summary.notifyCategory is directly available and makes the fallback both simpler and reliable. All other paths are correct and well-tested. The fallback subtitle-switch in the Claude notification hook (~line 23570 of CLI/cmux.swift) is the single place that needs attention. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant C as Claude CLI
participant CH as cmux CLI Hook
participant SS as Session Store
participant APP as App TerminalController
participant GATE as DeliveryGate
participant NS as Notification System
C->>CH: Stop hook
CH->>CH: hasActiveClaudeBackgroundWork
CH->>SS: cache hadPendingBackgroundWorkAtStop
CH->>APP: notify_target_async with meta segment
Note over C,CH: ~60s later
C->>CH: Notification idle_prompt
CH->>SS: read hadPendingBackgroundWorkAtStop
CH->>APP: "notify_target_async with c=idle-reminder p=cached"
APP->>APP: parseNotificationPayload extracts meta
APP->>GATE: agentNotificationShouldDeliver
alt delivers
GATE-->>APP: true
APP->>NS: enqueue notification
else gated by settings
GATE-->>APP: false
APP-->>C: OK dropped
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant C as Claude CLI
participant CH as cmux CLI Hook
participant SS as Session Store
participant APP as App TerminalController
participant GATE as DeliveryGate
participant NS as Notification System
C->>CH: Stop hook
CH->>CH: hasActiveClaudeBackgroundWork
CH->>SS: cache hadPendingBackgroundWorkAtStop
CH->>APP: notify_target_async with meta segment
Note over C,CH: ~60s later
C->>CH: Notification idle_prompt
CH->>SS: read hadPendingBackgroundWorkAtStop
CH->>APP: "notify_target_async with c=idle-reminder p=cached"
APP->>APP: parseNotificationPayload extracts meta
APP->>GATE: agentNotificationShouldDeliver
alt delivers
GATE-->>APP: true
APP->>NS: enqueue notification
else gated by settings
GATE-->>APP: false
APP-->>C: OK dropped
end
Reviews (17): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| enum AgentNotificationGate { | ||
| static func shouldDeliver( | ||
| category: AgentNotifyCategory, | ||
| pending: Bool, | ||
| permissionEnabled: Bool, | ||
| turnMode: AgentTurnCompleteMode, | ||
| idleEnabled: Bool | ||
| ) -> Bool { | ||
| switch category { | ||
| case .needsPermission: | ||
| return permissionEnabled | ||
| case .turnComplete: | ||
| switch turnMode { | ||
| case .always: return true | ||
| case .never: return false | ||
| case .whenIdle: return !pending | ||
| } | ||
| case .idleReminder: | ||
| return idleEnabled && !pending | ||
| case .other: | ||
| // Legacy/uncategorized (codex, grok, antigravity, pre-meta clients): | ||
| // deliver exactly as before. | ||
| return true | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
AgentNotificationGate is a caseless enum whose entire API is a single static func shouldDeliver(...). The cmux-no-ambient-global-state rule explicitly flags "a caseless enum used purely as a static func/static let namespace." The logic has no state, so the idiomatic fix is to promote shouldDeliver to a free nonisolated function (or a method on TerminalController) and drop the namespace type entirely — the test target can still call it via @testable import.
Rule Used: Flag new ambient global state in production Swift:... (source)
- AgentNotificationGateTests imported only `cmux`; the app-target types live in the `cmux_DEV` test-host module, so the bundle failed to compile and every unit-test shard failed. Import both, matching every other test. - Move AgentNotifyCategory / AgentTurnCompleteMode / AgentNotificationMeta / AgentNotificationGate out of the 14k-line TerminalController.swift into Sources/AgentNotificationGate.swift (pure, better isolated, shrinks the file's growth). Wired into the app target. - Refresh .github/swift-file-length-budget.tsv for the three files that grew (CLI/cmux.swift, TerminalController.swift, AppSection.swift). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![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
🤖 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/ClaudeBackgroundWorkNotifyTests.swift`:
- Around line 16-48: runStopHook currently returns storeURL after
context.cleanup() deletes context.root, so callers end up with a stale path and
the cachedPending assertions miss the file. Update runStopHook in
ClaudeBackgroundWorkNotifyTests to read the cached contents before cleanup or
return the cached value instead of the URL, using the existing helpers around
context.state.snapshot() and context.cleanup() to preserve a valid reference for
the later assertions.
In `@Sources/TerminalController.swift`:
- Line 11604: The metadata gate is missing from the metadata-aware notification
paths in `deliverNotificationSynchronously` callers that parse
`parseNotificationPayload(args)` but ignore `meta`. Update the shared delivery
flow used by the `notify_current`, `notify_surface`, and `notify_target`
handlers so `meta` is either validated against the new user setting or
explicitly rejected before calling `deliverNotificationSynchronously`. Prefer
centralizing this check in the common notification delivery logic rather than
duplicating it in each caller.
- Around line 62-76: The metadata parser in `AgentNotify.init?(meta:)` is
currently failing open because `parsedPending` defaults to false, so tagged
metadata without a valid pending flag can still create a notification. Update
`init?(meta:)` to track whether the `p` field was actually present and only
accept it when its value is exactly `0` or `1`; if `p` is missing or malformed,
return nil instead of constructing the notification. Keep the existing category
parsing, but make delivery depend on a valid pending signal so
`TerminalController` fails closed for tagged metadata.
🪄 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: 392d2b63-49e1-43f0-8cdc-b351ed0e6fea
📒 Files selected for processing (12)
CLI/cmux.swiftPackages/macOS/CmuxSettings/Sources/CmuxSettings/Keys/NotificationsCatalogSection.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/AppSection.swiftResources/Localizable.xcstringsSources/CmuxSettingsJSONPathSupport.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxprojcmuxTests/AgentNotificationGateTests.swiftcmuxTests/ClaudeBackgroundWorkNotifyTests.swiftweb/data/cmux.schema.jsonweb/messages/en.jsonweb/messages/ja.json
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 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="Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/AppSection.swift">
<violation number="1" location="Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/AppSection.swift:570">
P2: Add search/anchor entries for these new notification rows. As written, their configurationReview paths do not resolve through SettingsSearchIndex, so the new settings are not reachable/highlightable from settings search and the row-anchor contract drifts.</violation>
</file>
<file name="cmuxTests/ClaudeBackgroundWorkNotifyTests.swift">
<violation number="1" location="cmuxTests/ClaudeBackgroundWorkNotifyTests.swift:10">
P3: Use XCTest instead of Swift Testing for tests that spawn the CLI process/socket harness per team guidance. Swift Testing is reserved for pure decision/unit tests.</violation>
<violation number="2" location="cmuxTests/ClaudeBackgroundWorkNotifyTests.swift:194">
P3: Discard the stop result in `idlePromptAfterIdleStopTagsNotPending`. A silent stop-hook failure (non-zero exit) would go undetected, and the notification assertion could pass or fail for the wrong reason. Assert it like the sibling test does.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| // Agent: Needs Permission | ||
| SettingsCardRow( | ||
| configurationReview: .json("notifications.agentPermissionPrompt"), |
There was a problem hiding this comment.
P2: Add search/anchor entries for these new notification rows. As written, their configurationReview paths do not resolve through SettingsSearchIndex, so the new settings are not reachable/highlightable from settings search and the row-anchor contract drifts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/AppSection.swift, line 570:
<comment>Add search/anchor entries for these new notification rows. As written, their configurationReview paths do not resolve through SettingsSearchIndex, so the new settings are not reachable/highlightable from settings search and the row-anchor contract drifts.</comment>
<file context>
@@ -557,6 +563,46 @@ public struct AppSection: View {
+
+ // Agent: Needs Permission
+ SettingsCardRow(
+ configurationReview: .json("notifications.agentPermissionPrompt"),
+ String(localized: "settings.notifications.agentPermissionPrompt.title", defaultValue: "Agent Needs Permission"),
+ subtitle: String(localized: "settings.notifications.agentPermissionPrompt.subtitle", defaultValue: "Notify when an agent is blocked waiting for your permission to run a tool.")
</file context>
There was a problem hiding this comment.
Fixed in ca6c3cd: added CuratedSettingEntry rows for the three new notification settings (agent-permission-prompt, agent-turn-complete, agent-idle-reminder) with their notifications.* paths as anchor tokens, so they resolve through SettingsSearchIndex and highlight the rows.
— Claude Code
There was a problem hiding this comment.
Thanks — the curated SettingsSearchIndex entries now cover those three notification rows, so the parent comment is resolved here.
Thanks for the feedback! I've saved this as a new learning to improve future reviews.
| /// meta segment, and the `hadPendingBackgroundWorkAtStop` cache the idle_prompt | ||
| /// path reads. Drives the real CLI against the mock socket server, exactly like | ||
| /// `ClaudeNotificationStatusLifecycleTests`. | ||
| @Suite(.serialized) |
There was a problem hiding this comment.
P3: Use XCTest instead of Swift Testing for tests that spawn the CLI process/socket harness per team guidance. Swift Testing is reserved for pure decision/unit tests.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxTests/ClaudeBackgroundWorkNotifyTests.swift, line 10:
<comment>Use XCTest instead of Swift Testing for tests that spawn the CLI process/socket harness per team guidance. Swift Testing is reserved for pure decision/unit tests.</comment>
<file context>
@@ -0,0 +1,214 @@
+/// meta segment, and the `hadPendingBackgroundWorkAtStop` cache the idle_prompt
+/// path reads. Drives the real CLI against the mock socket server, exactly like
+/// `ClaudeNotificationStatusLifecycleTests`.
+@Suite(.serialized)
+struct ClaudeBackgroundWorkNotifyTests {
+ private func notifyLine(_ snapshot: [String], containing needle: String) -> String? {
</file context>
There was a problem hiding this comment.
Keeping Swift Testing here deliberately: the sibling process-spawning claude-hook suites (e.g. ClaudeNotificationStatusLifecycleTests) already use Swift Testing with this exact harness, so matching the sibling convention keeps one suite style for the claude-hook behavior tests.
— Claude Code
There was a problem hiding this comment.
The sibling-suite convention point is fair, and the parent comment is too broad for this PR. This ClaudeBackgroundWorkNotifyTests suite matches the existing Swift Testing harness used by the other claude-hook behavior tests, including ClaudeNotificationStatusLifecycleTests.
runStopHook called context.cleanup() (which deletes the temp dir including claude-hook-sessions.json) before the test read hadPendingBackgroundWorkAtStop, so the three cache assertions read a deleted file and failed. Capture the cached value inside the helper before cleanup and return it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be4cf8ced8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if candidate.hasPrefix("c=") { | ||
| meta = candidate |
There was a problem hiding this comment.
When an existing notify_target_async caller sends a normal body that contains a pipe followed by text starting with c= (for example title|subtitle|result|c=value), this branch treats that tail as metadata and removes it from the delivered body, whereas the previous maxSplits: 2 parser preserved it as part of the body. Since arbitrary socket/CLI notification bodies could already contain |, please only strip the fourth segment after validating the full agent-meta grammar (e.g. category plus pending flag) or otherwise fold it back into the body.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ca6c3cd: the 4th segment is treated as metadata only when it parses as the FULL c=;p=<0|1> grammar; a body tail like '|c=value' fails the parse (no valid p=) and is folded back into the delivered body. Regression-covered in AgentNotificationGateTests.metaRequiresValidPendingFlag.
— Claude Code
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)
cmuxTests/ClaudeBackgroundWorkNotifyTests.swift (1)
196-203: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMissing success assertion on the setup Stop hook.
Unlike the sibling test
idlePromptAfterPendingStopReadsCachedPending(which callsharness.assertSuccessfulHook(stopResult)at Line 163), this test discards the Stop hook's result with_ =and never verifies it succeeded. If the "idle" Stop hook silently fails, this test would still proceed to assertidle_promptpending=0 behavior against an unverified/incorrect prior state, potentially masking a real regression.🧪 Proposed fix
- _ = harness.runProcess( + let stopResult = harness.runProcess( executablePath: context.cliPath, arguments: ["hooks", "claude", "stop"], environment: environment, standardInput: #"{"session_id":"\#(session)","cwd":"/tmp/x","hook_event_name":"Stop","last_assistant_message":"ok","background_tasks":[],"session_crons":[]}"#, timeout: 5 ) `#expect`(handled.wait(timeout: .now() + 5) == .success) + harness.assertSuccessfulHook(stopResult)🤖 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 `@cmuxTests/ClaudeBackgroundWorkNotifyTests.swift` around lines 196 - 203, The setup Stop hook result is being ignored in ClaudeBackgroundWorkNotifyTests, so the test never verifies the hook succeeded before asserting idle prompt behavior. Capture the result from harness.runProcess for the `hooks claude stop` call and assert it with the same success check used in `idlePromptAfterPendingStopReadsCachedPending` (for example via `harness.assertSuccessfulHook(...)`) before waiting on `handled`.
🤖 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 `@cmuxTests/ClaudeBackgroundWorkNotifyTests.swift`:
- Around line 196-203: The setup Stop hook result is being ignored in
ClaudeBackgroundWorkNotifyTests, so the test never verifies the hook succeeded
before asserting idle prompt behavior. Capture the result from
harness.runProcess for the `hooks claude stop` call and assert it with the same
success check used in `idlePromptAfterPendingStopReadsCachedPending` (for
example via `harness.assertSuccessfulHook(...)`) before waiting on `handled`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 46fc101b-6f7f-42bf-8d12-a4e9b2f61d40
📒 Files selected for processing (1)
cmuxTests/ClaudeBackgroundWorkNotifyTests.swift
There was a problem hiding this comment.
2 issues found across 6 files (changes from recent commits).
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/AgentNotificationGate.swift">
<violation number="1" location="Sources/AgentNotificationGate.swift:38">
P2: The meta grammar validation is too permissive: `AgentNotificationMeta` returns a valid instance whenever a `c=` field is found (with any unrecognized value falling back to `.other`), without requiring the `p=` (pending) field. This means a legitimate notification body segment like `c=value` would be incorrectly recognized as agent metadata and stripped from the delivered body. Consider requiring both the `c=` and `p=` fields to be present before treating the segment as agent meta, or restrict accepted category values to known cases only (returning `nil` instead of `.other` for unknown values).</violation>
<violation number="2" location="Sources/AgentNotificationGate.swift:46">
P3: Avoid adding `AgentNotificationGate` as a caseless static namespace; make the gate a constructable service owned/injected by the notification path instead of another ambient global API.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The unconditional '@testable import cmux' failed under the Debug cmux-unit build where the app module is cmux_DEV, not cmux, so the test bundle didn't compile. Match the repo pattern: #if canImport(cmux_DEV) ... #elseif canImport(cmux). Caught on the AWS M4 Pro runner. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…und work pending The stop handler set the pane pill to 'Idle' on every turn end, which is misleading when a background task or Monitor is still running. Reuse the hasPendingBackgroundWork signal: pending -> 'Running' (bolt), truly-idle -> 'Idle' (unchanged). Hibernation lifecycle (set_agent_lifecycle) is left to its own PR; this only fixes the visible status pill. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9243b82f55
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| case "idle_prompt": | ||
| notifyCategory = .idleReminder | ||
| notifyPending = (mappedSession?.hadPendingBackgroundWorkAtStop == true) |
There was a problem hiding this comment.
When this idle_prompt path reads a cached pending Stop, the payload is tagged c=idle-reminder;p=1 so the app suppresses the banner, but the hook still continues below to upsert agentLifecycle: .needsInput and send set_status ... Needs input. Because Claude emits idle prompts while background tasks/Monitors are still running, the sidebar/lifecycle flips from the new pending-work Running state to a false waiting-for-input state even though the notification was gated; skip the visible needs-input mutation (or keep running) when notifyPending is true.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ca6c3cd: a pending idle_prompt (cached background work) no longer upserts .needsInput or sets the 'Needs input' pill; the pane stays Running and the next authoritative Stop reconciles. A genuine (not-pending) idle prompt still flips the pill. Covered by tests.
— Claude Code
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
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/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/AppSection.swift">
<violation number="1" location="Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/AppSection.swift:570">
P2: Add search/anchor entries for these new notification rows. As written, their configurationReview paths do not resolve through SettingsSearchIndex, so the new settings are not reachable/highlightable from settings search and the row-anchor contract drifts.</violation>
</file>
<file name="cmuxTests/ClaudeBackgroundWorkNotifyTests.swift">
<violation number="1" location="cmuxTests/ClaudeBackgroundWorkNotifyTests.swift:10">
P3: Use XCTest instead of Swift Testing for tests that spawn the CLI process/socket harness per team guidance. Swift Testing is reserved for pure decision/unit tests.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
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)
cmuxTests/ClaudeBackgroundWorkNotifyTests.swift (1)
189-227: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing hook-success assertion on the precondition
stopcall.The initial
stopinvocation's result is discarded with_ =and never passed toharness.assertSuccessfulHook(...), unlike the analogous precondition step inidlePromptAfterPendingStopReadsCachedPending(Line 174:harness.assertSuccessfulHook(stopResult)). If thestophook itself fails silently, this test would only surface a downstream failure on theidle_promptnotification assertion, making the root cause harder to diagnose.🧪 Proposed fix
- _ = harness.runProcess( + let stopResult = harness.runProcess( executablePath: context.cliPath, arguments: ["hooks", "claude", "stop"], environment: environment, standardInput: #"{"session_id":"\#(session)","cwd":"/tmp/x","hook_event_name":"Stop","last_assistant_message":"ok","background_tasks":[],"session_crons":[]}"#, timeout: 5 ) `#expect`(handled.wait(timeout: .now() + 5) == .success) + harness.assertSuccessfulHook(stopResult)🤖 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 `@cmuxTests/ClaudeBackgroundWorkNotifyTests.swift` around lines 189 - 227, The precondition stop hook result is being ignored in idlePromptAfterIdleStopTagsNotPending, so a failing stop would be hidden until later assertions. Capture the result of the hooks claude stop call in a named variable and pass it to harness.assertSuccessfulHook, matching the pattern used in idlePromptAfterPendingStopReadsCachedPending, so the test fails at the correct step if stop is unsuccessful.CLI/cmux.swift (1)
23229-23306: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winThread pending work into hibernation state
Stop still records
.idlewhenhasPendingBackgroundWorkis true. That flag only feeds the idle_prompt notification gate; hibernation readsagentLifecycle, so a session with live background work can still become eligible for suspension onceidleSecondselapse. Thread the pending-work state into the lifecycle path too.🤖 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 23229 - 23306, The stop-handling path in cmux.swift still writes `.idle` via the session update and lifecycle setter even when `hasPendingBackgroundWork` is true, so background work is not reflected in the hibernation eligibility state. Update the Claude hook stop flow around `summarizeClaudeHookStop`, `sessionStore.upsert`, and `setAgentLifecycle` so pending work is threaded into the persisted lifecycle, not just `hadPendingBackgroundWorkAtStop`, and make the status/notification logic derive from that shared state.
🤖 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/cmux.swift`:
- Around line 23229-23306: The stop-handling path in cmux.swift still writes
`.idle` via the session update and lifecycle setter even when
`hasPendingBackgroundWork` is true, so background work is not reflected in the
hibernation eligibility state. Update the Claude hook stop flow around
`summarizeClaudeHookStop`, `sessionStore.upsert`, and `setAgentLifecycle` so
pending work is threaded into the persisted lifecycle, not just
`hadPendingBackgroundWorkAtStop`, and make the status/notification logic derive
from that shared state.
In `@cmuxTests/ClaudeBackgroundWorkNotifyTests.swift`:
- Around line 189-227: The precondition stop hook result is being ignored in
idlePromptAfterIdleStopTagsNotPending, so a failing stop would be hidden until
later assertions. Capture the result of the hooks claude stop call in a named
variable and pass it to harness.assertSuccessfulHook, matching the pattern used
in idlePromptAfterPendingStopReadsCachedPending, so the test fails at the
correct step if stop is unsuccessful.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ae82f01d-0d6e-4528-a50d-db618548fc6d
📒 Files selected for processing (2)
CLI/cmux.swiftcmuxTests/ClaudeBackgroundWorkNotifyTests.swift
# Conflicts: # .github/swift-file-length-budget.tsv
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb4f9f462e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| icon: "pause.circle.fill", | ||
| color: "#8E8E93" | ||
| ) | ||
| if hasPendingBackgroundWork { |
There was a problem hiding this comment.
With agent hibernation enabled and a Claude Stop that still has a running background task or pending cron, this branch only changes the visible sidebar status to Running, but the session store and socket lifecycle were already set to .idle just above. The hibernation planner uses the lifecycle state (.idle is the only hibernatable state), not the status text, so after the idle timeout/max-live-terminal threshold it can hibernate a pane while that background work is still active; publish/store .running for the pending path as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in dd4b07a (review round 2): the pending stop now publishes lifecycle .running in BOTH the session-store upsert and set_agent_lifecycle, so the hibernation planner cannot select the pane while background work is live. Test-covered (pending stop asserts set_agent_lifecycle claude_code running and no idle).
— Claude Code
…tion gate - parseNotificationPayload accepts a 4th segment as gating metadata only when it parses as the FULL c=<category>;p=<0|1> grammar (AgentNotificationMeta now requires a valid p=0|1); anything else folds back into the body, so a legacy body tail like "|c=value" is never stripped or misread (Codex P2, cubic P2, CodeRabbit fail-closed note resolved by rejecting invalid meta as meta). - Apply the settings gate on every metadata-aware delivery path (notify, notify_surface, notify_target, notify_target_async) via one shared shouldDeliverAgentNotification helper (CodeRabbit). - Replace the caseless AgentNotificationGate enum namespace with a free nonisolated agentNotificationShouldDeliver function per the cmux no-static-namespace policy (Greptile/cubic). - idle_prompt with cached pending background work no longer flips the pane to "Needs input" (lifecycle + pill): the banner is suppressed app-side and the pane is still Running; a genuine idle prompt still flips it (Codex P2, cubic). - Settings search: curated entries for the three new notification rows so they resolve through SettingsSearchIndex (cubic P2). - Tests: meta grammar requires p=0|1; idle-pending skips Needs input pill; idle-not-pending still sets it; assert the stop hook result in the idle test (cubic P3). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CmuxSettingsUI's everyCuratedSettingEntryIsReachable failed in swift-package-tests: the new curated entries' anchors weren't backed by the rowConfigPaths fixture. Register the three notifications.agent* row paths. Verified locally: CmuxSettingsUI 78/78, CmuxSettings 230/230. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A fullyIdle=false waiting cue is gated app-side (idle-reminder p=1), but the generic handler still flipped the session/lifecycle/status to needs-input, overriding Running while background work was live. Mirror the Claude pending idle_prompt fix: keep lifecycle .running and skip the needs-input pill; the fullyIdle turn boundary reconciles. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c32a893f0e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| case "Completed": | ||
| notifyCategory = .turnComplete | ||
| notifyPending = (mappedSession?.hadPendingBackgroundWorkAtStop == true) |
There was a problem hiding this comment.
When a Claude Notification payload has no notification_type and the fallback classifier recognizes it as Completed, this branch tags the alert as turn-complete, but suppressNeedsInputState remains false so the code below still upserts .needsInput and sends the Needs input status. In the exact older/typeless completion case this fallback is meant to support—especially after a pending Stop where the app may suppress the banner—the sidebar/lifecycle flips to waiting-for-input even though the agent is finishing/running rather than blocked on the user.
Useful? React with 👍 / 👎.
The enum-valued setting rode the generic unvalidated string path, so a typo like 'nevr' persisted and silently fell back to whenIdle. Validate against AgentTurnCompleteMode before applying and log invalid values, mirroring the notifications.sound allowlist handling. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… cues Complete the round-12 invariant: the store upsert for a fullyIdle=false waiting cue now records runtimeStatus .running alongside the .running lifecycle, so stale-idle protection and duplicate-background-idle suppression keep seeing the live session. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # .github/swift-file-length-budget.tsv
…esktop-notifications-row-stuck-on-p Resolves conflicts from #7129 (per-category agent notification settings): - AppSection.swift: keep BOTH the desktopNotifications permission model (this branch) and the agentPermissionPrompt/agentTurnComplete/ agentIdleReminder rows (#7129) across the @State decls, init, and the startSettingsObservation array; the body auto-merged with both row sets. - .github/swift-file-length-budget.tsv: regenerated via scripts/swift_file_length_budget.py --write-budget (never hand-edited). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…agents PR #7129 landed the same claude Stop-lane background-work gate this branch carries, plus notification category gating. Resolution unifies the two: - One background-work parser: hasActiveClaudeBackgroundWork now delegates to AgentBackgroundWorkStatus(hookObject:) (unit-tested, fail-closed on present-but-unreadable payloads; non-terminal statuses count as live). Main's inline predicate (exact "running" only, fails open on malformed) is replaced. Main's tests only pin running/empty/cron/absent, all compatible. - Stop lane: main's shape kept (hasPendingBackgroundWork, localized Running/Idle pill, hadPendingBackgroundWorkAtStop cache). - Notification lane: union of main's category gating (notifyCategory, suppressNeedsInputState) and this branch's blocking gate — lifecycle flips to .needsInput only for genuinely blocking prompts (summary.isBlocking && !suppressNeedsInputState). Main's unconditional .needsInput write would have reintroduced the routine-reminder clobber this PR fixes. - Generic lane: persisted lifecycle composes suppressPendingWaitingState (.running) > isBlocking (.needsInput) > completion (.idle) > preserve. - AgentHookNotificationSummary carries both isBlocking and notifyCategory. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixes the noise paths behind grok Build notification spam (#7611): - Dedupe every notification status, not just .idle: fingerprints are now status + a stable FNV-1a body hash (cross-process safe; the session store persists between CLI invocations). .idle keeps the whole-turn "idle-turn" fingerprint so incidental completion-keyword messages cannot re-ding. - Dedupe identical permission prompts per turn: live capture from Grok Build 0.2.91 shows an identical generic {"notificationType":"permission_prompt","message":"Tool permission requested"} Notification for every tool step, even in auto-approve mode where nothing awaits the user, so long tasks ring once per step. Identical bodies now dedupe within the turn, prompt-submit re-arms delivery for the next turn, and permission prompts with novel content still always deliver. - Make every summary carry a notifyCategory: the "needs your attention" fallback, arbitrary-text attention alerts, and the stale-record rebuild path now tag c=idle-reminder, so the per-category settings from #7129 can silence them. Errors keep the explicit .other always-deliver exemption (unchanged wire behavior). - Preserve dedupe across grok's mid-session SessionStart re-fires (auto-continue/restarts) instead of re-arming the completion ding; prompt-submit clearing is unchanged. - Replace the single-slot emitted-fingerprint store with a small per-session map (60 min window, 16-entry cap, legacy back-compat) so an interleaved notification cannot evict the idle-turn fingerprint. - Recognize grok's camelCase "notificationType" payload key in the classifier signal (captured from real traffic). The classification/dedupe policy moves to a new pure file, CLI/AgentHookNotificationPolicy.swift, compiled into both cmux-cli and cmuxTests (same pattern as FeedEventClassifier), with unit coverage of the classification table, fingerprint stability, and the app-gate meta round-trip. Claude lane wire output and antigravity fullyIdle gating are byte-identical. No new user-facing strings; existing localization keys move verbatim. Closes #7611 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* mux: red regression tests for grok hook notification noise Integration coverage for #7611 driven through the spawn-the-CLI harness against a mock socket, using payload shapes captured from a live Grok Build 0.2.91 session: - repeated identical "waiting for input" Notification events must dedupe within a turn (currently every repeat delivers a fresh banner + sound) - repeated identical permission_prompt notifications must dedupe per turn: grok emits {"notificationType":"permission_prompt","message": "Tool permission requested"} for EVERY tool step, even in auto-approve mode where nothing awaits the user, so a 6-step task rings 6 times; a prompt-submit (new turn) must re-arm delivery - distinct permission prompts must each deliver (always-deliver for novel approval content is preserved) - unparseable payloads rebuilt from the stored session record must carry gateable c=idle-reminder meta and dedupe (currently untagged, so the per-category notification settings cannot silence them) - a mid-session SessionStart re-fire must not re-arm the completion dedupe (currently clearNotificationEmission re-arms the same ding) - guards: antigravity error notifications stay untagged, incidental completion keywords cannot re-ding after the real turn-complete Tests are committed first and are red on this commit by design; the fix lands in the follow-up commit (two-commit red/green policy). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * mux: dedupe and gate all grok agent-hook notifications Fixes the noise paths behind grok Build notification spam (#7611): - Dedupe every notification status, not just .idle: fingerprints are now status + a stable FNV-1a body hash (cross-process safe; the session store persists between CLI invocations). .idle keeps the whole-turn "idle-turn" fingerprint so incidental completion-keyword messages cannot re-ding. - Dedupe identical permission prompts per turn: live capture from Grok Build 0.2.91 shows an identical generic {"notificationType":"permission_prompt","message":"Tool permission requested"} Notification for every tool step, even in auto-approve mode where nothing awaits the user, so long tasks ring once per step. Identical bodies now dedupe within the turn, prompt-submit re-arms delivery for the next turn, and permission prompts with novel content still always deliver. - Make every summary carry a notifyCategory: the "needs your attention" fallback, arbitrary-text attention alerts, and the stale-record rebuild path now tag c=idle-reminder, so the per-category settings from #7129 can silence them. Errors keep the explicit .other always-deliver exemption (unchanged wire behavior). - Preserve dedupe across grok's mid-session SessionStart re-fires (auto-continue/restarts) instead of re-arming the completion ding; prompt-submit clearing is unchanged. - Replace the single-slot emitted-fingerprint store with a small per-session map (60 min window, 16-entry cap, legacy back-compat) so an interleaved notification cannot evict the idle-turn fingerprint. - Recognize grok's camelCase "notificationType" payload key in the classifier signal (captured from real traffic). The classification/dedupe policy moves to a new pure file, CLI/AgentHookNotificationPolicy.swift, compiled into both cmux-cli and cmuxTests (same pattern as FeedEventClassifier), with unit coverage of the classification table, fingerprint stability, and the app-gate meta round-trip. Claude lane wire output and antigravity fullyIdle gating are byte-identical. No new user-facing strings; existing localization keys move verbatim. Closes #7611 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This branch was successfully deployed
1 participant
Problem
cmux fired a "Completed" notification on every Claude turn end, even when the turn was intermediate: a background build, a
Monitor, or a scheduled wakeup was still running and would re-invoke the agent. Real repro: a chrome build + a 3600s Monitor were still running when cmux pinged "done" at the 16-minute mark.Empirical basis (claude 2.1.197, injected hooks)
Stophook payload carriesbackground_tasks({id,type,status,description,command}) andsession_crons. Both[]on a truly-idle turn; a completed task vanishes from the array. Present since claude v2.1.145; absent (nil) on older clients.Notificationpayload does not carrybackground_tasks. It hasnotification_type:permission_prompt(blocked on user, immediate) andidle_prompt(~60s after turn end, and fires even while a background task runs).Approach
The CLI has the runtime signal; the app has the config. The CLI classifies each agent notification and forwards a
c=<category>;p=<0|1>meta segment appended to thenotify_target_asyncpayload; the app gates delivery by user settings.agentPermissionPrompt(on)agentTurnComplete(whenIdle)agentTurnComplete=always/neveragentIdleReminder(on)pending= anybackground_tasks[].status == "running"OR non-emptysession_crons. A persistentMonitorkeepspending=1for its lifetime, sowhenIdleintentionally holds the done-ping until the watcher stops;permission_promptbypasses the gate.Changes
CLI/cmux.swift):hasActiveClaudeBackgroundWork(_:),notifyMeta,notificationPayload(meta:); cacheshadPendingBackgroundWorkAtStopon the session record (idle_prompt reads it since its payload lacksbackground_tasks). nil/absent arrays → not pending, so claude < 2.1.145 behaves as before.Sources/TerminalController.swift):parseNotificationPayloadgains an optional 4th meta segment, treated as meta only when it begins withc=(else folded back into the body, so legacy callers whose body contains|parse byte-identically).AgentNotificationGatedecides delivery; gated innotifyTargetQueuedbefore enqueue.notifications.agentPermissionPrompt/agentTurnComplete/agentIdleReminder— DefaultsKeys, Settings UI rows (2 toggles + enum picker),cmux.jsonbridge + valid-paths,cmux.schema.json, en/ja/ko/uk localization.ClaudeBackgroundWorkNotifyTests(7 CLI behavioral: meta tags + cache across stop/permission/idle) andAgentNotificationGateTests(8 pure: full decision table + meta parser), wired into pbxproj.Default-behavior note
For the common case (no background work)
pending=0, so you still get the instant "done" ping. Only when background work is pending doeswhenIdlesuppress it.permission_promptstill notifies by default.🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touches notification delivery, agent lifecycle/hibernation, and large CLI hook paths; wrong gating could hide permission prompts or duplicate/suppress completion pings, though defaults preserve most legacy behavior and tests cover key paths.
Overview
Fixes premature “agent finished” pings when Claude (and other agents) still have running background tasks or scheduled crons after a turn ends.
CLI classifies hook notifications and appends an optional 4th pipe segment
c=<category>;p=<0|1>onnotify_target_async. It detects pending work frombackground_tasks/session_crons, cacheshadPendingBackgroundWorkAtStopfor lateridle_promptevents (which lack those fields), keeps lifecycle/status Running instead of Idle while work is pending, and skips misleading Needs input for suppressed idle nags. Built-in agents (Claude, Grok, Antigravity) get consistent tagging and intermediate-event suppression.App parses that meta in
TerminalController, appliesagentNotificationShouldDeliverfrom newAgentNotificationGate, and drops gated payloads before enqueue. Untagged payloads behave as before.Settings:
notifications.agentPermissionPrompt,agentTurnComplete(whenIdle|always|never),agentIdleReminder— UI,cmux.json/schema, localization. Tests cover the decision table, meta parser, and CLI stop/notification behavior.Reviewed by Cursor Bugbot for commit f76143a. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Gates agent notifications by background work and per‑category settings so “Finished”/“Waiting” alerts only fire when truly idle across all built‑in agents. Keeps panes Running while work is pending, localizes the status pill, and adds per‑category notification controls.
New Features
c=<category>;p=<0|1>and gate in‑app viaAgentNotificationGateonnotify,notify_surface,notify_target, andnotify_target_async; cachehadPendingBackgroundWorkAtStopsoidle_promptreads pending.notifications.agentPermissionPrompt,notifications.agentTurnComplete(whenIdledefault |always|never),notifications.agentIdleReminderwith UI, search entries,cmux.jsonbridge/schema, and localization.needs-permission.Bug Fixes
c=turn-complete|needs-permission|idle-reminder;p=0|1is accepted; extras/duplicates/unknown categories are ignored and left in the body; legacy payloads parse unchanged.idle_promptno longer flips “Needs input”. Pane shows localized Running and publishes.runninglifecycle; persist.runningruntimeStatus for suppressed waiting cues.notifications.agentTurnCompletefromcmux.jsonagainstwhenIdle|always|never.Written for commit 5d4971d. Summary will update on new commits.
Summary by CodeRabbit