Repository navigation
Closes #3602: Suppress ignored Claude notifications - #3959
austinywang wants to merge 21 commits into
Conversation
📝 WalkthroughWalkthroughAdds ignored-CLAUDE-notification-type support: normalization utilities, settings/UserDefaults and file/template parsing, CLI parsing/fallback extraction, suppression decision that short-circuits hook rendering, environment/build wiring, unit/integration tests, docs, and CI test additions. ChangesNotification Suppression Feature
Sequence Diagram(s)sequenceDiagram
participant User as User/Config
participant Settings as Settings System
participant CLI as cmux CLI
participant Parser as ClaudeHook Parser
participant Suppressor as Suppression Logic
participant Output as stdout / hook commands
User->>Settings: Configure ignoredClaudeNotificationTypes (file / env / UserDefaults)
Settings->>Settings: Normalize & store values
Settings->>CLI: Inject via CMUX_CLAUDE_IGNORED_NOTIFICATION_TYPES env
User->>CLI: Send claude-hook notification payload
CLI->>Parser: Parse input, extract notification type candidates (including fallback)
Parser->>Suppressor: Provide normalized candidate set
Suppressor->>Settings: Load normalized ignored set
Suppressor->>Suppressor: Compute intersection
alt Intersection non-empty
Suppressor->>Output: Emit breadcrumb, print "OK"
Suppressor->>CLI: Return early (skip rendering)
else Intersection empty
Parser->>Output: Continue summary/mapping and emit notification commands
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 2 warnings)
✅ Passed checks (12 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…dle-prompt-notifs
Greptile SummaryThis PR closes #3602 by adding
Confidence Score: 5/5Safe to merge. All four issues flagged in previous review rounds are verifiably addressed, the new package boundary is correctly scoped, and both unit and integration tests cover the key edge cases. The notification suppression path is narrow and well-isolated: it only fires in the CLI claude-hook notification subcommand, reads settings files synchronously before falling back to env or UserDefaults, and returns early without touching any shared mutable state. The shared normalization logic is now in a single place. The wrong-type and missing-key precedence cases are explicitly tested, and the hook_event_name exclusion regression prevents the accidental wildcard suppression footgun. No actor isolation regressions, no sensitive data in telemetry, and the 78-line addition to the CLI file is well within the growth budget. No files require special attention. Important Files Changed
Reviews (17): Last reviewed commit: "refactor: isolate Claude notification do..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…dle-prompt-notifs
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…dle-prompt-notifs
…dle-prompt-notifs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 16879-16882: The breadcrumb currently logs the full
notificationTypes set which may contain freeform user content; change the code
before calling telemetry.breadcrumb so you compute a safe matched set (e.g., let
matchedTypes = notificationTypes.intersection(ignoredNotificationTypes) or
filter notificationTypes against a whitelist of known ignored types), and then
pass only matchedTypes.sorted().joined(separator: ",") (or omit the "types" key
entirely when matchedTypes.isEmpty) into telemetry.breadcrumb; update the
telemetry.breadcrumb call that uses notificationTypes to use matchedTypes (or
skip the field) to avoid logging unsanitized/high-cardinality values.
🪄 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: e6255e3d-939b-4c7a-a859-87260bca2dc0
📒 Files selected for processing (11)
CLI/cmux.swiftSources/ClaudeNotificationTypeNormalization.swiftSources/CmuxSettingsJSONPathSupport.swiftSources/GhosttyTerminalView.swiftSources/KeyboardShortcutSettingsFileStore+Template.swiftSources/KeyboardShortcutSettingsFileStore.swiftSources/cmuxApp.swiftcmux.xcodeproj/project.pbxprojcmuxTests/KeyboardShortcutSettingsFileStoreStartupTests.swiftdocs/notifications.mdtests/test_claude_hook_ignored_notification_types.py
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 11 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="CLI/cmux.swift">
<violation number="1" location="CLI/cmux.swift:18955">
P2: Raw fallback payloads are matched as a single string, so ignored notification types embedded in fallback JSON (e.g. `notification_type`) may not be suppressed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
Dismissed as stale. The requested telemetry privacy fix was applied in a22d3b6 and CodeRabbit acknowledged the inline thread as addressed; current head also resolves the remaining Cubic fallback parsing feedback.
…dle-prompt-notifs
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 `@CLI/cmux.swift`:
- Around line 16702-16708: The branch that currently returns a bare "OK"
(print("OK")) when suppressedNotificationTypes is non-empty should be replaced
with a product-facing message: keep the machine-readable breadcrumb via
telemetry.breadcrumb("claude-hook.notification.suppressed", ...) but replace
print("OK") with a sanitized human message that explains notifications were
suppressed, lists the suppressed types in user-friendly form, and provides 1–2
next actions (e.g., "See cmux.json to modify notification settings" and "Run
cmux --show-notifications to view current rules"). Update the output path so the
machine token remains unchanged for automated consumers while the human message
is written to stdout/stderr (sanitized) or a log, referencing
suppressedNotificationTypes and cmux.json in the text.
- Around line 18804-18818: The loop over paths currently continues scanning
lower-precedence files when loadIgnoredClaudeNotificationTypes returns
.parsed(nil), which resurrects legacy values; update the switch in the for-loop
(the call sites: loadIgnoredClaudeNotificationTypes,
normalizedClaudeNotificationTypes, and the foundSettingsFile flag) so that when
you get .parsed(let values) and values is nil you treat it as an explicit
removal and immediately return an empty array ([]); keep the existing behavior
of returning normalizedClaudeNotificationTypes(values) when values is non-nil,
and preserve the final return of nil only when no settings file was found.
In `@tests/test_claude_hook_ignored_notification_types.py`:
- Around line 25-37: The test currently falls back to any system "cmux"
(shutil.which) which can pick up stale/installed binaries; change the resolution
so the test requires CMUX_CLI_BIN or CMUX_CLI to point to the just-built
artifact or else fail: read CMUX_CLI_BIN/CMUX_CLI first and validate it exists
and is executable, accept candidates found in the build-only locations (the
existing candidates list populated from
"~/Library/Developer/Xcode/DerivedData/*/Build/Products/Debug/cmux" and
"/tmp/cmux-*/Build/Products/Debug/cmux") but remove the global PATH fallback
(the shutil.which branch) and instead raise a RuntimeError if no env var or
build-directory candidate is available; refer to the existing symbols
candidates, CMUX_CLI_BIN/CMUX_CLI, shutil.which, and os.path.getmtime when
implementing the checks.
🪄 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: 31c6ba89-463d-44df-b3ba-7b653b9d3601
📒 Files selected for processing (8)
CLI/cmux.swiftSources/CmuxSettingsJSONPathSupport.swiftSources/GhosttyTerminalView.swiftSources/KeyboardShortcutSettingsFileStore+Template.swiftSources/KeyboardShortcutSettingsFileStore.swiftSources/cmuxApp.swiftcmux.xcodeproj/project.pbxprojtests/test_claude_hook_ignored_notification_types.py
All three CodeRabbit comments from this review have been addressed: two were fixed in 0d442bc and the hook stdout item was confirmed as an intentional Claude hook protocol requirement in-thread. Dismissing the stale changes-requested review after resolving its threads.
| guard let values = rawValues as? [String] else { | ||
| return .parsed(nil) | ||
| } | ||
| return .parsed(values) |
There was a problem hiding this comment.
notifications.ignoredClaudeNotificationTypes is present in the settings file but its value is not a [String] (e.g., the user wrote "idle_prompt" instead of ["idle_prompt"]), .parsed(nil) is returned. In ignoredClaudeNotificationTypesFromSettingsFiles, .parsed(nil) resolves to return [], which makes the settings file "win" with an empty ignore list — silently bypassing the env var the app already set from the still-valid UserDefaults value. The app correctly leaves UserDefaults untouched (logInvalid is called, nothing is written) and sets CMUX_CLAUDE_IGNORED_NOTIFICATION_TYPES from the preserved UserDefaults value, but the CLI never sees it because the settings-file lookup short-circuits. Returning .invalid here instead lets the loop fall through to the env var (and UserDefaults), which matches the app's intent for an invalid-type value.
| guard let values = rawValues as? [String] else { | |
| return .parsed(nil) | |
| } | |
| return .parsed(values) | |
| guard let values = rawValues as? [String] else { | |
| return .invalid | |
| } | |
| return .parsed(values) |
|
Addressed Greptile's latest settings-file loader finding in 270a7c7: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 18803-18808: The loop over paths currently ignores `.invalid`
results from loadIgnoredClaudeNotificationTypes(at:fileManager:) and continues
scanning, allowing ignoredClaudeNotificationTypes() to fall back to stale
env/UserDefaults; change the `.invalid` branch to treat the presence of a
malformed settings file as authoritative by returning an empty set immediately
(i.e., when loadIgnoredClaudeNotificationTypes(...) yields `.invalid` return an
empty Set from ignoredClaudeNotificationTypes()), so malformed files unblock
notifications instead of letting the loop continue.
🪄 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: 7d6c8c91-f727-4a1f-a51c-7c5b8b19ac21
📒 Files selected for processing (4)
.circleci/config.yml.github/workflows/ci.ymlCLI/cmux.swifttests/test_claude_hook_ignored_notification_types.py
The single CodeRabbit request from this review has been addressed in-thread and intentionally declined: malformed settings files must fall back to the app-preserved env/default path, matching the Cursor and Greptile findings fixed in 270a7c7 and 76ee9f0. The review is also for a previous head commit.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4c7ba93. Configure here.
| ) | ||
| } else if section.keys.contains("ignoredClaudeNotificationTypes") { | ||
| logInvalid("notifications.ignoredClaudeNotificationTypes", sourcePath: sourcePath) | ||
| } |
There was a problem hiding this comment.
App merge ignores hook precedence
Medium Severity
When a valid primary cmux.json exists but omits notifications.ignoredClaudeNotificationTypes, the Claude hook treats the ignore list as empty and still renders matching notifications. The settings file store can still merge ignored types from legacy or App Support configs into UserDefaults and terminal env, so suppression looks enabled while hooks do not suppress.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 4c7ba93. Configure here.
There was a problem hiding this comment.
1 issue found across 11 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/CMUXClaudeNotifications/Sources/CMUXClaudeNotifications/ClaudeNotifications.swift">
<violation number="1" location="Packages/CMUXClaudeNotifications/Sources/CMUXClaudeNotifications/ClaudeNotifications.swift:75">
P2: Plain string notification types are discarded because string extraction only attempts JSON parsing, so ignored-type suppression can fail for raw-string payload shapes.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| return values.flatMap { self.values(inJSONValue: $0, scope: scope) } | ||
| } | ||
| if let string = value as? String { | ||
| return values(inRawFallback: string, scope: scope) |
There was a problem hiding this comment.
P2: Plain string notification types are discarded because string extraction only attempts JSON parsing, so ignored-type suppression can fail for raw-string payload shapes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/CMUXClaudeNotifications/Sources/CMUXClaudeNotifications/ClaudeNotifications.swift, line 75:
<comment>Plain string notification types are discarded because string extraction only attempts JSON parsing, so ignored-type suppression can fail for raw-string payload shapes.</comment>
<file context>
@@ -0,0 +1,221 @@
+ return values.flatMap { self.values(inJSONValue: $0, scope: scope) }
+ }
+ if let string = value as? String {
+ return values(inRawFallback: string, scope: scope)
+ }
+ return []
</file context>
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
3 participants
Closes #3602
Summary
notifications.ignoredClaudeNotificationTypessupport for Claude Code notification suppression, withidle_promptdocumented as the recurring idle-prompt use case.CMUXClaudeNotificationsso the app and CLI share one domain boundary.ignoredClaudeNotificationTypesas an empty suppression set, so stale launch-time hook env cannot keep hiding notifications.Testing