Skip to content

Run tmux hooks via detached CLI runner - #1002

Closed
austinywang wants to merge 2 commits into
mainfrom
issue-996-set-hook-sandbox
Closed

austinywang wants to merge 2 commits into
mainfrom
issue-996-set-hook-sandbox

Conversation

@austinywang

@austinywang austinywang commented Mar 6, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • move tmux-compat hook execution into a detached cmux tmux-hook-runner helper so hook commands run outside the app sandbox
  • dispatch hook events from the app over a private Unix socket, keep the runner warm from shell integration, and stop it when the last hook is removed
  • fire workspace-close only after the window actually unregisters, encode closed-surface directories safely, and drain hook stdout/stderr asynchronously to avoid deadlocks
  • extend the tmux compatibility matrix test to cover persisted hook execution and idempotent helper startup

Verification

  • ./scripts/setup.sh
  • ./scripts/reload.sh --tag issue-996-set-hook-sandbox
  • no local automated tests were run per repo policy

Fixes #996

Summary by CodeRabbit

  • New Features

    • Extensible hook system for workspace and surface lifecycle events, with a tmux-compatible hook runner and environment-aware command execution.
    • Shell integrations now ensure the hook runner is started automatically.
  • Bug Fixes

    • Improved stability when closing windows/tabs: safer close handling and guaranteed per-tab close hook invocation.
  • Tests

    • Added/updated tests covering hook setup, triggering, and teardown.

@vercel

vercel Bot commented Mar 6, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
cmux Ready Ready Preview, Comment Mar 9, 2026 10:48pm

@coderabbitai

coderabbitai Bot commented Mar 6, 2026 •

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Adds a tmux-compatible hook subsystem: a Unix-socket hook runner and dispatcher, wire-ups in TabManager/Workspace to fire workspace and surface close hooks, CLI support to run/ensure/stop the hook-runner, shell integration to auto-start the runner, and a small AppDelegate safety tweak when closing windows.

Changes

Cohort / File(s) Summary
Hook dispatcher & runtime
Sources/TabManager.swift, Sources/Workspace.swift
Adds TmuxCompatHookEvent, TmuxCompatHookContext, hook-dispatch helpers, environment rendering, and methods to fire workspace/surface hooks from TabManager and Workspace. Integrates hook firing at workspace create/close and surface close points.
Hook runner CLI & server
CLI/cmux.swift
Implements tmux-hook-runner command path: JSON request/response protocol over AF_UNIX, server loop, request handling (ping/fire/shutdown), command execution with captured stdout/stderr, lifecycle helpers to ensure/stop runner, and POSIX socket helpers.
App/window lifecycle
Sources/AppDelegate.swift
On window unregister, iterates removed context tabs and calls per-tab workspace-close hook; in closeMainWindowContainingTabId, guards optional window resolution before calling performClose to avoid force-unwrap.
Shell integrations
Resources/shell-integration/cmux-bash-integration.bash, Resources/shell-integration/cmux-zsh-integration.zsh
Adds one-time initializer and global flag to ensure tmux hook runner is started asynchronously from shell init; invokes during PATH fixup.
Tests
tests_v2/test_tmux_compat_matrix.py
Reworks hook test to use workspace-close hook: creates temp log/token, ensures runner, closes workspace to trigger hook, waits for token, and cleans up.

Sequence Diagram

sequenceDiagram
    participant App as AppDelegate
    participant TabMgr as TabManager
    participant Dispatcher as HookDispatcher
    participant SocketRunner as tmux-hook-runner (CLI)
    participant External as User Script/Hook

    App->>TabMgr: closeMainWindowContainingTabId(tabId)
    TabMgr->>TabMgr: resolve workspace/tab context
    TabMgr->>Dispatcher: fireWorkspaceCloseHookIfPresent(tabId)
    Dispatcher->>SocketRunner: connect & send JSON "fire" request (env + cwd + cmd)
    SocketRunner->>SocketRunner: spawn shell command with provided env & cwd
    SocketRunner->>External: executes hook command (stdout/stderr)
    SocketRunner-->>Dispatcher: send OK/ERROR response
    Dispatcher-->>TabMgr: report hook result
    TabMgr-->>App: continue window close flow
