Add cmux wait — block until notification arrives - #1
Open
shaunr0b wants to merge 1 commit into
Open
Conversation
…-ai#598) Orchestrator agents need to wait for notifications from other workspaces. This adds a lease-based polling `notification.wait` V2 endpoint and a `cmux wait` CLI command that blocks until a notification arrives or timeout. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis change introduces bidirectional notification polling functionality. The CLI adds a "wait" subcommand that initiates server-side polling, while the server implements a v2 notification.wait endpoint that polls the notification store with configurable timeouts and batch behavior, returning accumulated unseen notifications or an empty result on timeout. Changes
Sequence DiagramsequenceDiagram
participant Client as CLI Client
participant Server as TerminalController
participant Store as NotificationStore
Client->>Server: notification.wait (poll_timeout_ms, batch_ms, filters)
Server->>Store: Capture baseline notification IDs
loop Poll until deadline
Server->>Store: Query notifications
alt New notifications detected
Server->>Server: Record firstSeen timestamp
Server->>Server: Wait for batch_ms window
Server->>Store: Mark notifications as read
Server->>Client: Return {notifications: [...], timed_out: false}
else No new notifications
Server->>Server: Continue polling (0.05s interval)
end
end
alt Deadline reached
alt Notifications accumulated
Server->>Client: Return {notifications: [...], timed_out: true}
else No notifications
Server->>Client: Return {notifications: [], timed_out: true}
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
Sources/TerminalController.swift (1)
4751-4763: Consolidate duplicated notification serialization logic.The payload mapping is duplicated in both return paths. Extracting a single serializer (and shared formatter) will reduce maintenance drift risk.
Also applies to: 4781-4796
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 4751 - 4763, Extract the duplicated mapping into a single serializer and shared formatter: add a private method (e.g. serializeNotification(_ notification: Notification) -> [String: Any]) inside TerminalController that uses a shared ISO8601DateFormatter instance (static let or lazy var) and returns the same dictionary keys ("id","workspace_id","surface_id","title","subtitle","body","created_at","is_read") using v2OrNull for surface_id and formatter.string(from:). Replace both map blocks (the one creating newNotifications.map {...} at the shown snippet and the other at 4781-4796) to call serializeNotification(n) so the formatting/serialization logic is centralized.
🤖 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 4252-4263: The help text for the "cmux wait" command currently
says "--workspace <id|ref>" but the command also accepts an index; update the
usage/help strings to reflect this by changing occurrences of "<id|ref>" to
"<id|ref|index>" (or similar) in the wait help block and the other documented
location mentioned (the string used for the wait command help and any duplicated
help at the other occurrence), ensuring the displayed Flags section for
"--workspace" now lists the accepted formats including index so both help
outputs (the one around the cmux wait usage and the duplicated one) are
consistent.
- Around line 1308-1315: When parsing timeoutStr and batchStr for overallTimeout
and batchMs, reject invalid numeric input instead of silently falling back: if
timeoutStr is non-nil but cannot be converted to a positive Double (or is <= 0)
print a clear error and exit non-zero (do not treat as "wait forever");
likewise, if batchStr is non-nil but cannot be converted to an Int (or is
negative) print a clear error and exit; keep the existing default behavior only
when the flag is absent (timeoutStr == nil or batchStr == nil). Update the
parsing code that computes overallTimeout and batchMs to validate the presence
of timeoutStr/batchStr and fail fast on invalid values (referencing
overallTimeout, batchMs, timeoutStr, batchStr).
- Around line 1321-1324: The signal handler for SIGINT currently calls print()
and exit(), both unsafe in a signal handler; replace this by either (preferred)
creating a DispatchSourceSignal for SIGINT and perform printing and clean
shutdown on its dispatch queue, or (simpler) make the handler set a volatile
sig_atomic_t flag (e.g., shutdownRequested) and return, then check that flag in
the main run loop to call print() and perform graceful shutdown; also replace
exit() with _exit() if any immediate process termination is required inside a
handler. Ensure you update the SIGINT setup (the closure currently passed to
signal(SIGINT) { ... }) and reference the new DispatchSourceSignal or the
sig_atomic_t flag and the location where the main loop observes it.
In `@Sources/TerminalController.swift`:
- Around line 4717-4718: The current use of v2UUID(params, "workspace_id") and
v2UUID(params, "exclude_surface_id") lets invalid IDs silently become nil and
widen the query; change this so that when the params dictionary contains
"workspace_id" or "exclude_surface_id" but v2UUID returns nil, the handler
rejects the request with a validation error (e.g., 400) instead of treating it
as absent. Locate the code that assigns workspaceFilter and excludeSurface and
add a check: if params.keys.contains("workspace_id") && workspaceFilter == nil
(and similarly for exclude_surface_id) then return/throw a
validation/bad-request response with a clear message about the invalid UUID.
Ensure you reference the same v2UUID helper and preserve existing behavior only
for truly absent keys.
---
Nitpick comments:
In `@Sources/TerminalController.swift`:
- Around line 4751-4763: Extract the duplicated mapping into a single serializer
and shared formatter: add a private method (e.g. serializeNotification(_
notification: Notification) -> [String: Any]) inside TerminalController that
uses a shared ISO8601DateFormatter instance (static let or lazy var) and returns
the same dictionary keys
("id","workspace_id","surface_id","title","subtitle","body","created_at","is_read")
using v2OrNull for surface_id and formatter.string(from:). Replace both map
blocks (the one creating newNotifications.map {...} at the shown snippet and the
other at 4781-4796) to call serializeNotification(n) so the
formatting/serialization logic is centralized.
Comment on lines
+1308
to
+1315
| let overallTimeout: Double = { | ||
| guard let s = timeoutStr, let v = Double(s), v > 0 else { return 0 } | ||
| return v | ||
| }() | ||
| let batchMs: Int = { | ||
| guard let s = batchStr, let v = Int(s) else { return 200 } | ||
| return max(0, v) | ||
| }() |
There was a problem hiding this comment.
Validate --timeout/--batch strictly to avoid silent infinite waits.
On Line [1309] and Line [1313], invalid numeric input silently changes behavior (--timeout abc becomes wait-forever). This is a high-impact CLI correctness issue.
Suggested fix
- let overallTimeout: Double = {
- guard let s = timeoutStr, let v = Double(s), v > 0 else { return 0 }
- return v
- }()
- let batchMs: Int = {
- guard let s = batchStr, let v = Int(s) else { return 200 }
- return max(0, v)
- }()
+ let overallTimeout: Double
+ if let timeoutStr {
+ guard let v = Double(timeoutStr), v >= 0 else {
+ throw CLIError(message: "--timeout must be a non-negative number")
+ }
+ overallTimeout = v
+ } else {
+ overallTimeout = 0
+ }
+
+ let batchMs: Int
+ if let batchStr {
+ guard let v = Int(batchStr), v >= 0 else {
+ throw CLIError(message: "--batch must be a non-negative integer")
+ }
+ batchMs = v
+ } else {
+ batchMs = 200
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLI/cmux.swift` around lines 1308 - 1315, When parsing timeoutStr and
batchStr for overallTimeout and batchMs, reject invalid numeric input instead of
silently falling back: if timeoutStr is non-nil but cannot be converted to a
positive Double (or is <= 0) print a clear error and exit non-zero (do not treat
as "wait forever"); likewise, if batchStr is non-nil but cannot be converted to
an Int (or is negative) print a clear error and exit; keep the existing default
behavior only when the flag is absent (timeoutStr == nil or batchStr == nil).
Update the parsing code that computes overallTimeout and batchMs to validate the
presence of timeoutStr/batchStr and fail fast on invalid values (referencing
overallTimeout, batchMs, timeoutStr, batchStr).
Comment on lines
+1321
to
+1324
| signal(SIGINT) { _ in | ||
| print("[]") | ||
| exit(0) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "cmux.swift" -type fRepository: LegendsOfLearning/cmux
Length of output: 82
🏁 Script executed:
# Find the file and examine the context around lines 1321-1324
if [ -f "CLI/cmux.swift" ]; then
sed -n '1310,1330p' CLI/cmux.swift | cat -n
fiRepository: LegendsOfLearning/cmux
Length of output: 1069
🌐 Web query:
Swift signal() handler async-signal-safe print() Darwin POSIX safety
💡 Result:
In a POSIX/Darwin signal()/sigaction() handler, you must only call async-signal-safe functions; anything that might allocate, lock, touch stdio buffers, or run Swift/ObjC runtime code is undefined behavior. (docs.oracle.com)
Is Swift print() async-signal-safe?
No. print() goes through Swift’s standard library I/O machinery and (directly or indirectly) can use non-reentrant facilities (buffering/locks/allocation). It is not async-signal-safe, so calling it in a signal handler is unsafe. (docs.oracle.com)
What can you do to “print” from a signal handler?
If you absolutely must emit something from inside the handler, stick to write(2) to a file descriptor (for example STDERR_FILENO), using a prebuilt/static byte buffer (don’t allocate or build a String in the handler). write() is on the POSIX async-signal-safe list. (docs.oracle.com)
Recommended pattern in Swift on Darwin
Do almost nothing in the handler; instead, notify normal code and do logging/printing there:
- Set a
volatile sig_atomic_tflag in the handler and poll it elsewhere. (cocoadev.github.io) - Or, on Apple platforms, prefer
DispatchSourceSignal(GCD) to receive signals on a dispatch queue and then safely call Swift/print()there. (blog.smittytone.net)
Also avoid in handlers (common gotchas)
- Don’t create Swift concurrency work (like
Task { ... }) from a handler. (forums.swift.org) - Don’t call
exit()from a crashing-signal handler; use_exit()if you truly must terminate (and for crash reporting, Apple recommends restoring default handling and returning instead). (developer.apple.com)
Citations:
- 1: https://docs.oracle.com/cd/E19455-01/806-5257/gen-26/index.html?utm_source=openai
- 2: https://docs.oracle.com/cd/E19455-01/806-5257/gen-26/index.html?utm_source=openai
- 3: https://docs.oracle.com/cd/E19455-01/806-5257/gen-26/index.html?utm_source=openai
- 4: https://cocoadev.github.io/SignalSafety/?utm_source=openai
- 5: https://blog.smittytone.net/2021/07/19/tackle-async-signal-safety-in-swift/?utm_source=openai
- 6: https://forums.swift.org/t/signal-handling-in-swift/61351?utm_source=openai
- 7: https://developer.apple.com/forums/thread/113742?utm_source=openai
Replace unsafe print() in signal handler with a safer pattern.
The print() call on line 1322 is not async-signal-safe and can cause crashes or deadlocks. Use one of these patterns instead:
- Flag-based approach (simplest): Set a
volatile sig_atomic_tflag in the handler and check/handle output in the main loop. - DispatchSourceSignal (preferred on Apple platforms): Receive the signal on a dispatch queue where
print()is safe.
Also use _exit() instead of exit() to avoid flushing buffers in the handler.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLI/cmux.swift` around lines 1321 - 1324, The signal handler for SIGINT
currently calls print() and exit(), both unsafe in a signal handler; replace
this by either (preferred) creating a DispatchSourceSignal for SIGINT and
perform printing and clean shutdown on its dispatch queue, or (simpler) make the
handler set a volatile sig_atomic_t flag (e.g., shutdownRequested) and return,
then check that flag in the main run loop to call print() and perform graceful
shutdown; also replace exit() with _exit() if any immediate process termination
is required inside a handler. Ensure you update the SIGINT setup (the closure
currently passed to signal(SIGINT) { ... }) and reference the new
DispatchSourceSignal or the sig_atomic_t flag and the location where the main
loop observes it.
Comment on lines
+4252
to
+4263
| Usage: cmux wait [--timeout <seconds>] [--batch <ms>] [--workspace <id|ref>] [--include-self] | ||
|
|
||
| Block until a new notification arrives. Prints a JSON array of notifications | ||
| and exits with code 0. If --timeout is reached with no notifications, prints | ||
| an empty array ([]) and exits with code 0. | ||
|
|
||
| Flags: | ||
| --timeout <seconds> Overall wait timeout (default: 0 = wait forever) | ||
| --batch <ms> After first notification, wait this long to collect | ||
| more before returning (default: 200) | ||
| --workspace <id|ref> Only wait for notifications in this workspace | ||
| --include-self Include notifications from the caller's own surface |
There was a problem hiding this comment.
Update wait help text to include index handles.
Line [4252], Line [4262], and Line [6366] under-document accepted workspace formats. The command accepts index too, not just id/ref.
Suggested doc fix
- Usage: cmux wait [--timeout <seconds>] [--batch <ms>] [--workspace <id|ref>] [--include-self]
+ Usage: cmux wait [--timeout <seconds>] [--batch <ms>] [--workspace <id|ref|index>] [--include-self]
@@
- --workspace <id|ref> Only wait for notifications in this workspace
+ --workspace <id|ref|index> Only wait for notifications in this workspace
@@
- wait [--timeout <seconds>] [--batch <ms>] [--workspace <id|ref>] [--include-self]
+ wait [--timeout <seconds>] [--batch <ms>] [--workspace <id|ref|index>] [--include-self]Also applies to: 6366-6366
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLI/cmux.swift` around lines 4252 - 4263, The help text for the "cmux wait"
command currently says "--workspace <id|ref>" but the command also accepts an
index; update the usage/help strings to reflect this by changing occurrences of
"<id|ref>" to "<id|ref|index>" (or similar) in the wait help block and the other
documented location mentioned (the string used for the wait command help and any
duplicated help at the other occurrence), ensuring the displayed Flags section
for "--workspace" now lists the accepted formats including index so both help
outputs (the one around the cmux wait usage and the duplicated one) are
consistent.
Comment on lines
+4717
to
+4718
| let workspaceFilter = v2UUID(params, "workspace_id") | ||
| let excludeSurface = v2UUID(params, "exclude_surface_id") |
There was a problem hiding this comment.
Validate filter IDs instead of silently widening the query scope.
On Line 4717 and Line 4718, invalid workspace_id / exclude_surface_id values currently fall back to nil, which can unexpectedly return unfiltered notifications.
🔧 Proposed fix
private func v2NotificationWait(params: [String: Any]) -> V2CallResult {
let pollTimeoutMs = min(max(v2Int(params, "poll_timeout_ms") ?? 10000, 100), 12000)
let batchMs = min(max(v2Int(params, "batch_ms") ?? 200, 0), 5000)
let workspaceFilter = v2UUID(params, "workspace_id")
let excludeSurface = v2UUID(params, "exclude_surface_id")
+ if params["workspace_id"] != nil && workspaceFilter == nil {
+ return .err(code: "invalid_params", message: "Missing or invalid workspace_id", data: nil)
+ }
+ if params["exclude_surface_id"] != nil && excludeSurface == nil {
+ return .err(code: "invalid_params", message: "Missing or invalid exclude_surface_id", data: nil)
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TerminalController.swift` around lines 4717 - 4718, The current use
of v2UUID(params, "workspace_id") and v2UUID(params, "exclude_surface_id") lets
invalid IDs silently become nil and widen the query; change this so that when
the params dictionary contains "workspace_id" or "exclude_surface_id" but v2UUID
returns nil, the handler rejects the request with a validation error (e.g., 400)
instead of treating it as absent. Locate the code that assigns workspaceFilter
and excludeSurface and add a check: if params.keys.contains("workspace_id") &&
workspaceFilter == nil (and similarly for exclude_surface_id) then return/throw
a validation/bad-request response with a clear message about the invalid UUID.
Ensure you reference the same v2UUID helper and preserve existing behavior only
for truly absent keys.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes manaflow-ai#598
notification.waitV2 socket endpoint with lease-based polling (10s server-side polls, 50ms check interval)cmux waitCLI command that loops lease requests until a notification arrives or--timeoutis reached--workspacefilter,--batchbatching window, and--include-selfto control self-exclusion viaCMUX_SURFACE_IDTest plan
Build and launch:
cmux wait --timeout 3prints[]after ~3scmux wait --timeout 30in one terminal, thencmux notify --title "test" --body "hello"in another — first terminal prints notification JSON and exitscmux wait --timeout 5should not see own notifications;cmux wait --timeout 5 --include-selfshouldcmux wait --workspace workspace:1 --timeout 10only triggers for workspace 1 notificationscmux wait --timeout 30prints[]and exits cleanlycmux wait --helpprints usage without connecting to socket🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
cmux waitcommand to poll for new notifications with configurable timeout and batch settings.