feat: integrate cmux-aware runtime with resilient notifications#2933
Draft
feat: integrate cmux-aware runtime with resilient notifications#2933
Conversation
Resolve tmux/cmux capability at startup so pane control remains tmux-driven while notifications prefer cmux and gracefully fall back to desktop notifications.
Collaborator
Author
|
@cubic-dev-ai review |
@kdcokenny I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 27 files
Confidence score: 3/5
- There is moderate regression risk: in
src/features/tmux-subagent/manager.ts, the legacyTmuxUtilDepsconstructor path now relies on global runtime cache, which can break dependency-injected callers and existing tmux-subagent tests. src/tools/interactive-bash/tmux-path-resolver.tshas two high-confidence behavior issues whereprobeCmuxReachability()andprobeTmuxRuntime()ignore the supplied environment, so runtime binary resolution can be wrong for user-configured envs.- Additional medium/low issues look fixable but real:
src/shared/tmux/tmux-utils/multiplexer-runtime.tsstill probes disabled backends at startup, and docs/tests indocs/reference/configuration.mdandsrc/hooks/cmux-notification-adapter.test.tsappear mismatched or incomplete rather than merge-blocking. - Pay close attention to
src/features/tmux-subagent/manager.ts,src/tools/interactive-bash/tmux-path-resolver.ts, andsrc/shared/tmux/tmux-utils/multiplexer-runtime.ts- they contain the concrete runtime-behavior regressions that most affect execution and startup behavior.
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="docs/reference/configuration.md">
<violation number="1" location="docs/reference/configuration.md:579">
P2: The mode table overstates cmux notification routing: `cmux-shim` and `cmux-notify-only` can still use desktop notifications when `cmux notify` is not capability-verified.
(Based on your team's feedback about focusing doc-only reviews on accuracy and consistency in the changed docs.) [FEEDBACK_USED]</violation>
<violation number="2" location="docs/reference/configuration.md:604">
P3: The new hint-strength note describes behavior the runtime does not implement: strong hints are recorded, but they do not preserve `cmux-shim` mode or override probe results.
(Based on your team's feedback about focusing doc-only reviews on accuracy and consistency in the changed docs.) [FEEDBACK_USED]</violation>
</file>
<file name="src/shared/tmux/tmux-utils/multiplexer-runtime.ts">
<violation number="1" location="src/shared/tmux/tmux-utils/multiplexer-runtime.ts:177">
P2: Skip tmux/cmux probes when the corresponding backend is disabled. Right now disabled runtimes still spawn external probe commands at startup.</violation>
</file>
<file name="src/hooks/cmux-notification-adapter.test.ts">
<violation number="1" location="src/hooks/cmux-notification-adapter.test.ts:107">
P2: This test does not actually cover connection-refused handling; any non-zero cmux exit already downgrades, so it will pass even if the connection-refused check breaks.</violation>
</file>
<file name="src/features/tmux-subagent/manager.ts">
<violation number="1" location="src/features/tmux-subagent/manager.ts:118">
P2: The legacy `TmuxUtilDeps` constructor path now depends on the global runtime cache, which breaks dependency-injected callers and current tmux-subagent tests.</violation>
</file>
<file name="src/tools/interactive-bash/tmux-path-resolver.ts">
<violation number="1" location="src/tools/interactive-bash/tmux-path-resolver.ts:359">
P2: `probeTmuxRuntime()` ignores the supplied environment when resolving the tmux binary.</violation>
<violation number="2" location="src/tools/interactive-bash/tmux-path-resolver.ts:416">
P2: `probeCmuxReachability()` ignores the supplied environment when resolving the cmux binary.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Collaborator
Author
|
@cubic-dev-ai review |
@kdcokenny I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
4 issues found across 28 files
Confidence score: 3/5
- There is some concrete regression risk in tmux behavior: in
src/openclaw/tmux.ts,tmuxSessionis derived fromTMUX_PANE, which can misidentify the actual session and affect session-scoped logic. - In
src/tools/interactive-bash/tmux-path-resolver.ts, not returning immediately after timeout (post-kill stdout/stderr reads) could cause unstable probe handling under timeout conditions. - Also in
src/tools/interactive-bash/tmux-path-resolver.ts, custom environment probing may ignore intentional PATH removal, which can lead to incorrect command resolution in user-specified env setups. - Pay close attention to
src/openclaw/tmux.ts,src/tools/interactive-bash/tmux-path-resolver.ts, anddocs/reference/configuration.md- tmux session identification, timeout/path probing behavior, and user-facing config semantics need verification.
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="src/tools/interactive-bash/tmux-path-resolver.ts">
<violation number="1" location="src/tools/interactive-bash/tmux-path-resolver.ts:243">
P2: Return immediately on timeout instead of reading stdout/stderr after killing the probe process.</violation>
<violation number="2" location="src/tools/interactive-bash/tmux-path-resolver.ts:269">
P2: Honor an explicit PATH removal when probing a custom environment.</violation>
</file>
<file name="src/openclaw/tmux.ts">
<violation number="1" location="src/openclaw/tmux.ts:22">
P2: `tmuxSession` is being derived from `TMUX_PANE`, so it now reports a pane-based identifier instead of the actual tmux session.</violation>
</file>
<file name="docs/reference/configuration.md">
<violation number="1" location="docs/reference/configuration.md:581">
P3: `cmux-notify-only` is documented too narrowly; it also occurs when tmux exists but pane control is unavailable.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Collaborator
Author
|
@cubic-dev-ai review |
@kdcokenny I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
3 issues found across 31 files
Confidence score: 3/5
- There are a few concrete user-impacting risks (severity 5–6/10), so this isn’t a no-brainer merge despite being fixable.
- Most significant:
src/hooks/cmux-notification-adapter.tscan still hang past the configured timeout because it waits on stdout/stderr after killingcmux notify. src/tools/interactive-bash/tools.tsexports aninteractive_bashsnapshot that can permanently report pane control unavailable, which can mislead direct consumers.- Pay close attention to
src/hooks/cmux-notification-adapter.ts,src/tools/interactive-bash/tools.ts,src/tools/interactive-bash/tmux-path-resolver.test.ts- timeout hang, stale singleton state, and Windows-incompatible test guard.
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="src/tools/interactive-bash/tmux-path-resolver.test.ts">
<violation number="1" location="src/tools/interactive-bash/tmux-path-resolver.test.ts:85">
P2: Guard this POSIX-only timeout test on Windows, or it will break supported Windows test runs.</violation>
</file>
<file name="src/tools/interactive-bash/tools.ts">
<violation number="1" location="src/tools/interactive-bash/tools.ts:156">
P2: The exported `interactive_bash` singleton snapshots a disabled runtime during module import, so direct consumers will always see pane control as unavailable.</violation>
</file>
<file name="src/hooks/cmux-notification-adapter.ts">
<violation number="1" location="src/hooks/cmux-notification-adapter.ts:86">
P2: Return immediately on timeout instead of waiting on stdout/stderr after killing `cmux notify`. Otherwise the fallback path can still hang past the configured timeout.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Collaborator
Author
|
@cubic-dev-ai review |
@kdcokenny I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
3 issues found across 32 files
Confidence score: 3/5
- There is a concrete user-impact risk in
src/hooks/session-notification.ts: failed cmux sends on unsupported platforms can be marked as delivered, which may suppress later idle notifications the user should receive. - The issues in
src/tools/interactive-bash/tools.test.tsandsrc/openclaw/__tests__/tmux.test.tsare test-quality gaps that can let runtime/tmux regressions slip through, increasing uncertainty for this change set. - Given a medium-severity behavior bug (6/10, high confidence) plus weaker regression coverage, this looks mergeable with caution rather than low-risk.
- Pay close attention to
src/hooks/session-notification.ts,src/tools/interactive-bash/tools.test.ts,src/openclaw/__tests__/tmux.test.ts- notification delivery state and tmux/runtime regression paths need tighter validation.
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="src/tools/interactive-bash/tools.test.ts">
<violation number="1" location="src/tools/interactive-bash/tools.test.ts:55">
P2: This assertion is too weak to verify runtime resolution; it still passes when `interactive_bash.execute()` fails for unrelated tmux/path errors.</violation>
</file>
<file name="src/hooks/session-notification.ts">
<violation number="1" location="src/hooks/session-notification.ts:113">
P2: A failed cmux send on unsupported platforms is treated as a delivered idle notification, so later idle notifications for that session can be suppressed even though the user never saw one.</violation>
</file>
<file name="src/openclaw/__tests__/tmux.test.ts">
<violation number="1" location="src/openclaw/__tests__/tmux.test.ts:29">
P2: This test is bypassing the regression path by disabling tmux, so it only proves that disabled tmux returns `null`, not that stale `TMUX_PANE` is ignored.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Collaborator
Author
|
@cubic-dev-ai review |
@kdcokenny I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
4 issues found across 33 files
Confidence score: 3/5
- There is a concrete user-impacting risk in
src/tools/interactive-bash/tools.ts: the tmux guard can block valid detached commands when not already in a controllable pane, which could break expected command execution flows. src/hooks/session-notification-scheduler.tshas a moderate regression risk where stale-version classification may be wrong after eviction, potentially causing duplicate session notifications to be resent.- This lands at a moderate merge risk (rather than high) because the top issue is specific and fixable, while the remaining findings are one medium logic concern plus low-severity test gaps in
src/shared/tmux/tmux-utils/multiplexer-runtime.test.ts. - Pay close attention to
src/tools/interactive-bash/tools.ts,src/hooks/session-notification-scheduler.ts, andsrc/shared/tmux/tmux-utils/multiplexer-runtime.test.ts- tmux command gating and notification versioning may regress behavior, and current tests may not catch related parser/platform issues.
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="src/shared/tmux/tmux-utils/multiplexer-runtime.test.ts">
<violation number="1" location="src/shared/tmux/tmux-utils/multiplexer-runtime.test.ts:181">
P3: This test hardcodes the relay classification instead of verifying it, so a broken `host:port` endpoint parser would go unnoticed.</violation>
<violation number="2" location="src/shared/tmux/tmux-utils/multiplexer-runtime.test.ts:192">
P3: This test does not actually exercise the non-mac Ghostty hint logic; it hardcodes the probe result, so platform-specific regressions would still pass.</violation>
</file>
<file name="src/tools/interactive-bash/tools.ts">
<violation number="1" location="src/tools/interactive-bash/tools.ts:71">
P1: This guard blocks valid detached tmux commands whenever the process is not already inside a controllable tmux pane.</violation>
</file>
<file name="src/hooks/session-notification-scheduler.ts">
<violation number="1" location="src/hooks/session-notification-scheduler.ts:142">
P2: This version check can misclassify an evicted entry as stale and resend notifications that were already delivered.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Collaborator
Author
|
@cubic-dev-ai review |
@kdcokenny I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
5 issues found across 34 files
Confidence score: 3/5
- There is some concrete regression risk in
src/tools/interactive-bash/tools.ts: the pane-control allowlist can be bypassed by-t/-d-prefixed values, which may let tmux-specific behavior slip past thepaneBackend !== "tmux"guard. src/hooks/session-notification.tshas user-facing reliability impact: after one cmux failure on unsupported OSes, idle notifications stay disabled instead of retrying when cmux recovers.- The remaining findings are moderate and mostly correctness/maintenance focused (
src/tools/interactive-bash/tmux-path-resolver.test.ts,src/hooks/session-notification.test.ts,docs/reference/configuration.md), but they reinforce uncertainty around notification fallback behavior and test realism. - Pay close attention to
src/tools/interactive-bash/tools.ts,src/hooks/session-notification.ts,src/tools/interactive-bash/tmux-path-resolver.test.ts,src/hooks/session-notification.test.ts, anddocs/reference/configuration.md- guard bypass, persistent notification disablement, subprocess leak in tests, mismatched retry expectation, and overstated fallback docs.
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="src/tools/interactive-bash/tools.ts">
<violation number="1" location="src/tools/interactive-bash/tools.ts:90">
P2: The new pane-control allowlist is too permissive: argument values that start with `-t`/`-d` can bypass the `paneBackend !== "tmux"` guard because the flag scan ignores `--` and option position.</violation>
</file>
<file name="src/hooks/session-notification.test.ts">
<violation number="1" location="src/hooks/session-notification.test.ts:557">
P2: This assertion codifies a second idle retry even though a downgraded cmux adapter can never deliver on an unsupported platform.</violation>
</file>
<file name="docs/reference/configuration.md">
<violation number="1" location="docs/reference/configuration.md:590">
P2: This overstates the fallback guarantee: on unsupported platforms, a cmux failure does not fall back to desktop notifications.</violation>
</file>
<file name="src/hooks/session-notification.ts">
<violation number="1" location="src/hooks/session-notification.ts:186">
P2: After one cmux failure on an unsupported OS, idle notifications are permanently disabled instead of retrying when cmux recovers.</violation>
</file>
<file name="src/tools/interactive-bash/tmux-path-resolver.test.ts">
<violation number="1" location="src/tools/interactive-bash/tmux-path-resolver.test.ts:94">
P2: This timeout fixture ignores SIGTERM, so the test leaks a subprocess after the probe returns.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| (subcommand === "new-session" || subcommand === "new") | ||
| && hasDetachedFlag(tokens) | ||
|
|
||
| return isDetachedNewSession || hasTmuxTargetFlag(tokens) |
There was a problem hiding this comment.
P2: The new pane-control allowlist is too permissive: argument values that start with -t/-d can bypass the paneBackend !== "tmux" guard because the flag scan ignores -- and option position.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/tools/interactive-bash/tools.ts, line 90:
<comment>The new pane-control allowlist is too permissive: argument values that start with `-t`/`-d` can bypass the `paneBackend !== "tmux"` guard because the flag scan ignores `--` and option position.</comment>
<file context>
@@ -48,34 +53,86 @@ export function tokenizeCommand(cmd: string): string[] {
+ (subcommand === "new-session" || subcommand === "new")
+ && hasDetachedFlag(tokens)
+
+ return isDetachedNewSession || hasTmuxTargetFlag(tokens)
+}
+
</file context>
|
|
||
| await new Promise((resolve) => setTimeout(resolve, 50)) | ||
|
|
||
| expect(cmuxSendCalls).toBe(2) |
There was a problem hiding this comment.
P2: This assertion codifies a second idle retry even though a downgraded cmux adapter can never deliver on an unsupported platform.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/hooks/session-notification.test.ts, line 557:
<comment>This assertion codifies a second idle retry even though a downgraded cmux adapter can never deliver on an unsupported platform.</comment>
<file context>
@@ -387,6 +425,233 @@ describe("session-notification", () => {
+
+ await new Promise((resolve) => setTimeout(resolve, 50))
+
+ expect(cmuxSendCalls).toBe(2)
+ expect(cmuxNotifyCommandCalls).toBe(1)
+ expect(notificationCalls).toHaveLength(0)
</file context>
Suggested change
| expect(cmuxSendCalls).toBe(2) | |
| expect(cmuxSendCalls).toBe(1) |
|
|
||
| **Notification Backend**: | ||
| - Cmux-first when the runtime detects a live, notification-capable cmux endpoint | ||
| - Silent fallback to desktop notifications on any failure (non-zero exit, timeout, connection refused) |
There was a problem hiding this comment.
P2: This overstates the fallback guarantee: on unsupported platforms, a cmux failure does not fall back to desktop notifications.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/reference/configuration.md, line 590:
<comment>This overstates the fallback guarantee: on unsupported platforms, a cmux failure does not fall back to desktop notifications.</comment>
<file context>
@@ -565,6 +566,61 @@ Run background subagents in separate tmux panes. Requires running inside tmux wi
+
+**Notification Backend**:
+- Cmux-first when the runtime detects a live, notification-capable cmux endpoint
+- Silent fallback to desktop notifications on any failure (non-zero exit, timeout, connection refused)
+- Once downgraded to desktop, cmux notifications remain disabled for the session
+
</file context>
Suggested change
| - Silent fallback to desktop notifications on any failure (non-zero exit, timeout, connection refused) | |
| - Fallback to desktop notifications on supported desktop platforms when cmux send fails (non-zero exit, timeout, connection refused) |
| currentPlatform === "unsupported" && !cmuxNotificationAdapter.canSendViaCmux() | ||
| const shouldFastExitUnsupportedEvent = | ||
| cannotDeliverOnUnsupportedPlatform | ||
| && (event.type !== "session.idle" || !cmuxNotificationAdapter.hasDowngraded()) |
There was a problem hiding this comment.
P2: After one cmux failure on an unsupported OS, idle notifications are permanently disabled instead of retrying when cmux recovers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/hooks/session-notification.ts, line 186:
<comment>After one cmux failure on an unsupported OS, idle notifications are permanently disabled instead of retrying when cmux recovers.</comment>
<file context>
@@ -134,7 +179,13 @@ export function createSessionNotification(
+ currentPlatform === "unsupported" && !cmuxNotificationAdapter.canSendViaCmux()
+ const shouldFastExitUnsupportedEvent =
+ cannotDeliverOnUnsupportedPlatform
+ && (event.type !== "session.idle" || !cmuxNotificationAdapter.hasDowngraded())
+
+ if (shouldFastExitUnsupportedEvent) return
</file context>
| const fakeCmuxPath = join(tempDirectory, "cmux") | ||
| const slowCmuxScript = `#!/bin/sh | ||
| if [ "$1" = "notify" ]; then | ||
| trap '' TERM |
There was a problem hiding this comment.
P2: This timeout fixture ignores SIGTERM, so the test leaks a subprocess after the probe returns.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/tools/interactive-bash/tmux-path-resolver.test.ts, line 94:
<comment>This timeout fixture ignores SIGTERM, so the test leaks a subprocess after the probe returns.</comment>
<file context>
@@ -0,0 +1,166 @@
+ const fakeCmuxPath = join(tempDirectory, "cmux")
+ const slowCmuxScript = `#!/bin/sh
+if [ "$1" = "notify" ]; then
+ trap '' TERM
+ /bin/sleep 1
+ exit 0
</file context>
|
How it differs from this solution? https://github.com/0xCaso/opencode-cmux |
Collaborator
Author
This builds it into OMO natively. Does that plugin integrate with OMO well for you? It tends to break external plugins. |
It works quite nicely, honestly. I get all the notifications working and the data information for workspace and the splitting also works pretty well. Only thing that don't work is the notification script OMO uses, so for those notifications I just get the in app modal inside opencode. |
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
2 participants
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
Summary by cubic
Integrates a cmux-aware multiplexer runtime with cmux-first notifications and resilient fallback while keeping tmux as the pane backend. Adds a scheduler fix to prevent duplicate sends and tightens runtime gating and timeouts.
New Features
cmux-shim,tmux-only,cmux-notify-only,none) at startup and passresolvedMultiplexerthrough managers, hooks, and tools.host:portsupport, connection-refused detection, and desktop fallback with permanent downgrade.createInteractiveBashTool(resolvedMultiplexer)uses the dynamic tmux path and gives clear errors when tmux control isn’t available.Bug Fixes
send()now returns a boolean to mark actual delivery.openclaw, tools).openclaw(via tmux command, not env parsing) and tuned probes/timeouts for fewer flakes.Written for commit 2b02e2c. Summary will update on new commits.