Loading

Estimated Code Review Effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related PRs

Poem

I’m a rabbit by the Unix sock,
I nibble events around the clock.
Workspace closes, hooks take flight,
A token writes, the log is bright.
Hopping, piping—cleanup done—🐰✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 4.69% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: moving tmux hook execution to a detached CLI runner, which is the primary objective of the PR.
Description check ✅ Passed The description covers what changed, why it was done, and verification steps performed, following most of the template structure despite not using exact template formatting.
Linked Issues check ✅ Passed The PR successfully implements Option B (IPC mechanism) from issue #996 by dispatching hooks over a Unix socket, allowing hook commands to execute outside the sandbox via a detached runner.
Out of Scope Changes check ✅ Passed All changes directly support the core objective of executing tmux hooks outside the sandbox via a detached CLI runner; no extraneous modifications detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch issue-996-set-hook-sandbox

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/AppDelegate.swift`:
- Around line 8095-8098: The code calls
context.tabManager.fireWorkspaceCloseHookIfPresent(tabId:) before confirming a
close target exists, which can run hooks even when no NSWindow is resolved;
change the order so you first resolve the window (using the same
expectedIdentifier lookup and context.window fallback into the local window
variable) and only call
context.tabManager.fireWorkspaceCloseHookIfPresent(tabId:) if that resolved
window is non-nil, then call window?.performClose(nil); this ensures
fireWorkspaceCloseHookIfPresent, expectedIdentifier, window and performClose are
only used when a concrete close target exists.

In `@Sources/TabManager.swift`:
- Around line 74-76: The current assignment to
env["CMUX_CLOSED_SURFACE_DIRECTORIES"] uses
closedSurfaceDirectories.joined(separator: ":"), which will break when paths
contain ':'; change the code that sets this env value to use an unambiguous
encoding such as joining with "\n" or serializing closedSurfaceDirectories as a
JSON array (e.g., use JSONEncoder to encode closedSurfaceDirectories to Data and
convert to String) and assign that string to
env["CMUX_CLOSED_SURFACE_DIRECTORIES"] so downstream consumers can reliably
parse it; update any consumers to decode the chosen format accordingly.
- Around line 136-143: The code calls process.run() and then
process.waitUntilExit() without draining the stdout/stderr pipes (stdout and
stderr), which can deadlock if the hook command writes > pipe buffer; update the
logic that launches the process (the block handling process, stdout, stderr in
TabManager.swift) to read the pipes asynchronously before waiting for exit—e.g.,
attach readabilityHandler callbacks or use background readToEnd handlers on
stdout.fileHandleForReading and stderr.fileHandleForReading to capture output as
it arrives, store the data/errors, and only then call waitUntilExit() or observe
termination; ensure you remove/clear handlers after completion to avoid leaks.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 74fe92ee-580d-4a65-8454-d28fa10ee01e

📥 Commits

Reviewing files that changed from the base of the PR and between 43f42b7 and f640a3d.

📒 Files selected for processing (3)
  • Sources/AppDelegate.swift
  • Sources/TabManager.swift
  • Sources/Workspace.swift

Comment thread Sources/AppDelegate.swift Outdated
Comment thread Sources/TabManager.swift Outdated
Comment thread Sources/TabManager.swift Outdated
@greptile-apps

greptile-apps Bot commented Mar 6, 2026 •

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds a TmuxCompatHookDispatcher to fire lifecycle hooks (workspace-created, workspace-close, surface-close, pane-close) via osascript so that user-configured shell commands can run despite the app sandbox blocking direct process spawning. The architectural approach — building a shell environment, shell-quoting all values with cmuxShellQuoted, and delegating execution to osascript do shell script — is sound, but four correctness issues in dispatch timing and process handling must be addressed before merging:

  1. Pipe deadlock in run(command:context:) (line 137–157 of TabManager.swift): process.waitUntilExit() is called before either pipe is drained. If a hook script produces >64 KB of output, the subprocess blocks on write while waitUntilExit() blocks on process exit, permanently hanging the background queue.

  2. surface-close fires before user confirms (line 1483 of TabManager.swift): In the isLastTabInWorkspace path, fireSurfaceCloseHook is called before the confirmation dialog is shown. If the user cancels, the hook was dispatched for a close that never occurred.

  3. workspace-close fires before performClose confirms (line 8095 of AppDelegate.swift): The hook is fired before performClose(nil), which can be cancelled by the window's delegate. If cancelled, the hook was dispatched for a close that never happened.

  4. surface-close double-fires (line 4100 of Workspace.swift): The hook is fired in TabManager's pre-close paths (lines 1483 and 1623) and then again in splitTabBar(_:didCloseTab:) because panels[panelId] still exists at the time of the second dispatch (removed only at line 4134).

These are not edge-case regressions — they affect the primary hook delivery paths that the PR is explicitly designed to implement. The pipe-deadlock can permanently stall the hook dispatch queue; double-fires and premature fires before confirmation violate the semantic contract that hooks only execute for closes that actually occur.

Confidence Score: 2/5

  • Not safe to merge — multiple hooks fire prematurely before user confirmation, can double-fire, and a background-queue deadlock risks hang for any hook producing substantial output.
  • Four independent correctness bugs affect the primary hook delivery paths: (1) a pipe deadlock that can permanently stall the background queue if hook output exceeds ~64 KB, (2–3) hooks dispatched before the user confirms the close action (via dialog or performClose delegate), meaning hook scripts observe a "close" that may be subsequently cancelled, and (4) surface-close double-firing in the last-tab and child-exit paths. None are edge-case regressions — they strike at the core of what this PR is designed to implement. Without fixes, users will see unreliable hook execution, background queue hangs, and duplicate hook invocations.
  • Sources/TabManager.swift (pipe deadlock in run method and premature hook dispatch in isLastTabInWorkspace), Sources/AppDelegate.swift (premature hook dispatch in closeMainWindowContainingTabId), and Sources/Workspace.swift (double-fire risk in splitTabBar(_:didCloseTab:)).

Sequence Diagram

sequenceDiagram
    participant User
    participant TabManager
    participant Workspace
    participant Bonsplit
    participant TmuxCompatHookDispatcher
    participant osascript

    Note over TabManager,osascript: surface-close (last panel, explicit close)
    User->>TabManager: closeShortcutTab(panelId)
    TabManager->>TmuxCompatHookDispatcher: fireSurfaceCloseHook ⚠️ (before confirm dialog)
    TabManager->>User: Show confirm dialog (optional)
    TabManager->>Workspace: closePanel / closeWorkspace
    Workspace->>Bonsplit: close pane
    Bonsplit-->>Workspace: splitTabBar(_:didCloseTab:)
    Workspace->>TmuxCompatHookDispatcher: fire(surfaceClose) ⚠️ duplicate if panel still in panels

    Note over TabManager,osascript: workspace-close (closing window)
    User->>TabManager: closeShortcutTab (isLastTab, willCloseWindow)
    TabManager->>AppDelegate: closeMainWindowContainingTabId
    AppDelegate->>TmuxCompatHookDispatcher: fireWorkspaceCloseHookIfPresent ⚠️ before performClose
    AppDelegate->>NSWindow: performClose(nil)
    NSWindow-->>AppDelegate: (may be cancelled by delegate)

    Note over TabManager,osascript: pipe deadlock risk
    TmuxCompatHookDispatcher->>osascript: launch process with stdout/stderr pipes
    osascript->>osascript: writes >64KB to stdout
    osascript->>TmuxCompatHookDispatcher: (blocked waiting for reader)
    TmuxCompatHookDispatcher->>TmuxCompatHookDispatcher: waitUntilExit() ⚠️ (blocked waiting for process)
Loading

Comments Outside Diff (2)

  1. Sources/TabManager.swift, line 1481-1508 (link)

    Hook fires before confirmation dialog; fires prematurely on user cancel

    fireSurfaceCloseHook is called at line 1483 before the needsConfirm dialog is shown (lines 1486–1508). If the user is presented with a "Close tab?" dialog and clicks Cancel (line 1507 return), the surface-close hook has already been dispatched for a close that never actually happened.

    The hook should be moved to after the user confirms, so it only fires when the close is certain to proceed:

    if isLastTabInWorkspace {
        let willCloseWindow = tabs.count <= 1
        let needsConfirm = workspaceNeedsConfirmClose(tab)
        if needsConfirm {
            guard confirmClose(...) else { return }
        }
        fireSurfaceCloseHook(workspace: tab, panelId: panelId)   // ← moved after confirm
        AppDelegate.shared?.notificationStore?.clearNotifications(forTabId: tab.id)
        ...
    }
  2. Sources/AppDelegate.swift, line 8093-8099 (link)

    workspace-close hook fires before performClose confirms dismissal

    fireWorkspaceCloseHookIfPresent is called at line 8095 before performClose(nil) at line 8098. Since performClose(nil) is the "polite" close path that calls windowShouldClose: on the window's delegate first, if the delegate (or any confirmation sheet) returns false / cancels, the window stays open, but the workspace-close hook has already been dispatched.

    The call should be moved to after the window has confirmed it will close (e.g., inside windowWillClose / windowShouldClose where the outcome is certain), or guarded to only fire after performClose succeeds.

Last reviewed commit: f640a3d

Comment thread Sources/TabManager.swift Outdated
Comment on lines +137 to +157
let stderr = Pipe()
process.standardOutput = stdout
process.standardError = stderr

do {
try process.run()
process.waitUntilExit()
} catch {
NSLog("tmux hook %@ failed to launch via osascript: %@", context.event.rawValue, String(describing: error))
#if DEBUG
dlog("hook.exec.fail event=\(context.event.rawValue) reason=launch")
#endif
return
}

guard process.terminationStatus == 0 else {
let stderrText = String(data: stderr.fileHandleForReading.readDataToEndOfFile(), encoding: .utf8)?
.trimmingCharacters(in: .whitespacesAndNewlines) ?? ""
let stdoutText = String(data: stdout.fileHandleForReading.readDataToEndOfFile(), encoding: .utf8)?
.trimmingCharacters(in: .whitespacesAndNewlines) ?? ""
let details = stderrText.isEmpty ? stdoutText : stderrText

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pipe deadlock if subprocess produces large output

process.waitUntilExit() is called synchronously before either pipe is drained. On macOS, each pipe has a fixed kernel buffer (~64 KB). If the hook script writes more than that to stdout or stderr before exiting, the subprocess will block waiting for a reader, while waitUntilExit() blocks waiting for the subprocess to exit — a classic deadlock that will hang the background queue indefinitely.

The success path never reads stdout at all (lines 152–157 only read pipes on failure), which means even a well-behaved hook that only logs to stdout can trigger this.

The standard fix is to drain both pipes asynchronously before calling waitUntilExit():

// Before calling process.run(), attach async readers:
stdout.fileHandleForReading.readabilityHandler = { _ in
    // discard or accumulate stdout bytes
}
stderr.fileHandleForReading.readabilityHandler = { handle in
    // accumulate for later error reporting
}
try process.run()
process.waitUntilExit()
stdout.fileHandleForReading.readabilityHandler = nil
stderr.fileHandleForReading.readabilityHandler = nil

Comment thread Sources/Workspace.swift
Comment on lines +4100 to +4107
if !isDetaching, let hookContext = hookContextForSurfaceClose(panelId: panelId) {
TmuxCompatHookDispatcher.shared.fire(hookContext)
}
if paneClosedViaTabClose {
TmuxCompatHookDispatcher.shared.fire(
hookContextForPaneClose(paneId: pane, closedPanelIds: [panelId])
)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

surface-close hook can fire twice in the isLastTabInWorkspace and child-exit-last-panel paths

In TabManager, both closeShortcutTab (line ~1483, the isLastTabInWorkspace branch) and closePanelAfterChildExited (line ~1623, the panels.count <= 1 branch) call fireSurfaceCloseHook before the close propagates to Bonsplit. Those paths then continue into the normal close machinery which ultimately calls splitTabBar(_:didCloseTab:) here — and at line 4100–4101, the hook is fired a second time because panels[panelId] is still populated at that point (it is only removed at line 4134).

The guard guard panels[panelId] != nil else { return nil } inside hookContextForSurfaceClose prevents duplicates for the pendingPaneClosePanelIds flow (since the panel is removed before didClosePane runs), but it does not protect against the TabManager-level pre-fire in the above two paths.

Consider tracking already-fired hook events per panel (e.g., a Set<UUID> cleared after close completes), or removing the early fireSurfaceCloseHook calls from TabManager and relying solely on the splitTabBar(_:didCloseTab:) dispatch site.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 3 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="Sources/TabManager.swift">

<violation number="1" location="Sources/TabManager.swift:143">
P1: Calling `waitUntilExit()` before reading from connected `Pipe` objects can cause a deadlock if the command output exceeds the pipe buffer capacity. Data must be read before or concurrently with waiting for the process to exit.</violation>

<violation number="2" location="Sources/TabManager.swift:1483">
P2: The `surface-close` hook is fired before the user confirms closing the tab. If the user cancels the dialog, the hook will have incorrectly fired for a surface that remains open. Move this hook invocation below the `needsConfirm` block.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread Sources/TabManager.swift Outdated
Comment thread Sources/TabManager.swift Outdated
@austinywang austinywang changed the title Fix tmux compatibility hook execution Run tmux hooks via detached CLI runner Mar 9, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (2)
Sources/TabManager.swift (2)

263-281: Consider adding a read timeout to prevent indefinite blocking.

The readResponse function reads from the socket until newline or EOF, but if the hook runner hangs without closing the connection or sending a response, this could block the utility queue indefinitely. Consider setting SO_RCVTIMEO on the socket.

💡 Suggested timeout addition
         guard connectResult == 0 else {
             throw posixError("connect")
         }
+
+        // Set a read timeout to prevent indefinite blocking if the runner hangs
+        var timeout = timeval(tv_sec: 5, tv_usec: 0)
+        _ = setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &timeout, socklen_t(MemoryLayout<timeval>.size))
 
         var payload = try JSONEncoder().encode(request)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Sources/TabManager.swift` around lines 263 - 281, readResponse currently
