Repository navigation
Add agent hook setup prompts - #3505
lawrencecchen wants to merge 13 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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 end-to-end "agent hooks": shell detection emits nudges, terminal socket accepts nudge commands (v1/v2), NotificationStore schedules actionable nudges and install/diff logic, UI surfaces review/install flows and settings controls, CLI hook dispatch prefers bundled CLI, and localization keys were added. ChangesAgent Hooks Setup & Integration
Sequence DiagramsequenceDiagram
participant Shell as Shell (zsh)
participant CLI as Bundled CLI (relay)
participant Socket as Unix Socket / JSON socket
participant TC as TerminalController
participant NS as NotificationStore
participant Hooks as AgentHookIntegrationSettings
participant UI as Notifications UI
Shell->>Shell: detect agent via _cmux_agent_hook_agent_for_command(cmd)
Shell->>CLI: call _cmux_cli_rpc_bg agent_hooks.nudge (or write unix-socket agent_hooks_nudge)
CLI->>Socket: deliver v2 JSON or v1 line command
Socket->>TC: dispatch agent_hooks.nudge / agent_hooks_nudge
TC->>NS: enqueue main-thread showSetupPromptIfNeeded(...)
NS->>Hooks: evaluate status, cooldown, diff/install availability
Hooks->>NS: addNotification(action: .agentHookSetup(agent))
NS->>UI: render notification with action buttons
UI->>Hooks: user taps Review/Install/Enable
Hooks->>Hooks: run diff or install, toggle wrapper, capture output
Hooks-->>UI: return localized result (success/failure/message)
UI->>NS: clear or keep notification
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (4 errors, 1 warning, 1 inconclusive)
✅ Passed checks (7 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 |
|
Reviews (5): Last reviewed commit: "Review agent hook diffs before install" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private var status: AgentHookIntegrationStatus { | ||
| let _ = refreshToken | ||
| return AgentHookIntegrationSettings.status(for: agent) | ||
| } |
There was a problem hiding this comment.
AgentHookSettingsRow.status is a computed property evaluated every time body runs. It calls AgentHookIntegrationSettings.status(for: agent), which performs FileManager.default.fileExists(atPath:) and try? String(contentsOfFile:encoding:) synchronously — two disk calls on the @MainActor every render cycle. With nine agents displayed in a ForEach, this means up to 18 synchronous file-system operations per body pass triggered by any refreshToken change.
The correct shape is to store the status in @State var status (or a local cache in an @Observable model), load it asynchronously in .task or .onReceive, and update it when statusDidChangeNotification fires — not recompute live from the file system inside body.
Rule Used: Flag SwiftUI changes that can cause stale state, b... (source)
| DispatchQueue.global(qos: .userInitiated).async { | ||
| let result = runInstallCommand( | ||
| executableURL: launch.executableURL, | ||
| arguments: launch.arguments, | ||
| fallbackCommand: agent.installCommand | ||
| ) | ||
| DispatchQueue.main.async { | ||
| NotificationCenter.default.post(name: statusDidChangeNotification, object: nil) | ||
| completion(result) | ||
| } | ||
| } |
There was a problem hiding this comment.
DispatchQueue.global + completion handler for cmux-controlled async work
installHooks(for:completion:) uses DispatchQueue.global(qos: .userInitiated).async with an @escaping completion callback, and both the callee and every caller (AgentHookSettingsRow.install(), TerminalNotificationActionButtons.install()) are cmux-owned code. Per the concurrency modernization rule, this should be an async function returning AgentHookInstallResult, with callers wrapping the call in Task { await ... } inside their button handlers.
runInstallCommand already blocks a thread via process.waitUntilExit(), so moving it to a structured-concurrency Task.detached(priority: .userInitiated) or an async function where Process is driven off the cooperative pool would give the same background-process semantics with proper cancellation support and no extra dispatch layers.
Rule Used: Flag new legacy async patterns in cmux-owned Swift... (source)
| private struct AgentHooksSettingsCard: View { | ||
| @AppStorage(AgentHookIntegrationSettings.promptEnabledKey) | ||
| private var promptEnabled = AgentHookIntegrationSettings.defaultPromptEnabled | ||
| @State private var refreshToken: UInt64 = 0 |
There was a problem hiding this comment.
refreshToken side channel creates a second owner for status state
AgentHooksSettingsCard holds a @State private var refreshToken: UInt64 that is incremented on every status-change notification and on every install completion, and is threaded as a @Binding into each AgentHookSettingsRow whose status computed property reads let _ = refreshToken to subscribe. This is a manual invalidation side channel that duplicates ownership of state already owned by the filesystem and UserDefaults.
The cleaner shape is an @Observable model (e.g., AgentHookStatusModel) that caches [AgentHookIntegration: AgentHookIntegrationStatus], loads on startup, and updates on statusDidChangeNotification. The card and rows would read from the model directly, with no token or extra binding needed.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/TerminalNotificationStore.swift (1)
1281-1357:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t evict unrelated panel notifications when adding the hook prompt.
The new
.agentHookSetupnudge goes through the same(tabId, surfaceId)replacement path as real terminal notifications. That means running an agent command on a panel with an unread build/test alert will delete that alert and clear its delivered notification, even though the prompt is unrelated. Dedupe this by action/kind instead of removing every notification on the surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalNotificationStore.swift` around lines 1281 - 1357, The removal currently clears all notifications matching tabId and surfaceId in addNotification, which evicts unrelated alerts; change the removal predicate in the updated.removeAll closure (the block that appends to idsToClear) so it only removes notifications that are the same "kind" as the incoming TerminalNotification/action (compare existing.action to the new action or a distinguishing property like action.kind/identifier) in addition to matching tabId and surfaceId; update the condition that builds idsToClear and the insert logic so unrelated notifications remain while still deduping identical nudges.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Resources/Localizable.xcstrings`:
- Around line 59243-59259: The Agent hooks Settings card alias in
Resources/Localizable.xcstrings is missing "claude"/"claudecode", so add those
tokens to the alias string for that card (the same entry that supplies the
search aliases for the Agent hooks card) so searching "claude" finds the card;
update the alias value to include "claude", "claudecode" (and optionally "claude
code") as comma/space-separated aliases alongside the existing tokens, keeping
the key that is referenced by status.claudeEnabled and status.claudeDisabled
unchanged.
In `@Resources/shell-integration/cmux-zsh-integration.zsh`:
- Around line 1067-1119: The command-parsing function
_cmux_agent_hook_agent_for_command currently skips wrapper tokens like sudo/doas
but doesn't consume their option arguments, so a call like "sudo -u alice
claude" misidentifies "alice" as the command; update the sudo|doas branch in
_cmux_agent_hook_agent_for_command to advance the index to skip any immediate
option payloads: when you detect word matching sudo or doas, increment index as
you already do, then if the next token exists and does NOT begin with '-' (or is
not an assignment containing '=') treat it as the option's argument and
increment index again; also handle the common paired form where a previous token
is an option like '-u' by checking the prior token pattern if needed so tokens
like '-u alice' are consumed together before continuing the main loop. Ensure
these checks operate on the words array and index variable used in the function.
- Around line 1121-1129: The nudge guard currently requires both CMUX_TAB_ID and
CMUX_PANEL_ID which prevents sending tab-only nudges; in
_cmux_report_agent_hook_nudge build the payload starting with "agent_hooks_nudge
$agent --tab=$CMUX_TAB_ID" unconditionally and only append
"--panel=$CMUX_PANEL_ID" if CMUX_PANEL_ID is non-empty, then pass the
constructed string to _cmux_send_bg; keep the existing checks for a valid agent
from _cmux_agent_hook_agent_for_command but remove the strict requirement that
CMUX_PANEL_ID must be set before sending.
In `@Sources/cmuxApp.swift`:
- Around line 7842-7843: The subtitle currently always prefers installMessage
over the live status subtitle causing stale install text to persist; change the
logic to only use installMessage when it is relevant to the currentStatus (e.g.,
an install-related/transient state) and otherwise fall back to
AgentHookIntegrationSettings.statusSubtitle(for: agent, status: currentStatus).
Implement this by replacing the bare "installMessage ??
AgentHookIntegrationSettings.statusSubtitle(...)" with a conditional like
"(shouldShowInstallMessage ? installMessage : nil) ??
AgentHookIntegrationSettings.statusSubtitle(...)", where
shouldShowInstallMessage is computed from installMessage != nil &&
currentStatus.isInstallRelated (add a small computed Bool on the status type for
install-related states); apply the same change to the other occurrence noted in
the diff.
- Around line 7799-7804: Replace the hardcoded searchAnchorID usage in
SettingsCardRow with the typed SettingsSearchIndex helper: remove
searchAnchorID: "automation:agent-hooks" from the SettingsCardRow invocation and
instead apply .settingsSearchAnchor(SettingsSearchIndex.sectionID(for:
<appropriate enum/case>)) on the view; ensure SettingsSearchIndex has a matching
entry for the agent hooks target (add/update the enum/case or mapping used by
sectionID(for:)) so the anchor ID and indexed settings remain consistent with
SettingsSearchIndex.sectionID(for:).
In `@Sources/SettingsNavigation.swift`:
- Line 324: The new automation entry uses sentence case for its default title;
update the title to Title Case by changing the defaultValue from "Agent hooks"
to "Agent Hooks" where the setting is declared (the setting(.automation,
"agent-hooks", String(localized: "settings.automation.agentHooks.title",
defaultValue: "Agent hooks", ...)) in SettingsNavigation.swift) and also update
the corresponding English string in Resources/Localizable.xcstrings
(settings.automation.agentHooks.title) so the stored localization matches the
code change.
In `@Sources/TerminalController.swift`:
- Line 12670: Update the help text for the "agent_hooks_nudge" command to
include the supported alias --surface by amending the existing usage string
"agent_hooks_nudge <agent> --tab=<uuid> --panel=<uuid>" to also show
"--surface=<uuid>" (e.g., "--panel=<uuid> | --surface=<uuid>"). Ensure this text
change corresponds to the same help/usage literal that prints the
agent_hooks_nudge line so callers see the --surface flag that the parser accepts
(the parser already treats --surface as an alias for --panel).
In `@Sources/TerminalNotificationStore.swift`:
- Around line 874-899: The current showSetupPromptIfNeeded uses
AppDelegate.shared?.toggleNotificationsPopover(animated: true), which can
unexpectedly dismiss the popover; change this to a show-only path: either call a
dedicated show method (e.g.
AppDelegate.shared?.showNotificationsPopover(animated: true)) if available, or
guard the toggle by checking visibility (e.g. if
!(AppDelegate.shared?.isNotificationsPopoverVisible ?? false) {
AppDelegate.shared?.toggleNotificationsPopover(animated: true) }) so the
socket-driven agentHooks.nudge cannot steal focus; update
showSetupPromptIfNeeded to use the show-only API or the visibility guard and
ensure you reference AppDelegate.shared and the toggle/show method names
consistently.
- Around line 965-1008: The runInstallCommand function can deadlock because
waitUntilExit() is called before draining stdout/stderr; start asynchronous
reads on stdoutPipe.fileHandleForReading and stderrPipe.fileHandleForReading
(e.g. set readabilityHandler or use readToEndOfFileInBackgroundAndNotify)
immediately after launching the Process so the child cannot block writing,
collect the data into Data objects (or wait for notifications) and only then
call waitUntilExit() (or wait for termination notification), then remove
handlers and convert collected Data to Strings as currently done for
output/errorOutput.
---
Outside diff comments:
In `@Sources/TerminalNotificationStore.swift`:
- Around line 1281-1357: The removal currently clears all notifications matching
tabId and surfaceId in addNotification, which evicts unrelated alerts; change
the removal predicate in the updated.removeAll closure (the block that appends
to idsToClear) so it only removes notifications that are the same "kind" as the
incoming TerminalNotification/action (compare existing.action to the new action
or a distinguishing property like action.kind/identifier) in addition to
matching tabId and surfaceId; update the condition that builds idsToClear and
the insert logic so unrelated notifications remain while still deduping
identical nudges.
🪄 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: 26e0f4ea-3433-4af4-ba25-886eb7fa0389
📒 Files selected for processing (9)
Resources/Localizable.xcstringsResources/shell-integration/cmux-zsh-integration.zshSources/NotificationsPage.swiftSources/SettingsNavigation.swiftSources/SettingsSearchAliases.swiftSources/TerminalController.swiftSources/TerminalNotificationStore.swiftSources/Update/UpdateTitlebarAccessory.swiftSources/cmuxApp.swift
| private func primaryButtonTitle(for agent: AgentHookIntegration) -> String { | ||
| if isRunning { | ||
| return String(localized: "agentHooks.prompt.installing", defaultValue: "Installing...") | ||
| } | ||
| if AgentHookIntegrationSettings.status(for: agent).isUpdateAvailable { | ||
| return String(localized: "agentHooks.prompt.update", defaultValue: "Update hooks") | ||
| } |
There was a problem hiding this comment.
body via primaryButtonTitle
primaryButtonTitle(for:) is called directly from body (line 195) and unconditionally calls AgentHookIntegrationSettings.status(for: agent), which executes FileManager.default.fileExists(atPath:) and try? String(contentsOfFile:encoding:) on the @MainActor during every render pass. This view lives inside a ForEach of notifications in the popover, so any observable change to notificationStore.notifications will re-run body and re-read the hook config files from disk.
The fix follows the same shape needed in AgentHookSettingsRow: cache the status in @State var cachedStatus: AgentHookIntegrationStatus, populate it in .task and in a .onReceive of AgentHookIntegrationSettings.statusDidChangeNotification, and read only the cached value from primaryButtonTitle.
Rule Used: Flag SwiftUI changes that can cause stale state, b... (source)
| } | ||
| } | ||
|
|
||
| enum TerminalNotificationAction: Hashable { | ||
| case agentHookSetup(agentName: String) | ||
| } | ||
|
|
||
| struct AgentHookIntegration: Identifiable, Hashable, Sendable { | ||
| let name: String | ||
| let displayName: String | ||
| let commandNames: [String] | ||
| let configDir: String? | ||
| let configFile: String? | ||
| let configDirEnvOverride: String? | ||
| let hookMarkers: [String] | ||
| let currentMarkers: [String] | ||
| let isClaudeWrapper: Bool | ||
|
|
||
| var id: String { name } | ||
|
|
||
| var installCommand: String { | ||
| if isClaudeWrapper { | ||
| return "cmux settings open --section automation" | ||
| } | ||
| return "cmux hooks \(name) install" | ||
| } | ||
| } | ||
|
|
||
| enum AgentHookIntegrationStatus: Equatable { | ||
| case enabled | ||
| case disabled | ||
| case installed(path: String) | ||
| case updateAvailable(path: String) | ||
| case notInstalled(path: String?) | ||
| case unreadable(path: String) | ||
|
|
||
| var isActive: Bool { | ||
| switch self { | ||
| case .enabled, .installed: | ||
| return true | ||
| case .disabled, .updateAvailable, .notInstalled, .unreadable: | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| var isUpdateAvailable: Bool { | ||
| if case .updateAvailable = self { | ||
| return true | ||
| } | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| struct AgentHookInstallResult { | ||
| let succeeded: Bool | ||
| let message: String | ||
| } | ||
|
|
||
| enum AgentHookIntegrationSettings { | ||
| static let promptEnabledKey = "agentHookSetupPromptEnabled" | ||
| static let defaultPromptEnabled = true | ||
| static let statusDidChangeNotification = Notification.Name("cmux.agentHookIntegration.statusDidChange") | ||
|
|
||
| private static let promptCooldown: TimeInterval = 24 * 60 * 60 | ||
|
|
||
| static let allAgents: [AgentHookIntegration] = [ | ||
| AgentHookIntegration( | ||
| name: "claude", | ||
| displayName: "Claude Code", | ||
| commandNames: ["claude"], | ||
| configDir: nil, | ||
| configFile: nil, | ||
| configDirEnvOverride: nil, | ||
| hookMarkers: [], | ||
| currentMarkers: [], | ||
| isClaudeWrapper: true | ||
| ), | ||
| AgentHookIntegration( | ||
| name: "codex", | ||
| displayName: "Codex", | ||
| commandNames: ["codex"], | ||
| configDir: ".codex", | ||
| configFile: "hooks.json", | ||
| configDirEnvOverride: "CODEX_HOME", | ||
| hookMarkers: ["cmux hooks codex", "cmux codex-hook"], | ||
| currentMarkers: ["CMUX_AGENT_HOOK_VERSION=1"], | ||
| isClaudeWrapper: false | ||
| ), | ||
| AgentHookIntegration( | ||
| name: "opencode", | ||
| displayName: "OpenCode", | ||
| commandNames: ["opencode", "open-code"], | ||
| configDir: ".config/opencode", | ||
| configFile: "plugins/cmux-session.js", | ||
| configDirEnvOverride: "OPENCODE_CONFIG_DIR", | ||
| hookMarkers: ["cmux-opencode-session-plugin-marker", "cmux hooks opencode"], | ||
| currentMarkers: ["cmux-opencode-session-plugin-marker v1"], | ||
| isClaudeWrapper: false | ||
| ), | ||
| AgentHookIntegration( | ||
| name: "cursor", | ||
| displayName: "Cursor", | ||
| commandNames: ["cursor"], | ||
| configDir: ".cursor", | ||
| configFile: "hooks.json", | ||
| configDirEnvOverride: nil, | ||
| hookMarkers: ["cmux hooks cursor"], | ||
| currentMarkers: ["CMUX_AGENT_HOOK_VERSION=1"], | ||
| isClaudeWrapper: false | ||
| ), | ||
| AgentHookIntegration( | ||
| name: "gemini", | ||
| displayName: "Gemini", | ||
| commandNames: ["gemini"], | ||
| configDir: ".gemini", | ||
| configFile: "settings.json", | ||
| configDirEnvOverride: nil, | ||
| hookMarkers: ["cmux hooks gemini"], | ||
| currentMarkers: ["CMUX_AGENT_HOOK_VERSION=1"], | ||
| isClaudeWrapper: false | ||
| ), | ||
| AgentHookIntegration( | ||
| name: "copilot", | ||
| displayName: "Copilot", | ||
| commandNames: ["copilot"], | ||
| configDir: ".copilot", | ||
| configFile: "config.json", | ||
| configDirEnvOverride: nil, | ||
| hookMarkers: ["cmux hooks copilot"], | ||
| currentMarkers: ["CMUX_AGENT_HOOK_VERSION=1"], | ||
| isClaudeWrapper: false | ||
| ), | ||
| AgentHookIntegration( | ||
| name: "codebuddy", | ||
| displayName: "CodeBuddy", | ||
| commandNames: ["codebuddy"], | ||
| configDir: ".codebuddy", | ||
| configFile: "settings.json", | ||
| configDirEnvOverride: nil, | ||
| hookMarkers: ["cmux hooks codebuddy"], | ||
| currentMarkers: ["CMUX_AGENT_HOOK_VERSION=1"], | ||
| isClaudeWrapper: false | ||
| ), | ||
| AgentHookIntegration( | ||
| name: "factory", | ||
| displayName: "Factory", | ||
| commandNames: ["factory"], | ||
| configDir: ".factory", | ||
| configFile: "settings.json", | ||
| configDirEnvOverride: nil, | ||
| hookMarkers: ["cmux hooks factory"], | ||
| currentMarkers: ["CMUX_AGENT_HOOK_VERSION=1"], | ||
| isClaudeWrapper: false | ||
| ), | ||
| AgentHookIntegration( | ||
| name: "qoder", | ||
| displayName: "Qoder", | ||
| commandNames: ["qoder"], | ||
| configDir: ".qoder", | ||
| configFile: "settings.json", | ||
| configDirEnvOverride: nil, | ||
| hookMarkers: ["cmux hooks qoder"], | ||
| currentMarkers: ["CMUX_AGENT_HOOK_VERSION=1"], | ||
| isClaudeWrapper: false | ||
| ), | ||
| ] | ||
|
|
||
| static func promptEnabled(defaults: UserDefaults = .standard) -> Bool { | ||
| if defaults.object(forKey: promptEnabledKey) == nil { | ||
| return defaultPromptEnabled | ||
| } | ||
| return defaults.bool(forKey: promptEnabledKey) | ||
| } | ||
|
|
||
| static func setPromptEnabled(_ enabled: Bool, defaults: UserDefaults = .standard) { | ||
| defaults.set(enabled, forKey: promptEnabledKey) | ||
| NotificationCenter.default.post(name: statusDidChangeNotification, object: nil) | ||
| } | ||
|
|
||
| static func agent(named name: String) -> AgentHookIntegration? { | ||
| let normalized = name.trimmingCharacters(in: .whitespacesAndNewlines).lowercased() | ||
| return allAgents.first { agent in | ||
| agent.name == normalized || agent.commandNames.contains(normalized) | ||
| } | ||
| } | ||
|
|
||
| static func status(for agent: AgentHookIntegration, defaults: UserDefaults = .standard) -> AgentHookIntegrationStatus { | ||
| if agent.isClaudeWrapper { | ||
| return ClaudeCodeIntegrationSettings.hooksEnabled(defaults: defaults) ? .enabled : .disabled | ||
| } | ||
|
|
||
| guard let path = configFilePath(for: agent) else { | ||
| return .notInstalled(path: nil) | ||
| } | ||
|
|
||
| guard FileManager.default.fileExists(atPath: path) else { | ||
| return .notInstalled(path: path) | ||
| } | ||
| guard let contents = try? String(contentsOfFile: path, encoding: .utf8) else { | ||
| return .unreadable(path: path) | ||
| } | ||
| if agent.currentMarkers.contains(where: { contents.contains($0) }) { | ||
| return .installed(path: path) | ||
| } | ||
| if agent.hookMarkers.contains(where: { contents.contains($0) }) { | ||
| return .updateAvailable(path: path) | ||
| } | ||
| return .notInstalled(path: path) | ||
| } | ||
|
|
||
| static func statusLabel(for status: AgentHookIntegrationStatus) -> String { | ||
| switch status { | ||
| case .enabled: | ||
| return String(localized: "settings.automation.agentHooks.status.enabled", defaultValue: "Enabled") | ||
| case .disabled: | ||
| return String(localized: "settings.automation.agentHooks.status.disabled", defaultValue: "Disabled") | ||
| case .installed: | ||
| return String(localized: "settings.automation.agentHooks.status.installed", defaultValue: "Installed") | ||
| case .updateAvailable: | ||
| return String(localized: "settings.automation.agentHooks.status.updateAvailable", defaultValue: "Update available") | ||
| case .notInstalled: | ||
| return String(localized: "settings.automation.agentHooks.status.notInstalled", defaultValue: "Not installed") | ||
| case .unreadable: | ||
| return String(localized: "settings.automation.agentHooks.status.unknown", defaultValue: "Unknown") | ||
| } | ||
| } | ||
|
|
||
| static func statusSubtitle(for agent: AgentHookIntegration, status: AgentHookIntegrationStatus) -> String { | ||
| switch status { | ||
| case .enabled: | ||
| return String(localized: "settings.automation.agentHooks.status.claudeEnabled", defaultValue: "cmux wraps the claude command in cmux terminals.") | ||
| case .disabled: | ||
| return String(localized: "settings.automation.agentHooks.status.claudeDisabled", defaultValue: "Claude Code runs without cmux hooks.") | ||
| case .installed(let path): | ||
| return String(localized: "settings.automation.agentHooks.status.installedAt", defaultValue: "cmux hooks found in \(path).") | ||
| case .updateAvailable: | ||
| return String(localized: "settings.automation.agentHooks.status.updateAvailable.subtitle", defaultValue: "cmux hooks are installed, but this app has a newer hook script.") | ||
| case .notInstalled: | ||
| return String(localized: "settings.automation.agentHooks.status.notInstalled.subtitle", defaultValue: "No cmux hooks found.") | ||
| case .unreadable(let path): | ||
| return String(localized: "settings.automation.agentHooks.status.unreadable", defaultValue: "Could not read \(path).") | ||
| } | ||
| } | ||
|
|
||
| @MainActor | ||
| static func showSetupPromptIfNeeded(agentName: String, tabId: UUID, surfaceId: UUID?) { | ||
| guard promptEnabled(), | ||
| let agent = agent(named: agentName) else { | ||
| return | ||
| } | ||
| let currentStatus = status(for: agent) | ||
| guard !currentStatus.isActive else { | ||
| return | ||
| } | ||
| guard shouldShowPrompt(for: agent) else { | ||
| return | ||
| } | ||
|
|
||
| markPromptShown(for: agent) | ||
| TerminalNotificationStore.shared.addNotification( | ||
| tabId: tabId, | ||
| surfaceId: surfaceId, | ||
| title: currentStatus.isUpdateAvailable | ||
| ? String(localized: "agentHooks.nudge.updateTitle", defaultValue: "Update \(agent.displayName) hooks") | ||
| : String(localized: "agentHooks.nudge.title", defaultValue: "Install \(agent.displayName) hooks"), | ||
| subtitle: String(localized: "agentHooks.nudge.subtitle", defaultValue: "Notifications and session restore"), | ||
| body: currentStatus.isUpdateAvailable | ||
| ? String(localized: "agentHooks.nudge.updateBody", defaultValue: "cmux has a newer hook script for notifications and session restore.") | ||
| : String(localized: "agentHooks.nudge.body", defaultValue: "Hooks let cmux show agent notifications and restore sessions after cmux restarts."), | ||
| cooldownKey: "agent-hooks-setup-\(agent.name)", | ||
| cooldownInterval: promptCooldown, | ||
| action: .agentHookSetup(agentName: agent.name) | ||
| ) | ||
| AppDelegate.shared?.toggleNotificationsPopover(animated: true) | ||
| } | ||
|
|
||
| static func installHooks(for agent: AgentHookIntegration, completion: @escaping (AgentHookInstallResult) -> Void) { | ||
| if agent.isClaudeWrapper { | ||
| UserDefaults.standard.set(true, forKey: ClaudeCodeIntegrationSettings.hooksEnabledKey) | ||
| NotificationCenter.default.post(name: statusDidChangeNotification, object: nil) | ||
| completion(AgentHookInstallResult( | ||
| succeeded: true, | ||
| message: String(localized: "settings.automation.agentHooks.status.claudeEnabled", defaultValue: "cmux wraps the claude command in cmux terminals.") | ||
| )) | ||
| return | ||
| } | ||
|
|
||
| let launch = hookInstallLaunch(for: agent) | ||
| DispatchQueue.global(qos: .userInitiated).async { | ||
| let result = runInstallCommand( | ||
| executableURL: launch.executableURL, | ||
| arguments: launch.arguments, | ||
| fallbackCommand: agent.installCommand | ||
| ) | ||
| DispatchQueue.main.async { | ||
| NotificationCenter.default.post(name: statusDidChangeNotification, object: nil) | ||
| completion(result) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private static func configFilePath(for agent: AgentHookIntegration) -> String? { | ||
| guard let configDir = agent.configDir, | ||
| let configFile = agent.configFile else { | ||
| return nil | ||
| } | ||
| let directory: String | ||
| if let envKey = agent.configDirEnvOverride, | ||
| let envValue = ProcessInfo.processInfo.environment[envKey], | ||
| !envValue.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty { | ||
| directory = NSString(string: envValue).expandingTildeInPath | ||
| } else { | ||
| directory = NSString(string: "~/\(configDir)").expandingTildeInPath | ||
| } | ||
| return (directory as NSString).appendingPathComponent(configFile) | ||
| } | ||
|
|
||
| private static func shouldShowPrompt(for agent: AgentHookIntegration, defaults: UserDefaults = .standard) -> Bool { | ||
| let key = lastPromptKey(for: agent) | ||
| let lastShown = defaults.double(forKey: key) | ||
| guard lastShown > 0 else { return true } | ||
| return Date().timeIntervalSince1970 - lastShown >= promptCooldown | ||
| } | ||
|
|
||
| private static func markPromptShown(for agent: AgentHookIntegration, defaults: UserDefaults = .standard) { | ||
| defaults.set(Date().timeIntervalSince1970, forKey: lastPromptKey(for: agent)) | ||
| } | ||
|
|
||
| private static func lastPromptKey(for agent: AgentHookIntegration) -> String { | ||
| "agentHookSetupPromptLastShown.\(agent.name)" | ||
| } | ||
|
|
||
| private static func hookInstallLaunch(for agent: AgentHookIntegration) -> (executableURL: URL, arguments: [String]) { | ||
| if let bundledCLIURL = Bundle.main.resourceURL?.appendingPathComponent("bin/cmux"), | ||
| FileManager.default.isExecutableFile(atPath: bundledCLIURL.path) { | ||
| return (bundledCLIURL, ["hooks", agent.name, "install", "--yes"]) | ||
| } | ||
| return (URL(fileURLWithPath: "/usr/bin/env"), ["cmux", "hooks", agent.name, "install", "--yes"]) | ||
| } | ||
|
|
||
| private static func runInstallCommand( | ||
| executableURL: URL, | ||
| arguments: [String], | ||
| fallbackCommand: String | ||
| ) -> AgentHookInstallResult { | ||
| let process = Process() | ||
| process.executableURL = executableURL | ||
| process.arguments = arguments | ||
| process.standardInput = FileHandle.nullDevice | ||
| let stdoutPipe = Pipe() | ||
| let stderrPipe = Pipe() | ||
| process.standardOutput = stdoutPipe | ||
| process.standardError = stderrPipe | ||
|
|
||
| do { | ||
| try process.run() | ||
| process.waitUntilExit() | ||
| } catch { | ||
| return AgentHookInstallResult( | ||
| succeeded: false, | ||
| message: String(localized: "agentHooks.prompt.installFailed", defaultValue: "Could not install hooks. Run \(fallbackCommand) in a terminal.") | ||
| ) | ||
| } | ||
|
|
||
| let outputData = stdoutPipe.fileHandleForReading.readDataToEndOfFile() | ||
| let errorData = stderrPipe.fileHandleForReading.readDataToEndOfFile() | ||
| let output = String(data: outputData, encoding: .utf8)?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "" | ||
| let errorOutput = String(data: errorData, encoding: .utf8)?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "" | ||
|
|
||
| guard process.terminationStatus == 0 else { | ||
| let detail = errorOutput.isEmpty ? output : errorOutput | ||
| if detail.isEmpty { | ||
| return AgentHookInstallResult( | ||
| succeeded: false, | ||
| message: String(localized: "agentHooks.prompt.installFailed", defaultValue: "Could not install hooks. Run \(fallbackCommand) in a terminal.") | ||
| ) | ||
| } | ||
| return AgentHookInstallResult(succeeded: false, message: detail) | ||
| } | ||
|
|
||
| return AgentHookInstallResult( | ||
| succeeded: true, | ||
| message: String(localized: "agentHooks.prompt.installSucceeded", defaultValue: "Hooks installed.") | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| struct TerminalNotification: Identifiable, Hashable { | ||
| let id: UUID | ||
| let tabId: UUID | ||
| let surfaceId: UUID? |
There was a problem hiding this comment.
TerminalNotificationStore.swift now stands at 1,863 lines after this PR added 390 lines — well over both the 800-line and the 250-added-line thresholds in the file-package-boundaries rule. The additions mix multiple orthogonal responsibilities into a file that was already a notification store:
- Domain model types (
AgentHookIntegration,AgentHookIntegrationStatus,AgentHookInstallResult) - File-system inspection and config-path resolution
- Subprocess spawning (
runInstallCommand/process.waitUntilExit()) - UserDefaults read/write and cooldown tracking
- UI label strings (
statusLabel,statusSubtitle) - Notification scheduling and popover triggering
AgentHookIntegrationSettings and its supporting types are independently testable without AppKit, SwiftUI view state, or cmux app singletons. The smallest extraction cut is a new AgentHookIntegration SwiftPM package target (or at minimum a standalone AgentHookIntegration.swift file) that owns the model, status, install logic, and settings persistence, leaving only the thin showSetupPromptIfNeeded bridge in the app target.
Rule Used: Flag Swift changes that add too much unrelated res... (source)
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
Sources/TerminalNotificationStore.swift (2)
899-929:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a show-only popover path here instead of toggling.
This socket-driven nudge should not be able to dismiss the notifications popover when it is already open, and
toggleNotificationsPopover(...)does exactly that. Please gate on visibility or call a dedicated show API here.As per coding guidelines, "Socket/CLI commands must not steal macOS app focus."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalNotificationStore.swift` around lines 899 - 929, The call in showSetupPromptIfNeeded currently uses AppDelegate.shared?.toggleNotificationsPopover(...) which can dismiss the notifications popover if it is already visible; change this to use a show-only path (e.g. a new or existing AppDelegate method like showNotificationsPopover(animated:)) or first check a visibility flag before calling toggle to only show when hidden. Update showSetupPromptIfNeeded to call the show-only API (or gate on a visible property) instead of toggleNotificationsPopover, and ensure references to TerminalNotificationStore.shared.addNotification and the cooldown/action logic remain unchanged.
995-1038:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDrain child output before waiting for exit.
waitUntilExit()happens before either pipe is read. Ifcmux hooks ... installwrites enough stdout/stderr to fill the pipe buffer, the child blocks and this task never completes, leaving the action stuck in the installing state.💡 Suggested fix
- let stdoutPipe = Pipe() - let stderrPipe = Pipe() - process.standardOutput = stdoutPipe - process.standardError = stderrPipe + let outputPipe = Pipe() + process.standardOutput = outputPipe + process.standardError = outputPipe do { try process.run() - process.waitUntilExit() } catch { return AgentHookInstallResult( succeeded: false, message: String(localized: "agentHooks.prompt.installFailed", defaultValue: "Could not install hooks. Run \(fallbackCommand) in a terminal.") ) } - let outputData = stdoutPipe.fileHandleForReading.readDataToEndOfFile() - let errorData = stderrPipe.fileHandleForReading.readDataToEndOfFile() - let output = String(data: outputData, encoding: .utf8)?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "" - let errorOutput = String(data: errorData, encoding: .utf8)?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "" + let outputData = outputPipe.fileHandleForReading.readDataToEndOfFile() + process.waitUntilExit() + let output = String(data: outputData, encoding: .utf8)? + .trimmingCharacters(in: .whitespacesAndNewlines) ?? "" guard process.terminationStatus == 0 else { - let detail = errorOutput.isEmpty ? output : errorOutput + let detail = output🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalNotificationStore.swift` around lines 995 - 1038, runInstallCommand currently calls process.waitUntilExit() before reading stdoutPipe and stderrPipe, which can deadlock if the child fills the pipe; fix it by starting background reads on stdoutPipe.fileHandleForReading and stderrPipe.fileHandleForReading (e.g. set readabilityHandler or use readToEndOfFileInBackgroundAndNotify and collect Data) immediately after setting process.standardOutput/standardError and before calling try process.run(), then call process.waitUntilExit(), stop the handlers, and use the collected Data for outputData/errorData (references: runInstallCommand, process, stdoutPipe, stderrPipe, outputData, errorData).
🤖 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 16348-16356: Reject directory-valued CMUX_BUNDLED_CLI_PATH values
by ensuring the bundled-path branch checks that CMUX_BUNDLED_CLI_PATH is a
regular file and executable (e.g. replace/improve occurrences of [ -x
"$CMUX_BUNDLED_CLI_PATH" ] with a check like [ -f "$CMUX_BUNDLED_CLI_PATH" ] &&
[ -x "$CMUX_BUNDLED_CLI_PATH" ] or [ -x "$CMUX_BUNDLED_CLI_PATH" ] && [ ! -d
"$CMUX_BUNDLED_CLI_PATH" ]), updating both the feedHookCommand string and the
other hook-dispatch string shown in the diff so directories are not treated as
valid executables.
- Line 16543: The assignment to the cmux constant currently prefers
process.env.CMUX_BUNDLED_CLI_PATH without validating it; implement a small
helper (e.g., firstExecutableFile) that iterates candidate paths
(process.env.CMUX_OPENCODE_CMUX_BIN, process.env.CMUX_BUNDLED_CLI_PATH) and
returns the first that exists, is not a directory, and is executable (use
fs.statSync + fs.accessSync with fs.constants.X_OK), then set cmux to that
helper's result or fallback to the plain "cmux" string; update the code that
constructs cmux to call this helper so spawnSync only ever receives a real
executable file path.
In `@Resources/Localizable.xcstrings`:
- Around line 59243-59256: Update the alias entry
"settings.search.alias.setting.automation.agent-hooks" so the English and
Japanese "value" strings include the hyphenated and spaced variants for
opencode: add both "open-code" and "open code" (in addition to the existing
"opencode"); ensure the same additions appear in the ja localization string
value as well so searches match hyphenated and spaced forms for both languages.
In `@Sources/cmuxApp.swift`:
- Around line 7791-7916: The file is too large; extract the three view types
AgentHooksSettingsCard, AgentHookSettingsRow, and AgentHookStatusPill into a new
Swift source file to reduce Sources/cmuxApp.swift size. Create a new file (e.g.,
AgentHooksViews.swift) that imports SwiftUI and contains these three private
structs unchanged (keep their usages of AgentHookIntegrationSettings,
AgentHookIntegration, AgentHookIntegrationStatus, and NotificationCenter as-is),
then remove the duplicated definitions from Sources/cmuxApp.swift; ensure the
new file is added to the target so references to AgentHooksSettingsCard,
AgentHookSettingsRow, and AgentHookStatusPill still compile.
- Around line 7805-7809: The Toggle is currently bound directly to the
`@AppStorage-backed` promptEnabled which bypasses AgentHookIntegrationSettings'
notification logic; change the binding so setting the toggle calls
AgentHookIntegrationSettings.setPromptEnabled(_:) instead of mutating
promptEnabled directly (e.g., replace use of Toggle(..., isOn: $promptEnabled)
with a Binding that reads promptEnabled but calls
AgentHookIntegrationSettings.setPromptEnabled(newValue) in its setter) so
statusDidChangeNotification is emitted for subscribers.
In `@Sources/TerminalNotificationStore.swift`:
- Around line 841-863: The synchronous file I/O in status(for:
AgentHookIntegration, defaults:) blocks the main actor; change it to perform
fileExists/String(contentsOfFile:) off-main and return/update a cached snapshot
for UI use: add an async/background helper (e.g. statusAsync(for:defaults:)
using Task.detached or DispatchQueue.global) that calls configFilePath(for:),
performs file reads off-main, computes AgentHookIntegrationStatus, and stores it
in a lightweight cache; update callers (UI/@MainActor paths and
TerminalNotificationActionButtons.primaryButtonTitle) to read the cached
snapshot synchronously and subscribe to updates rather than calling the blocking
status(for:), or provide a non-blocking statusCached(for:) that returns the last
computed status while the async helper refreshes in background. Ensure unique
symbols touched: status(for:), configFilePath(for:), AgentHookIntegrationStatus,
and any cache getter used by the UI.
- Around line 662-1040: The file is too large because the entire agent-hooks
domain (types and install logic) lives inside TerminalNotificationStore.swift;
extract all agent-hooks domain code—specifically AgentHookIntegration,
AgentHookIntegrationStatus, AgentHookInstallResult, and the
AgentHookIntegrationSettings namespace including methods configFilePath(_:),
hookInstallLaunch(_:),
runInstallCommand(executableURL:arguments:fallbackCommand:),
installHooks(for:completion:), status(for:defaults:), statusLabel(_:),
statusSubtitle(for:status:), agent(named:), promptEnabled/setPromptEnabled, and
related helpers (lastPromptKey(_:), shouldShowPrompt(_:),
markPromptShown(_:))—into a new Swift module or file (SwiftPM package target)
that has no AppKit/SwiftUI/App singletons; keep only the minimal prompt-enqueue
bridge in TerminalNotificationStore.swift (e.g., a small call to
AgentHookIntegrationSettings.showSetupPromptIfNeeded or an adapter) and update
call sites to import the new module, move Notification.Name
statusDidChangeNotification with it, and expose any APIs needed by the UI
(ensure public/internal access levels and update Bundle/ProcessInfo/FileManager
references accordingly).
---
Duplicate comments:
In `@Sources/TerminalNotificationStore.swift`:
- Around line 899-929: The call in showSetupPromptIfNeeded currently uses
AppDelegate.shared?.toggleNotificationsPopover(...) which can dismiss the
notifications popover if it is already visible; change this to use a show-only
path (e.g. a new or existing AppDelegate method like
showNotificationsPopover(animated:)) or first check a visibility flag before
calling toggle to only show when hidden. Update showSetupPromptIfNeeded to call
the show-only API (or gate on a visible property) instead of
toggleNotificationsPopover, and ensure references to
TerminalNotificationStore.shared.addNotification and the cooldown/action logic
remain unchanged.
- Around line 995-1038: runInstallCommand currently calls
process.waitUntilExit() before reading stdoutPipe and stderrPipe, which can
deadlock if the child fills the pipe; fix it by starting background reads on
stdoutPipe.fileHandleForReading and stderrPipe.fileHandleForReading (e.g. set
readabilityHandler or use readToEndOfFileInBackgroundAndNotify and collect Data)
immediately after setting process.standardOutput/standardError and before
calling try process.run(), then call process.waitUntilExit(), stop the handlers,
and use the collected Data for outputData/errorData (references:
runInstallCommand, process, stdoutPipe, stderrPipe, outputData, errorData).
🪄 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: 19e39147-9560-4cc1-bcc0-24d73de49a70
📒 Files selected for processing (5)
CLI/cmux.swiftResources/Localizable.xcstringsSources/NotificationsPage.swiftSources/TerminalNotificationStore.swiftSources/cmuxApp.swift
| static func status(for agent: AgentHookIntegration, defaults: UserDefaults = .standard) -> AgentHookIntegrationStatus { | ||
| if agent.isClaudeWrapper { | ||
| return ClaudeCodeIntegrationSettings.hooksEnabled(defaults: defaults) ? .enabled : .disabled | ||
| } | ||
|
|
||
| guard let path = configFilePath(for: agent) else { | ||
| return .notInstalled(path: nil) | ||
| } | ||
|
|
||
| guard FileManager.default.fileExists(atPath: path) else { | ||
| return .notInstalled(path: path) | ||
| } | ||
| guard let contents = try? String(contentsOfFile: path, encoding: .utf8) else { | ||
| return .unreadable(path: path) | ||
| } | ||
| if agent.currentMarkers.contains(where: { contents.contains($0) }) { | ||
| return .installed(path: path) | ||
| } | ||
| if agent.hookMarkers.contains(where: { contents.contains($0) }) { | ||
| return .updateAvailable(path: path) | ||
| } | ||
| return .notInstalled(path: path) | ||
| } |
There was a problem hiding this comment.
Move hook-status file reads off hot UI/socket paths.
status(for:) does synchronous fileExists / String(contentsOfFile:) work, and this API is now used from the @MainActor nudge path and from TerminalNotificationActionButtons.primaryButtonTitle(...) during SwiftUI body recomputation. A slow home directory or large config will stall both socket handling and list rendering; compute/cache the status off-main and pass a snapshot into the UI instead.
As per coding guidelines, "If adding new socket command, default to off-main handling."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TerminalNotificationStore.swift` around lines 841 - 863, The
synchronous file I/O in status(for: AgentHookIntegration, defaults:) blocks the
main actor; change it to perform fileExists/String(contentsOfFile:) off-main and
return/update a cached snapshot for UI use: add an async/background helper (e.g.
statusAsync(for:defaults:) using Task.detached or DispatchQueue.global) that
calls configFilePath(for:), performs file reads off-main, computes
AgentHookIntegrationStatus, and stores it in a lightweight cache; update callers
(UI/@MainActor paths and TerminalNotificationActionButtons.primaryButtonTitle)
to read the cached snapshot synchronously and subscribe to updates rather than
calling the blocking status(for:), or provide a non-blocking statusCached(for:)
that returns the last computed status while the async helper refreshes in
background. Ensure unique symbols touched: status(for:), configFilePath(for:),
AgentHookIntegrationStatus, and any cache getter used by the UI.
| @MainActor | ||
| static func showSetupPromptIfNeeded(agentName: String, tabId: UUID, surfaceId: UUID?) { | ||
| guard promptEnabled(), | ||
| let agent = agent(named: agentName) else { | ||
| return | ||
| } | ||
| let currentStatus = status(for: agent) | ||
| guard !currentStatus.isActive else { | ||
| return | ||
| } | ||
| guard shouldShowPrompt(for: agent, status: currentStatus) else { | ||
| return | ||
| } | ||
|
|
||
| markPromptShown(for: agent, status: currentStatus) | ||
| TerminalNotificationStore.shared.addNotification( | ||
| tabId: tabId, | ||
| surfaceId: surfaceId, | ||
| title: currentStatus.isUpdateAvailable | ||
| ? String(localized: "agentHooks.nudge.updateTitle", defaultValue: "Update \(agent.displayName) hooks") | ||
| : String(localized: "agentHooks.nudge.title", defaultValue: "Install \(agent.displayName) hooks"), | ||
| subtitle: String(localized: "agentHooks.nudge.subtitle", defaultValue: "Notifications and session restore"), | ||
| body: currentStatus.isUpdateAvailable | ||
| ? String(localized: "agentHooks.nudge.updateBody", defaultValue: "cmux has a newer hook script for notifications and session restore.") | ||
| : String(localized: "agentHooks.nudge.body", defaultValue: "Hooks let cmux show agent notifications and restore sessions after cmux restarts."), | ||
| cooldownKey: "agent-hooks-setup-\(agent.name)", | ||
| cooldownInterval: promptCooldown, | ||
| action: .agentHookSetup(agentName: agent.name) | ||
| ) | ||
| AppDelegate.shared?.toggleNotificationsPopover(animated: true) |
There was a problem hiding this comment.
@MainActor in showSetupPromptIfNeeded
status(for: agent) is called unconditionally on the main actor before the !currentStatus.isActive guard. It performs FileManager.default.fileExists(atPath:) and String(contentsOfFile:encoding:) synchronously. This method is triggered via TerminalMutationBus.shared.enqueueMainActorMutation on every preexec event for any of the nine registered agent commands — meaning repeated hook-config file reads block the main thread every time the user runs cursor, claude, codex, etc.
The fix is to move status(for:) off the main actor — either make showSetupPromptIfNeeded nonisolated async and await a background-isolated read, or cache the last-known status in the store and consult the cache here rather than reading from disk inline.
Rule Used: Flag incorrect or missing use of Swift @Concurrent... (source)
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 `@Sources/TerminalNotificationStore.swift`:
- Around line 996-1002: hookInstallLaunch currently prefers Bundle.main/bin/cmux
then falls back to /usr/bin/env cmux, but it should honor the
CMUX_BUNDLED_CLI_PATH environment variable first; update the
hookInstallLaunch(for:) function to check
ProcessInfo.processInfo.environment["CMUX_BUNDLED_CLI_PATH"] and if that path
points to an executable use it (returning its URL and the same ["hooks",
agent.name, "install", "--yes"] args), otherwise continue to check the bundle
path and finally /usr/bin/env as the last fallback.
- Around line 877-878: The .unreadable enum branch currently returns the generic
"settings.automation.agentHooks.status.unknown" string; change it to a distinct
key like "settings.automation.agentHooks.status.unreadable" in
TerminalNotificationStore.swift (the case .unreadable return) and update the
defaultValue to a descriptive label such as "Unreadable" so the Settings card
can distinguish permission/read failures from unknown; also add the matching
"settings.automation.agentHooks.status.unreadable" entry to
Resources/Localizable.xcstrings with the localized translation(s).
- Around line 924-925: The cooldown key used when calling addNotification
currently uses a single per-agent key ("agent-hooks-setup-\(agent.name)"), which
causes install nudges to suppress update nudges; update addNotification to pass
a status-specific cooldown key (e.g. reuse the same logic as
lastPromptKey/shouldShowPrompt/markPromptShown) by including the prompt status
(install vs update) in the key generation and replacing cooldownKey:
"agent-hooks-setup-\(agent.name)" with that status-specific key so
addNotification, shouldShowPrompt, and markPromptShown all use the same
uniquely-identifying key for each prompt type.
🪄 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: e3c6a30f-bd00-432c-9a6b-8b75f8f24486
📒 Files selected for processing (1)
Sources/TerminalNotificationStore.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 (1)
Resources/shell-integration/cmux-zsh-integration.zsh (1)
79-94: 🧹 Nitpick | 🔵 Trivial | 💤 Low value
_cmux_cli_rpc_bgduplicates_cmux_relay_rpc_bg's body; delegate instead.The two functions share identical execution logic — the only difference is
_cmux_relay_rpc_bgadds an upfront_cmux_socket_uses_remote_relayguard. Collapse the duplication by having_cmux_relay_rpc_bgdelegate to_cmux_cli_rpc_bg:♻️ Proposed refactor
_cmux_relay_rpc_bg() { local method="$1" local params="$2" - local relay_cli="" _cmux_socket_uses_remote_relay || return 1 - relay_cli="$(_cmux_relay_cli_path)" || return 1 - { "$relay_cli" rpc "$method" "$params" >/dev/null 2>&1 || true } >/dev/null 2>&1 &! + _cmux_cli_rpc_bg "$method" "$params" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resources/shell-integration/cmux-zsh-integration.zsh` around lines 79 - 94, Collapse the duplicated logic by making _cmux_relay_rpc_bg perform only the _cmux_socket_uses_remote_relay guard and then delegate to _cmux_cli_rpc_bg with the same arguments; keep _cmux_cli_rpc_bg as the single implementation of the relay CLI invocation (the brace+background invocation and silent redirection), and ensure _cmux_relay_rpc_bg preserves the guard's return behavior (return 1 if the guard fails) and returns the exit status from calling _cmux_cli_rpc_bg.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Resources/shell-integration/cmux-zsh-integration.zsh`:
- Around line 1129-1131: The function _cmux_report_agent_hook_nudge currently
bails out early due to the _cmux_socket_is_unix guard which prevents calling the
transport-agnostic _cmux_cli_rpc_bg in remote relay sessions; remove or relocate
the `_cmux_socket_is_unix || return 0` check so that _cmux_cli_rpc_bg is invoked
regardless of socket type, and instead apply the `_cmux_socket_is_unix` guard
only around the socket-based fallback branch (the code path that actually uses
Unix-socket-specific logic) to preserve the original Unix-only fallback
behavior.
In `@Sources/TerminalController.swift`:
- Around line 7896-7913: The file exceeds the Swift length budget because two
new handlers and routing pushed it over the limit; extract the offending
handlers into a separate file/extension to shrink this file. Move the functions
v2AgentHooksNudge and agentHooksNudge (and any tiny routing helper added for
them) into a new extension (e.g., TerminalController extension) or a dedicated
helper file, keeping their signatures and access to TerminalMutationBus.shared
and AgentHookIntegrationSettings.showSetupPromptIfNeeded intact; alternatively
extract just the shared dispatch logic that calls
AgentHookIntegrationSettings.showSetupPromptIfNeeded into a small helper
function in a new file and have both handlers call that helper. Ensure you
remove the originals from this large file and update any references so
compilation remains unchanged.
- Around line 7900-7902: The guard that extracts tabId with v2UUID(params,
"workspace_id") ?? v2UUID(params, "tab_id") accepts either workspace_id or
tab_id but returns an error referencing only workspace_id; update the error
returned from that guard (the .err call) to mention both possible keys (e.g.
"Missing workspace_id or tab_id") and set the data payload's "field" to reflect
both keys (e.g. "workspace_id or tab_id") so callers using tab_id get an
accurate diagnostic; modify the return in the guard that currently references
workspace_id to use the combined message and data while keeping the existing
.err structure.
---
Outside diff comments:
In `@Resources/shell-integration/cmux-zsh-integration.zsh`:
- Around line 79-94: Collapse the duplicated logic by making _cmux_relay_rpc_bg
perform only the _cmux_socket_uses_remote_relay guard and then delegate to
_cmux_cli_rpc_bg with the same arguments; keep _cmux_cli_rpc_bg as the single
implementation of the relay CLI invocation (the brace+background invocation and
silent redirection), and ensure _cmux_relay_rpc_bg preserves the guard's return
behavior (return 1 if the guard fails) and returns the exit status from calling
_cmux_cli_rpc_bg.
🪄 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: fe2d2a10-e027-4263-a339-834126cc531c
📒 Files selected for processing (2)
Resources/shell-integration/cmux-zsh-integration.zshSources/TerminalController.swift
Greptile SummaryThis PR adds agent hook setup nudges for nine CLI agents (Claude Code, Codex, OpenCode, Cursor, Gemini, Copilot, CodeBuddy, Factory, Qoder). A zsh Several issues from previous review rounds are resolved: Confidence Score: 4/5Safe to merge with the open concurrency and HOME-resolution concerns tracked — none cause data loss or broken installs for typical users. The biggest structural concerns from prior rounds (disk I/O on @mainactor in body, popover toggle closing an open popover, missing env-override in diff baseline) have all been fixed. The new inline comments are about diffHooks sharing the legacy completion pattern with the already-flagged installHooks, the app-side configDirectoryPath not respecting $HOME overrides (affects non-standard home setups), and a dead code fallback — none of these break installs or cause incorrect data for typical users. Sources/AgentHookIntegrationSettings.swift (diffHooks legacy pattern and configDirectoryPath HOME inconsistency) and Sources/AgentHookIntegrationDiffing.swift (dead expandedHomePath fallback). Important Files Changed
Sequence DiagramsequenceDiagram
participant ZSH as zsh preexec
participant CLI as cmux CLI (RPC)
participant TC as TerminalController
participant Bus as TerminalMutationBus
participant Settings as AgentHookIntegrationSettings
participant Store as TerminalNotificationStore
participant UI as NotificationsPopover
ZSH->>CLI: agent_hooks.nudge {agent, workspace_id}
CLI->>TC: v2AgentHooksNudge(params)
TC->>Bus: enqueueMainActorMutation
Bus->>Settings: showSetupPromptIfNeeded(agentName, tabId)
Settings->>Settings: guard promptEnabled and no existing notification
Settings->>Settings: Task.detached status(for agent)
Note over Settings: reads config file off @MainActor
Settings-->>Settings: status = .notInstalled or .updateAvailable
Settings->>Store: addNotification(.agentHookSetup)
Settings->>UI: showNotificationsPopover()
Note over UI: opens only if not already shown
UI->>UI: TerminalNotificationActionButtons
UI->>Settings: AgentHookDiffReviewView sheet
Settings->>Settings: DispatchQueue.global buildHookDiff
Settings-->>UI: diff result via completion on main
UI->>Settings: installHooks(for agent) completion
Settings->>Settings: DispatchQueue.global runInstallCommand
Settings-->>Store: statusDidChangeNotification
Store-->>UI: refreshToken increments then refreshStatus()
Reviews (12): Last reviewed commit: "Fix agent hook review and Cursor detecti..." | Re-trigger Greptile |
| private var title: String { | ||
| if AgentHookIntegrationSettings.status(for: agent).isUpdateAvailable { | ||
| return String(localized: "agentHooks.diff.updateTitle", defaultValue: "Update \(agent.displayName) hooks") | ||
| } | ||
| return String(localized: "agentHooks.diff.installTitle", defaultValue: "Install \(agent.displayName) hooks") | ||
| } | ||
|
|
||
| private var installButtonTitle: String { | ||
| if isInstalling { | ||
| return String(localized: "agentHooks.prompt.installing", defaultValue: "Installing...") | ||
| } | ||
| if AgentHookIntegrationSettings.status(for: agent).isUpdateAvailable { | ||
| return String(localized: "agentHooks.prompt.update", defaultValue: "Update hooks") | ||
| } | ||
| if agent.isClaudeWrapper { | ||
| return String(localized: "agentHooks.prompt.enable", defaultValue: "Enable") | ||
| } | ||
| return String(localized: "agentHooks.prompt.install", defaultValue: "Install hooks") | ||
| } |
There was a problem hiding this comment.
body via title and installButtonTitle
AgentHookDiffReviewView.title (line 301) and installButtonTitle (line 311) each call AgentHookIntegrationSettings.status(for: agent) — which does FileManager.default.fileExists and String(contentsOfFile:encoding:) synchronously — from computed properties evaluated during every SwiftUI body pass on @MainActor. The sheet re-renders whenever isInstalling changes (two state writes: set to true, then false after completion), triggering two blocking file reads per install attempt. The same pattern flagged in AgentHookSettingsRow and TerminalNotificationActionButtons applies here. Cache the status in @State var cachedStatus, populate it in .task and .onReceive(AgentHookIntegrationSettings.statusDidChangeNotification), and read only the cached value from these properties.
Rule Used: Flag SwiftUI changes that can cause stale state, b... (source)
| let originalConfigDir = expandedHomePath(configDir) | ||
| let tempConfigDir = tempHome.appendingPathComponent(configDir, isDirectory: true) | ||
| try fm.createDirectory(at: tempConfigDir.deletingLastPathComponent(), withIntermediateDirectories: true) | ||
| if fm.fileExists(atPath: originalConfigDir.path) { | ||
| try fm.copyItem(at: originalConfigDir, to: tempConfigDir) | ||
| } else { | ||
| try fm.createDirectory(at: tempConfigDir, withIntermediateDirectories: true) | ||
| } | ||
|
|
||
| var environment = ProcessInfo.processInfo.environment | ||
| environment["HOME"] = tempHome.path | ||
| if let envKey = agent.configDirEnvOverride { | ||
| environment[envKey] = tempConfigDir.path | ||
| } | ||
|
|
||
| let launch = hookInstallLaunch(for: agent) | ||
| let installResult = runInstallCommand( | ||
| executableURL: launch.executableURL, | ||
| arguments: launch.arguments, | ||
| environment: environment, | ||
| fallbackCommand: agent.installCommand | ||
| ) | ||
| guard installResult.succeeded else { | ||
| return AgentHookDiffResult(succeeded: false, message: installResult.message, diff: "") | ||
| } | ||
|
|
||
| let relativePaths = diffRelativePaths(for: agent) | ||
| let diffs = relativePaths.compactMap { relativePath in | ||
| let oldURL = URL(fileURLWithPath: NSString(string: "~/\(relativePath)").expandingTildeInPath) | ||
| let newURL = tempHome.appendingPathComponent(relativePath) | ||
| return unifiedDiff(relativePath: relativePath, oldURL: oldURL, newURL: newURL) | ||
| } |
There was a problem hiding this comment.
buildHookDiff ignores configDirEnvOverride for source path
originalConfigDir is always derived from expandedHomePath(configDir) (e.g., ~/.codex), bypassing the same env-override logic used in configDirectoryPath(for:). When a user has CODEX_HOME=/custom/path or OPENCODE_CONFIG_DIR=/custom/dir, the copy step reads from ~/.codex (empty or stale) while the user's real config lives elsewhere. The oldURL in the diff comparison is also fixed to ~/configDir/configFile, so the displayed diff will show incorrect changes — the install writes to the temp env-override path, but the "before" is compared against the wrong default path. Use configDirectoryPath(for: agent) (which already handles the override) as the source, and compute oldURL from the same path.
| static func snoozePrompt(agentName: String, defaults: UserDefaults = .standard) { | ||
| guard let agent = agent(named: agentName) else { return } | ||
| let currentStatus = status(for: agent, defaults: defaults) | ||
| markPromptSnoozed(for: agent, status: currentStatus, defaults: defaults) | ||
| } |
There was a problem hiding this comment.
@MainActor in snoozePrompt
snoozePrompt is called from the "Not Now" button handler in TerminalNotificationActionButtons, which runs on @MainActor. It calls status(for: agent) — which executes FileManager.default.fileExists(atPath:) and String(contentsOfFile:encoding:) synchronously — before writing the snooze timestamp. This blocks the main thread on every "Not Now" tap.
The fix follows the shape now used in showSetupPromptIfNeeded: read the status in a Task.detached off the main actor and mark the snooze only after the status is known, or pass the already-known status as a parameter from the call site (the caller has it in @State var status).
Rule Used: Flag incorrect or missing use of Swift @Concurrent... (source)
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary
Testing
Summary by cubic
Adds agent hook setup prompts and a settings card so users can install or update hooks for supported agent CLIs, enabling
cmuxnotifications and session restore. Prompts trigger from zsh after agent commands, prefer the bundled CLI, and statuses refresh live by watching config files; code is split into smaller files to stay within Swift’s compilation budget.New Features
claude,codex,opencode/open-code,cursor,gemini,copilot,codebuddy,factory,qoderand reports viaagent_hooks.nudgeRPC through the bundled CLI (fallbackagent_hooks_nudge); separate cooldowns for install vs update; “Not Now” snoozes only (Never Show Again disables); per‑agent dedupe with stable IDs.cmux hooks <agent> install”); the popover opens automatically on a nudge.CMUX_AGENT_HOOK_VERSION=1; preferCMUX_BUNDLED_CLI_PATHfor hooks and the OpenCode bridge; added localized strings (en, ja).Bug Fixes
CODEX_HOME,OPENCODE_CONFIG_DIR, temporaryHOME) to match user setups.cursor agent/cursor-agent) and improved reliability of the hook review modal.Written for commit 0d567dd. Summary will update on new commits.
Summary by CodeRabbit
New Features
Localization