Repository navigation
Disable Claude OSC notifications in cmux wrapper - #3418
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds notification suppression to the bundled Claude wrapper settings, extends CI to run two additional Claude hook regressions, updates CLI summary parsing to detect/short-circuit on assistant preamble/last-assistant fields, and adds an end-to-end test that validates Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Runner
participant CLI as cmux CLI
participant Socket as Local Socket Server
participant Notifier as notify_target_async
Test->>Socket: start capturing UNIX socket
Test->>CLI: run `cmux claude-hook stop` (env overrides, Stop payload)
CLI->>Socket: connect and POST hook request (Stop payload with assistant preamble/last)
Socket-->>CLI: respond 200 OK
CLI->>Notifier: emit `notify_target_async` commands (includes final assistant text)
Socket-->>Test: captured commands recorded
Test->>Test: assert captured commands contain final assistant text
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/8 reviews remaining, refill in 2 minutes and 5 seconds.Comment |
Greptile SummaryThis PR suppresses Claude Code's built-in OSC/terminal notifications when launched through the cmux wrapper by prepending Confidence Score: 5/5Safe to merge — minimal, well-tested change to a single JSON string and its matching assertion. The change is a single-field addition to a static JSON string, with a direct regression test and CI coverage. No logic paths are restructured, and existing tests continue to pass. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[claude wrapper invoked] --> B{CMUX socket live?}
B -- yes --> C[Build HOOKS_JSON\npreferredNotifChannel: notifications_disabled\n+ lifecycle hooks]
B -- no --> D[Skip hook injection\nbut HOOKS_JSON still passed]
C --> E{SKIP_SESSION_ID?}
D --> E
E -- yes --> F[exec real claude\n--settings HOOKS_JSON]
E -- no --> G[uuidgen session ID\nexec real claude\n--session-id ID\n--settings HOOKS_JSON]
F --> H[Claude Code starts\nOSC notifications suppressed\ncmux hooks are only notify source]
G --> H
Reviews (1): Last reviewed commit: "fix: disable Claude OSC notifications in..." | Re-trigger Greptile |
![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 the current code and only fix it if needed.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 14655-14657: The new user-facing literals assigned to subtitle
("Completed" and "Completed in \(projectName)") must be replaced with localized
strings; update the assignment in the code that sets var subtitle (where
projectName is checked) to use String(localized: "notification.completed",
defaultValue: "Completed") for the simple case and String(localized:
"notification.completed_in_project", defaultValue: "Completed in %@" ) or
similar for the projectName case, using String format/localization APIs so the
projectName is injected into the localized template; ensure you add the
corresponding localization keys and default English values.
In `@tests/test_claude_hook_stop_last_assistant.py`:
- Around line 88-97: The post-first-chunk idle_deadline is set to 0.5s which is
too short for CI; in the receive loop inside
tests/test_claude_hook_stop_last_assistant.py (the while loop that uses
idle_deadline, conn.recv, buffer and self.stop), increase the idle window after
receiving the first chunk (the assignment idle_deadline = time.time() + 0.5) to
a larger value (for example 2.0 or 3.0 seconds) so late-arriving data on loaded
runners isn't truncated; keep the initial pre-first-chunk deadline at 6.0s
unchanged.
- Around line 19-21: When an explicit CLI path is provided in the environment
variable (the variable named explicit), fail immediately if that path does not
exist or is not executable instead of falling back to other binaries. Change the
logic around the explicit variable so: if explicit is set and not
(os.path.exists(explicit) and os.access(explicit, os.X_OK)) then raise an
exception (e.g., RuntimeError or pytest.fail) with a clear message including the
value of explicit; otherwise, if it exists and is executable return explicit.
This ensures the test fails fast when CMUX_CLI_BIN/CMUX_CLI points to an invalid
binary.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 39c19a45-f3eb-46bf-8231-2d8d9c7d2e50
📒 Files selected for processing (3)
.github/workflows/ci.ymlCLI/cmux.swifttests/test_claude_hook_stop_last_assistant.py
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/ci.yml
This branch was successfully deployed
1 participant
Summary
preferredNotifChannel: notifications_disabledso cmux hook notifications are the only notification source.last_assistant_message/assistantPreamble) before transcript fallback, so the sidebar notification shows2instead of stale generic text.Testing
./scripts/setup.sh./scripts/reload.sh --tag closc./scripts/reload.sh --tag cstopbash -n Resources/bin/claudepython3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsvpython3 tests/test_claude_wrapper_hooks.pyCMUX_CLI_BIN='/Users/lawrence/Library/Developer/Xcode/DerivedData/cmux-cstop/Build/Products/Debug/cmux DEV cstop.app/Contents/Resources/bin/cmux' python3 tests/test_claude_hook_stop_last_assistant.pyIssue
Task: suppress Claude Code OSC notifications when launched through the cmux Claude wrapper, and keep Stop hook notifications sourced from hook data.
Summary by CodeRabbit
New Features
Tests
Chores