blocks indefinitely reading from the file descriptor; set a receive timeout on
the socket (SO_RCVTIMEO) for fd before the read loop (use setsockopt with a
timeval) and handle timeout errors inside readResponse by detecting
EAGAIN/EWOULDBLOCK/EINTR or the specific errno for timeout and throwing a
distinct error (or returning a descriptive error via posixError). Ensure you
restore or clear the socket option if needed, and update callers to handle the
timeout error from readResponse. Reference the function name readResponse and
the posixError helper when adding the timeout handling and error path.

249-261: Minor: Consider handling zero-byte write edge case.

While rare, write() returning 0 (no bytes written but no error) could cause an infinite loop. This typically shouldn't happen with stream sockets, but adding a guard would be defensive.

💡 Defensive check for zero-byte write
         try data.withUnsafeBytes { rawBuffer in
             guard let baseAddress = rawBuffer.bindMemory(to: UInt8.self).baseAddress else { return }
             var offset = 0
             while offset < rawBuffer.count {
                 let written = Darwin.write(fd, baseAddress.advanced(by: offset), rawBuffer.count - offset)
                 guard written >= 0 else {
                     throw posixError("write")
                 }
+                guard written > 0 else {
+                    throw NSError(domain: NSPOSIXErrorDomain, code: Int(EAGAIN),
+                                  userInfo: [NSLocalizedDescriptionKey: "write returned 0 bytes"])
+                }
                 offset += written
             }
         }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Sources/TabManager.swift` around lines 249 - 261, The writeAll(_:, to:)
function can loop forever if Darwin.write returns 0; modify the loop in writeAll
to treat written == 0 as an error by throwing (e.g., throw posixError("write
returned 0") or a specific EOF/write error) so the function breaks out instead
of spinning; update the guard around the written result (after calling
Darwin.write with baseAddress.advanced(by: offset) and rawBuffer.count - offset)
to handle written < 0 (existing) and written == 0 (new) and ensure callers of
writeAll handle the thrown error appropriately.
🤖 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 5929-5942: Concurrent --ensure calls can race: both may detect the
runner is down and one can unlink the other's freshly bound socket. Wrap the
stale-socket cleanup and bind/creation sequence in a serialization guard (e.g.,
a filesystem lock or PID lockfile) so only one process performs the
check/remove/create sequence at a time; specifically, before calling
tmuxCompatHookRunnerCanConnect(to: socketURL.path) return path or before
executing FileManager.default.removeItem(at: socketURL), acquire the lock for
socketURL (and write the holder PID), re-check tmuxCompatHookRunnerCanConnect
after acquiring the lock, perform removeItem and createDirectory while holding
the lock, and release the lock after bind/spawn completes; apply the same change
to the other instance of this sequence at the 6026-6062 region.
- Around line 6177-6194: The current failure logging writes raw hook output
(variables stdoutData/stderrData combined into details) into production logs;
change the block in the tmux hook runner where it builds details and calls NSLog
to only log the event and process.terminationStatus at release, and move the
extraction of stderrText/stdoutText and the detailed log into a debug-only path:
wrap the detailed diagnostics in `#if` DEBUG ... `#endif` and call dlog(...) inside
that block using stderrText/stdoutText (or details) so production logs never
contain arbitrary hook output; keep the public NSLog call limited to the event
and exit code.
- Around line 5830-5848: tmuxCompatHookRunnerCanConnect currently calls
sendTmuxCompatHookRunnerRequest which can block forever if the runner wedges;
modify the RPC path to enforce a hard timeout: add a timeout parameter to
sendTmuxCompatHookRunnerRequest (or create a wrapper) and ensure the socket
read/response path uses nonblocking I/O or a DispatchSource/DispatchSemaphore
with a DispatchQueue timer so the call throws/returns on timeout; update
tmuxCompatHookRunnerCanConnect to pass a short timeout and treat timeout as
failure, and apply the same change to the other hook-runner RPC caller around
the 5890-5918 region so all hook-runner RPCs fail fast instead of blocking
forever.
- Around line 6143-6149: The loop that merges request.environment into the local
environment currently drops entries with empty values; change it so empty values
are preserved for keys that start with "CMUX_" but still skip empty keys and
skip other empty values. Concretely, in the merge over request.environment (the
environment variable and request.environment identifiers), keep the key if
!key.isEmpty, and only skip when value.isEmpty AND !key.hasPrefix("CMUX_");
otherwise assign environment[key] = value; leave the PATH default logic
unchanged.

