Keep unrelated file descriptors out of notification hooks

[teamleaderleo]

Lands #11649 by @chapati23: notification hooks are spawned with POSIX_SPAWN_CLOEXEC_DEFAULT, so they no longer inherit unrelated app file descriptors (an inherited pipe write end could keep a reader waiting until the hook timed out). The dup2 file actions still give the hook its stdin, stdout and stderr. Their two commits are kept; the only change is the conflict resolution, which keeps main's signal-mask reset and combines the flags as SETPGROUP | SETSIGMASK | CLOEXEC_DEFAULT.

Includes their regression test (NotificationHookProcessIsolationTests), which opens an unrelated pipe and checks the hook can't see it.

🤖 Generated with Claude Code

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

---

Summary by cubic

Notification hooks are now spawned with POSIX_SPAWN_CLOEXEC_DEFAULT, so they no longer inherit unrelated app file descriptors. Previously an inherited pipe write end could keep a reader waiting until the hook timed out; the dup2 file actions still give the hook its stdin, stdout, and stderr. Adds a regression test that opens an unrelated pipe and verifies the hook cannot see it.

^{Written for commit d2e9835. Summary will update on new commits.}

Summary by CodeRabbit

• Bug Fixes
  • Notification hooks now run with tighter process isolation: they no longer inherit unrelated open resources from the app, while their configured input and output remain available. This helps prevent unintended access to app resources when a hook runs. Existing notification content is preserved.

Changelog

Fixed: Notification hooks no longer inherit unrelated app file descriptors.

[github-actions]

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 1f7bc774-4211-4f62-af53-5b06447a6204

📥 Commits

Reviewing files that changed from the base of the PR and between cc90659 and d2e9835.

📒 Files selected for processing (2)

• Sources/TerminalNotificationPolicy.swift
• cmuxTests/NotificationAndMenuBarTests.swift

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

---

📝 Walkthrough

Walkthrough

Notification hooks now launch with POSIX_SPAWN_CLOEXEC_DEFAULT, while retaining the existing process-group and signal-mask flags. A new test checks that a hook cannot observe an unrelated parent pipe descriptor.

Changes

Notification hook process isolation

Layer / File(s)	Summary
Spawn flags and isolation check Sources/TerminalNotificationPolicy.swift, cmuxTests/NotificationAndMenuBarTests.swift	spawnHook adds POSIX_SPAWN_CLOEXEC_DEFAULT. The test opens an unrelated pipe and checks that the hook returns the original notification body.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to d2e98

The notification-hook descriptor isolation change is ready to merge after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to d2e98

The change narrows what notification hooks inherit from the app while preserving their standard input and output. No new security exposure was identified, but the available evidence does not establish complete end-to-end coverage.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The effective change is confined to descriptors inherited by notification-hook child processes. It reduces access to unrelated app descriptors without extending hook reachability.

Trust Boundaries and Controls

• observed — Explicit dup2 actions retain the hook's standard streams, while the added flag closes unrelated descriptors on execution. The existing empty signal mask and process-group setup remain in place.
• inferred — Configuration-derived hook execution remains an existing authority, not one introduced by this flag change. The direct caller supplying authorized hooks was not established by the bounded evidence.

Resilience and Maintainability Implications

• observed — Descriptor ownership transfers to the run only after a successful spawn. Local deferred closure, launch-failure cleanup, and the existing termination path remain in place.