---

Nitpick comments:
In `@Sources/TabManager.swift`:
- Around line 263-281: readResponse currently blocks indefinitely reading from
the file descriptor; set a receive timeout on the socket (SO_RCVTIMEO) for fd
before the read loop (use setsockopt with a timeval) and handle timeout errors
inside readResponse by detecting EAGAIN/EWOULDBLOCK/EINTR or the specific errno
for timeout and throwing a distinct error (or returning a descriptive error via
posixError). Ensure you restore or clear the socket option if needed, and update
callers to handle the timeout error from readResponse. Reference the function
name readResponse and the posixError helper when adding the timeout handling and
error path.
- Around line 249-261: The writeAll(_:, to:) function can loop forever if
Darwin.write returns 0; modify the loop in writeAll to treat written == 0 as an
error by throwing (e.g., throw posixError("write returned 0") or a specific
EOF/write error) so the function breaks out instead of spinning; update the
guard around the written result (after calling Darwin.write with
baseAddress.advanced(by: offset) and rawBuffer.count - offset) to handle written
< 0 (existing) and written == 0 (new) and ensure callers of writeAll handle the
thrown error appropriately.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 85a5dae0-d30b-474e-833b-3f19f26789f4

📥 Commits

Reviewing files that changed from the base of the PR and between f640a3d and ea01d0e.

📒 Files selected for processing (6)
  • CLI/cmux.swift
  • Resources/shell-integration/cmux-bash-integration.bash
  • Resources/shell-integration/cmux-zsh-integration.zsh
  • Sources/AppDelegate.swift
  • Sources/TabManager.swift
  • tests_v2/test_tmux_compat_matrix.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • Sources/AppDelegate.swift

Comment thread CLI/cmux.swift
Comment on lines +5830 to +5848
private func tmuxCompatHookRunnerCanConnect(to path: String) -> Bool {
var st = stat()
guard lstat(path, &st) == 0 else { return false }
guard (st.st_mode & mode_t(S_IFMT)) == mode_t(S_IFSOCK) else { return false }
let socketURL = URL(fileURLWithPath: path)
do {
let response = try sendTmuxCompatHookRunnerRequest(
TmuxCompatHookRunnerRequest(
kind: .ping,
event: nil,
environment: [:],
workingDirectory: nil
),
socketURL: socketURL
)
return response == "OK"
} catch {
return false
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add a timeout to hook-runner RPCs.

Once connect() succeeds, readLine() can block forever here. A wedged runner will then hang --ensure, --stop, and set-hook indefinitely instead of falling back to restart/cleanup.

Also applies to: 5890-5918

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@CLI/cmux.swift` around lines 5830 - 5848, tmuxCompatHookRunnerCanConnect
currently calls sendTmuxCompatHookRunnerRequest which can block forever if the
runner wedges; modify the RPC path to enforce a hard timeout: add a timeout
parameter to sendTmuxCompatHookRunnerRequest (or create a wrapper) and ensure
the socket read/response path uses nonblocking I/O or a
DispatchSource/DispatchSemaphore with a DispatchQueue timer so the call
throws/returns on timeout; update tmuxCompatHookRunnerCanConnect to pass a short
timeout and treat timeout as failure, and apply the same change to the other
hook-runner RPC caller around the 5890-5918 region so all hook-runner RPCs fail
fast instead of blocking forever.

Comment thread CLI/cmux.swift
Comment on lines +5929 to +5942
if tmuxCompatHookRunnerCanConnect(to: socketURL.path) {
return
}

let parent = socketURL.deletingLastPathComponent()
try FileManager.default.createDirectory(
at: parent,
withIntermediateDirectories: true,
attributes: [.posixPermissions: 0o700]
)

if FileManager.default.fileExists(atPath: socketURL.path) {
try? FileManager.default.removeItem(at: socketURL)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Serialize runner startup before unlinking the socket path.

Two concurrent --ensure calls can both decide the runner is down, and the later one can unlink the first runner’s freshly bound socket before spawning another copy. That leaves an orphaned helper behind and can make startup fail even though one runner is already alive. Put a lock/PID guard around the stale-socket cleanup + bind sequence.

Also applies to: 6026-6062

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@CLI/cmux.swift` around lines 5929 - 5942, Concurrent --ensure calls can race:
both may detect the runner is down and one can unlink the other's freshly bound
socket. Wrap the stale-socket cleanup and bind/creation sequence in a
serialization guard (e.g., a filesystem lock or PID lockfile) so only one
process performs the check/remove/create sequence at a time; specifically,
before calling tmuxCompatHookRunnerCanConnect(to: socketURL.path) return path or
before executing FileManager.default.removeItem(at: socketURL), acquire the lock
for socketURL (and write the holder PID), re-check
tmuxCompatHookRunnerCanConnect after acquiring the lock, perform removeItem and
createDirectory while holding the lock, and release the lock after bind/spawn
completes; apply the same change to the other instance of this sequence at the
6026-6062 region.

Comment thread CLI/cmux.swift
Comment on lines +6143 to +6149
var environment = ProcessInfo.processInfo.environment
for (key, value) in request.environment where !key.isEmpty && !value.isEmpty {
environment[key] = value
}
if environment["PATH"]?.isEmpty ?? true {
environment["PATH"] = "/usr/bin:/bin:/usr/sbin:/sbin"
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Keep empty CMUX_* variables instead of dropping them.

Blank titles/cwds are valid hook context. The !value.isEmpty filter turns those into unset variables, which breaks set -u hook scripts and no longer matches the “full hook environment” contract.

Suggested fix
-        for (key, value) in request.environment where !key.isEmpty && !value.isEmpty {
+        for (key, value) in request.environment where !key.isEmpty {
             environment[key] = value
         }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
var environment = ProcessInfo.processInfo.environment
for (key, value) in request.environment where !key.isEmpty && !value.isEmpty {
environment[key] = value
}
if environment["PATH"]?.isEmpty ?? true {
environment["PATH"] = "/usr/bin:/bin:/usr/sbin:/sbin"
}
var environment = ProcessInfo.processInfo.environment
for (key, value) in request.environment where !key.isEmpty {
environment[key] = value
}
if environment["PATH"]?.isEmpty ?? true {
environment["PATH"] = "/usr/bin:/bin:/usr/sbin:/sbin"
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@CLI/cmux.swift` around lines 6143 - 6149, The loop that merges
request.environment into the local environment currently drops entries with
empty values; change it so empty values are preserved for keys that start with
"CMUX_" but still skip empty keys and skip other empty values. Concretely, in
the merge over request.environment (the environment variable and
request.environment identifiers), keep the key if !key.isEmpty, and only skip
when value.isEmpty AND !key.hasPrefix("CMUX_"); otherwise assign
environment[key] = value; leave the PATH default logic unchanged.

Comment thread CLI/cmux.swift
Comment on lines +6177 to +6194
NSLog("tmux hook runner failed to launch %@: %@", event, String(describing: error))
return
}

let stdoutData = stdoutCapture.wait()
let stderrData = stderrCapture.wait()
guard process.terminationStatus == 0 else {
let stderrText = String(data: stderrData, encoding: .utf8)?
.trimmingCharacters(in: .whitespacesAndNewlines) ?? ""
let stdoutText = String(data: stdoutData, encoding: .utf8)?
.trimmingCharacters(in: .whitespacesAndNewlines) ?? ""
let details = stderrText.isEmpty ? stdoutText : stderrText
NSLog(
"tmux hook runner command %@ failed (%d): %@",
event,
process.terminationStatus,
details
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Avoid writing raw hook stdout/stderr to production logs.

details is arbitrary user hook output, so this can leak repo paths, filenames, or secrets into the unified log on failures. Keep release logging to event/exit status only, and gate detailed diagnostics behind #if DEBUG with dlog.

As per coding guidelines, **/*.swift: Debug event logging must be wrapped in #if DEBUG / #endif blocks; use free function dlog("message") for logging with timestamp that appends to file in real time.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@CLI/cmux.swift` around lines 6177 - 6194, The current failure logging writes
raw hook output (variables stdoutData/stderrData combined into details) into
production logs; change the block in the tmux hook runner where it builds
details and calls NSLog to only log the event and process.terminationStatus at
release, and move the extraction of stderrText/stdoutText and the detailed log
into a debug-only path: wrap the detailed diagnostics in `#if` DEBUG ... `#endif`
and call dlog(...) inside that block using stderrText/stdoutText (or details) so
production logs never contain arbitrary hook output; keep the public NSLog call
limited to the event and exit code.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 7 files

@lawrencecchen lawrencecchen added the stale-revisit Closed after 30+ days without activity; preserved for possible revisit or reopening. label Sep 23, 2026
@github-project-automation github-project-automation Bot moved this from Todo to Done in cmux backlog Sep 23, 2026

This branch was successfully deployed

1 active deployment
Preview — ea01d0e6 Deployed Mar 9, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale-revisit Closed after 30+ days without activity; preserved for possible revisit or reopening.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

set-hook: ENOENT posix_spawn '/bin/sh' — sandbox blocks shell execution

3 participants