🚥 Pre-merge checks | ✅ 24 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (24 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Cmux Cloud Persistent Session And Early Input	✅ Passed	PASS. The PR changes only notification-hook process spawning and adds a descriptor-isolation test. It does not change Cloud terminal creation, cmux-tui transport, manual renderer admission, attachment…
Cmux Swift Actor Isolation	✅ Passed	PASS. The only production change is the POSIX_SPAWN_CLOEXEC_DEFAULT flag and its comment inside the existing NotificationHookProcessRun: @unchecked Sendable implementation. It does not add or alte…
Cmux Swift Blocking Runtime	✅ Passed	PASS: The production diff only adds POSIX_SPAWN_CLOEXEC_DEFAULT to the existing posix_spawn flags and adds comments. It introduces no semaphore, wait, sleep, delayed dispatch, polling, main-queue …
Cmux Browser Automation Off-Main	✅ Passed	The pull request changes only Sources/TerminalNotificationPolicy.swift and cmuxTests/NotificationAndMenuBarTests.swift. It adds notification-hook process isolation with `POSIX_SPAWN_CLOEXEC_DEFAUL…
Cmux Expensive Synchronous Load	✅ Passed	PASS — The production diff only adds POSIX_SPAWN_CLOEXEC_DEFAULT to notification-hook spawn flags and comments. It does not add or move RestorableAgentSessionIndex.load(), agent-history file reads…
Cmux Cache Substitution Correctness	✅ Passed	The check does not apply. The production diff only adds POSIX_SPAWN_CLOEXEC_DEFAULT to notification-hook spawn flags and adds comments. It does not replace an authoritative read with a cached value or…
Cmux No Hacky Sleeps	✅ Passed	PASS: The pull request changes only Swift files. The production change adds POSIX_SPAWN_CLOEXEC_DEFAULT and does not add a sleep, timer, polling loop, or wall-clock wait. The existing test sleep is …
Cmux Algorithmic Complexity	✅ Passed	The production diff only adds the POSIX_SPAWN_CLOEXEC_DEFAULT flag and comments in Sources/TerminalNotificationPolicy.swift; it adds no collection traversal or algorithmic work. The test adds a bo…
Cmux Swift Concurrency	✅ Passed	The diff does not introduce or expand a prohibited legacy concurrency pattern. Production code only adds POSIX_SPAWN_CLOEXEC_DEFAULT to existing posix_spawn flags. The new regression test uses `as…
Cmux Swift @Concurrent	✅ Passed	The changed production code is synchronous spawnHook() flag configuration. The existing CPU/file/process-heavy TerminalNotificationPolicyEngine.evaluate overloads already use @concurrent with `n…
Cmux Swift Package Boundaries	✅ Passed	PASS: The production diff changes only the existing private NotificationHookProcessRun.spawnHook flag set in Sources/TerminalNotificationPolicy.swift, adding POSIX_SPAWN_CLOEXEC_DEFAULT and comm…
Cmux Swiftpm Lockfiles	✅ Passed	The pull request changes only Sources/TerminalNotificationPolicy.swift and cmuxTests/NotificationAndMenuBarTests.swift. The diff contains no Package.swift, Package.resolved, .gitignore, work…
Cmux Swift Logging	✅ Passed	The production diff only adds spawn flags and comments. It adds no print, debugPrint, dump, NSLog, ad hoc diagnostic logging, Logger declaration, or sensitive-data logging. The added printf …
Cmux User-Facing Error Privacy	✅ Passed	PASS. The PR changes only notification-hook process spawning and adds a regression test. The production additions are POSIX_SPAWN_CLOEXEC_DEFAULT and a developer-only comment; the test command is al…
Cmux Full Internationalization	✅ Passed	PASS: The production diff only adds the POSIX_SPAWN_CLOEXEC_DEFAULT flag and developer comments. It adds no user-facing text, catalog entry, metadata, web UI, API response, markdown, or changelog co…
Cmux Swiftui State Layout	✅ Passed	The PR does not introduce a prohibited SwiftUI state or layout pattern. The source change only adds POSIX_SPAWN_CLOEXEC_DEFAULT to notification-hook spawning. The test adds Darwin/Swift Testing covera…
Cmux Architecture Rethink	✅ Passed	PASS. The diff is a small local POSIX spawn correctness fix. NotificationHookProcessRun.spawnHook remains the single owner of hook process setup, and POSIX_SPAWN_CLOEXEC_DEFAULT enforces the descr…
Cmux Swift Auxiliary Window Close Shortcuts	✅ Passed	PASS: The PR changes notification-hook process flags and adds a test-only isolation suite. It does not add or materially change an NSWindow, NSPanel, NSWindowController, SwiftUI Window, or WindowGroup…
Cmux Source Artifacts	✅ Passed	The pull request changes only Sources/TerminalNotificationPolicy.swift and cmuxTests/NotificationAndMenuBarTests.swift. The diff contains hand-written Swift source and a regression test for notifi…
Cmux No Test Or Debug Seam In Production Source	✅ Passed	The only production change in Sources/TerminalNotificationPolicy.swift adds POSIX_SPAWN_CLOEXEC_DEFAULT to existing spawn flags and a comment. It adds no #if DEBUG block, test/debug-named member…
Title check	✅ Passed	The title clearly and concisely describes the primary change: preventing notification hooks from inheriting unrelated file descriptors.
Description check	✅ Passed	The description explains the problem, resulting behavior, implementation mechanism, regression test, and changelog entry. It does not state a test command or execution result and omits the template ch…

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[teamleaderleo]

Closing in favor of #11649, which landed the same fix with credit to @chapati23.