ssh-tmux: deliver OSC 777/9 notifications from mirrored panes (#833) - #10048
Open
alloevil wants to merge 2 commits into
Open
ssh-tmux: deliver OSC 777/9 notifications from mirrored panes (#833)#10048alloevil wants to merge 2 commits into
alloevil wants to merge 2 commits into
Conversation
…ow-ai#833) In a pure ssh-tmux mirror every remote shell runs inside tmux, so the shell integration only ever pulls CMUX_* values and nothing publishes; the tmux session environment goes permanently stale after an app relaunch, and notification escapes emitted by remote agents are parsed by a mirror surface that has no local process TTY to attribute them by. The failure is silent: agent hooks report success and nothing arrives. Two changes, both in the mirror layer (no remote daemon, no user tmux config required): - Push mirror identity into the remote tmux SESSION environment (set-environment -t) at the post-attach drain point, covering first connect and every reconnect. Session scope is deliberate: the shell-integration refresh path runs a session-scoped show-environment, which does not surface -g values. CMUX_SOCKET_PATH is deliberately NOT pushed - the ssh-tmux transport has no relay, so a local socket path would be dead on the remote. - Intercept OSC 777;notify and OSC 9 sequences in the mirrored pane output stream (RemoteTmuxNotificationOSCFilter, a chunk-split-safe FSM with a 4KB unfinished-sequence ceiling), strip them, and deliver through the shared GhosttyDesktopNotificationIngress attributed to the pane's surface and workspace. Reseed replay strips but suppresses delivery so a reconnect cannot re-fire old notifications. Pane identity is ground truth in mirror mode; launch-time env vars never were. Fixes manaflow-ai#833 for ssh-tmux mirrors; plain `cmux ssh` workspaces are unaffected.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds session-scoped environment propagation for remote tmux mirrors and filters OSC 777/9 notifications from remote tmux output. Notifications are routed per pane to the mirrored workspace and surface. Tests cover command generation, reconnects, parsing, chunk boundaries, passthrough, and buffering limits. ChangesMirror environment propagation
Remote tmux notifications
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: ⚪ Minimal · up to This change adds mirrored-pane notifications and refreshes remote tmux session identity; no actionable merge-blocking risk remains beyond normal checks and review. Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Remote tmux
participant RemoteTmuxSessionMirror
participant RemoteTmuxNotificationOSCFilter
participant Notification ingress
Remote tmux->>RemoteTmuxSessionMirror: Send pane output
RemoteTmuxSessionMirror->>RemoteTmuxNotificationOSCFilter: Filter output bytes
RemoteTmuxNotificationOSCFilter->>RemoteTmuxSessionMirror: Return filtered bytes and decoded notification
RemoteTmuxSessionMirror->>Notification ingress: Deliver live notification with workspace and surface
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors)
✅ Passed checks (22 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
4 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/RemoteTmuxNotificationOSCFilterTests.swift`:
- Around line 224-251: Add a test alongside the oversized and maximum-size
notification tests that constructs a complete candidate whose total raw length,
including the ESC and ] prefix bytes, is exactly
RemoteTmuxNotificationOSCFilter.maxBufferedBytes. Assert it is stripped
successfully and produces the expected notification, pinning the
reclassifyCandidate boundary behavior without changing existing oversized cases.
In `@Sources/RemoteTmuxControlConnection`+Commands.swift:
- Around line 44-48: Update the mirror-session command flow around
mirrorEnvironmentCommands to include a set-environment -u command removing
CMUX_SOCKET_PATH for the target in the same batch, while preserving the existing
environment assignments. Extend the relevant test to seed CMUX_SOCKET_PATH and
assert that the mirror operation removes it.
In `@Sources/RemoteTmuxNotificationOSCFilter.swift`:
- Around line 44-45: Update Sources/RemoteTmuxNotificationOSCFilter.swift lines
44-45 by defining the OSC 9 progress prefix 9;4; and excluding it in both
finishCandidate and reclassifyCandidate so progress sequences pass through
unchanged. Update cmuxTests/RemoteTmuxNotificationOSCFilterTests.swift lines
155-160 by adding a counterpart to nonNotifyOsc777SubcommandPassesVerbatim that
verifies ESC ] 9;4;1;50 BEL is preserved verbatim and produces no notification.
In `@Sources/RemoteTmuxSessionMirror.swift`:
- Around line 122-125: Update rebuildTopology to prune notificationFilters
alongside titleFilters, retaining only entries whose pane IDs are present in
livePanes. Place the cleanup next to the existing titleFilters filtering and
preserve the disconnect removeAll behavior.
In `@Sources/RemoteTmuxSessionMirror`+OutputRouting.swift:
- Around line 7-10: Replace the type-level paneNotificationIngress instance with
the existing GhosttyApp.desktopNotificationIngress, threading that ingress
through the mirror composition path and updating affected callers or
initializers. Preserve the shared delivery, retargeting, and flood-policy
behavior, and rely on the property’s existing `@MainActor` isolation without
adding actor isolation to GhosttyDesktopNotificationIngress.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 213f6dc6-83cd-4735-9c89-75dd9d8bf619
📒 Files selected for processing (10)
Sources/RemoteTmuxControlConnection+CommandResults.swiftSources/RemoteTmuxControlConnection+Commands.swiftSources/RemoteTmuxControlConnection.swiftSources/RemoteTmuxController.swiftSources/RemoteTmuxNotificationOSCFilter.swiftSources/RemoteTmuxSessionMirror+OutputRouting.swiftSources/RemoteTmuxSessionMirror.swiftcmux.xcodeproj/project.pbxprojcmuxTests/RemoteTmuxMirrorEnvironmentPushTests.swiftcmuxTests/RemoteTmuxNotificationOSCFilterTests.swift
Comment on lines
+224
to
+251
| @Test func oversizedUnfinishedCandidatePassesVerbatim() { | ||
| // A prefix-compatible sequence that exceeds the ceiling before its | ||
| // terminator is flushed verbatim — never stripped, never retained. | ||
| let hugeBody = String(repeating: "A", count: RemoteTmuxNotificationOSCFilter.maxBufferedBytes + 16) | ||
| let input = "\(ESC)]9;\(hugeBody)\(BEL)after" | ||
| let result = run(input) | ||
| #expect(result.output == bytes(input)) | ||
| #expect(result.notifications.isEmpty) | ||
| } | ||
|
|
||
| @Test func oversizedCandidateSplitAcrossChunksPassesVerbatim() { | ||
| let hugeBody = String(repeating: "B", count: RemoteTmuxNotificationOSCFilter.maxBufferedBytes) | ||
| let result = run([ | ||
| "\(ESC)]777;notify;t;", | ||
| hugeBody, | ||
| "tail\(BEL)done", | ||
| ]) | ||
| #expect(result.output == bytes("\(ESC)]777;notify;t;\(hugeBody)tail\(BEL)done")) | ||
| #expect(result.notifications.isEmpty) | ||
| } | ||
|
|
||
| @Test func maxSizedCompleteNotificationStillStrips() { | ||
| // Just under the ceiling must still work. | ||
| let body = String(repeating: "C", count: RemoteTmuxNotificationOSCFilter.maxBufferedBytes - 64) | ||
| let result = run("\(ESC)]9;\(body)\(BEL)") | ||
| #expect(result.output == bytes("")) | ||
| #expect(result.notifications.first?.body == body) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Pin the exact ceiling boundary.
reclassifyCandidate compares raw.count (payload plus the two ESC ] bytes) against maxBufferedBytes. The current tests probe maxBufferedBytes + 16, maxBufferedBytes, and maxBufferedBytes - 64, so an off-by-one in that comparison stays undetected. Add a case whose candidate ends exactly at the ceiling.
♻️ Proposed test for the exact ceiling
`@Test` func maxSizedCompleteNotificationStillStrips() {
// Just under the ceiling must still work.
let body = String(repeating: "C", count: RemoteTmuxNotificationOSCFilter.maxBufferedBytes - 64)
let result = run("\(ESC)]9;\(body)\(BEL)")
`#expect`(result.output == bytes(""))
`#expect`(result.notifications.first?.body == body)
}
+
+ `@Test` func candidateEndingExactlyAtCeilingStillStrips() {
+ // `raw` counts the two `ESC ]` bytes plus `9;` plus the body, so this
+ // candidate's last buffered byte lands exactly on the ceiling.
+ let body = String(
+ repeating: "D",
+ count: RemoteTmuxNotificationOSCFilter.maxBufferedBytes - 4
+ )
+ let result = run("\(ESC)]9;\(body)\(BEL)")
+ `#expect`(result.output == bytes(""))
+ `#expect`(result.notifications.first?.body == body)
+ }📝 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
| @Test func oversizedUnfinishedCandidatePassesVerbatim() { | |
| // A prefix-compatible sequence that exceeds the ceiling before its | |
| // terminator is flushed verbatim — never stripped, never retained. | |
| let hugeBody = String(repeating: "A", count: RemoteTmuxNotificationOSCFilter.maxBufferedBytes + 16) | |
| let input = "\(ESC)]9;\(hugeBody)\(BEL)after" | |
| let result = run(input) | |
| #expect(result.output == bytes(input)) | |
| #expect(result.notifications.isEmpty) | |
| } | |
| @Test func oversizedCandidateSplitAcrossChunksPassesVerbatim() { | |
| let hugeBody = String(repeating: "B", count: RemoteTmuxNotificationOSCFilter.maxBufferedBytes) | |
| let result = run([ | |
| "\(ESC)]777;notify;t;", | |
| hugeBody, | |
| "tail\(BEL)done", | |
| ]) | |
| #expect(result.output == bytes("\(ESC)]777;notify;t;\(hugeBody)tail\(BEL)done")) | |
| #expect(result.notifications.isEmpty) | |
| } | |
| @Test func maxSizedCompleteNotificationStillStrips() { | |
| // Just under the ceiling must still work. | |
| let body = String(repeating: "C", count: RemoteTmuxNotificationOSCFilter.maxBufferedBytes - 64) | |
| let result = run("\(ESC)]9;\(body)\(BEL)") | |
| #expect(result.output == bytes("")) | |
| #expect(result.notifications.first?.body == body) | |
| } | |
| @Test func oversizedUnfinishedCandidatePassesVerbatim() { | |
| // A prefix-compatible sequence that exceeds the ceiling before its | |
| // terminator is flushed verbatim — never stripped, never retained. | |
| let hugeBody = String(repeating: "A", count: RemoteTmuxNotificationOSCFilter.maxBufferedBytes + 16) | |
| let input = "\(ESC)]9;\(hugeBody)\(BEL)after" | |
| let result = run(input) | |
| #expect(result.output == bytes(input)) | |
| #expect(result.notifications.isEmpty) | |
| } | |
| @Test func oversizedCandidateSplitAcrossChunksPassesVerbatim() { | |
| let hugeBody = String(repeating: "B", count: RemoteTmuxNotificationOSCFilter.maxBufferedBytes) | |
| let result = run([ | |
| "\(ESC)]777;notify;t;", | |
| hugeBody, | |
| "tail\(BEL)done", | |
| ]) | |
| #expect(result.output == bytes("\(ESC)]777;notify;t;\(hugeBody)tail\(BEL)done")) | |
| #expect(result.notifications.isEmpty) | |
| } | |
| @Test func maxSizedCompleteNotificationStillStrips() { | |
| // Just under the ceiling must still work. | |
| let body = String(repeating: "C", count: RemoteTmuxNotificationOSCFilter.maxBufferedBytes - 64) | |
| let result = run("\(ESC)]9;\(body)\(BEL)") | |
| #expect(result.output == bytes("")) | |
| #expect(result.notifications.first?.body == body) | |
| } | |
| @Test func candidateEndingExactlyAtCeilingStillStrips() { | |
| // `raw` counts the two `ESC ]` bytes plus `9;` plus the body, so this | |
| // candidate's last buffered byte lands exactly on the ceiling. | |
| let body = String( | |
| repeating: "D", | |
| count: RemoteTmuxNotificationOSCFilter.maxBufferedBytes - 4 | |
| ) | |
| let result = run("\(ESC)]9;\(body)\(BEL)") | |
| #expect(result.output == bytes("")) | |
| #expect(result.notifications.first?.body == body) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmuxTests/RemoteTmuxNotificationOSCFilterTests.swift` around lines 224 - 251,
Add a test alongside the oversized and maximum-size notification tests that
constructs a complete candidate whose total raw length, including the ESC and ]
prefix bytes, is exactly RemoteTmuxNotificationOSCFilter.maxBufferedBytes.
Assert it is stripped successfully and produces the expected notification,
pinning the reclassifyCandidate boundary behavior without changing existing
oversized cases.
Comment on lines
+44
to
+48
| var pairs = mirrorEnvironment | ||
| pairs[Self.mirrorMarkerEnvironmentKey] = "1" | ||
| let commands = Self.mirrorEnvironmentCommands(target: target, pairs: pairs) | ||
| guard !commands.isEmpty else { return } | ||
| _ = sendBatchInternal(commands, kinds: commands.map { _ in .other }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
socket="cmux-env-review-$$"
session="cmux-env-review-$$"
trap 'tmux -L "$socket" kill-server 2>/dev/null || true' EXIT
tmux -L "$socket" new-session -d -s "$session"
tmux -L "$socket" set-environment -t "$session" CMUX_SOCKET_PATH "/tmp/dead.sock"
tmux -L "$socket" show-environment -t "$session" | grep -Fx "CMUX_SOCKET_PATH=/tmp/dead.sock"
tmux -L "$socket" set-environment -u -t "$session" CMUX_SOCKET_PATH"
! tmux -L "$socket" show-environment -t "$session" | grep -q '^CMUX_SOCKET_PATH='Repository: manaflow-ai/cmux
Length of output: 196
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate files ---'
git ls-files | rg 'RemoteTmuxControlConnection|Tmux|tmux|Environment|Tests' | head -200
printf '%s\n' '--- symbols and references ---'
rg -n -C 4 \
'CMUX_SOCKET_PATH|mirrorEnvironmentCommands|pushMirrorSessionEnvironment|mirrorEnvironment|sendBatchInternal' \
Sources Tests . 2>/dev/null | head -500
printf '%s\n' '--- target file outline ---'
if command -v ast-grep >/dev/null 2>&1; then
ast-grep outline Sources/RemoteTmuxControlConnection+Commands.swift
else
wc -l Sources/RemoteTmuxControlConnection+Commands.swift
fi
printf '%s\n' '--- target implementation ---'
sed -n '1,130p' Sources/RemoteTmuxControlConnection+Commands.swiftRepository: manaflow-ai/cmux
Length of output: 18889
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate files ---'
git ls-files | rg 'RemoteTmuxControlConnection|Tmux|tmux|Environment|Tests' | head -200
printf '%s\n' '--- symbols and references ---'
rg -n -C 4 \
'CMUX_SOCKET_PATH|mirrorEnvironmentCommands|pushMirrorSessionEnvironment|mirrorEnvironment|sendBatchInternal' \
Sources Tests . 2>/dev/null | head -500
printf '%s\n' '--- target implementation ---'
sed -n '1,130p' Sources/RemoteTmuxControlConnection+Commands.swiftRepository: manaflow-ai/cmux
Length of output: 18889
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- target file status ---'
ls -l Sources/RemoteTmuxControlConnection+Commands.swift 2>&1 || true
wc -l Sources/RemoteTmuxControlConnection+Commands.swift 2>&1 || true
printf '%s\n' '--- target implementation ---'
sed -n '1,140p' Sources/RemoteTmuxControlConnection+Commands.swift 2>&1 || true
printf '%s\n' '--- matching paths ---'
git ls-files | grep -Ei 'remote.*tmux|tmux.*connection|tmux' | head -300 || true
printf '%s\n' '--- all relevant references ---'
grep -RIn --exclude-dir=.git --exclude-dir=Packages --exclude='*.jsonl' \
-E 'CMUX_SOCKET_PATH|mirrorEnvironmentCommands|pushMirrorSessionEnvironment|mirrorEnvironment|sendBatchInternal' \
Sources CLI Tests 2>/dev/null | head -500 || trueRepository: manaflow-ai/cmux
Length of output: 32254
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- environment push tests ---'
sed -n '1,260p' cmuxTests/RemoteTmuxMirrorEnvironmentPushTests.swift
printf '%s\n' '--- environment state and setter ---'
sed -n '170,225p' Sources/RemoteTmuxControlConnection.swift
sed -n '590,630p' Sources/RemoteTmuxControlConnection.swift
printf '%s\n' '--- push call sites ---'
sed -n '220,260p' Sources/RemoteTmuxControlConnection+CommandResults.swift
sed -n '290,325p' Sources/RemoteTmuxController.swift
printf '%s\n' '--- remote shell integration consumers ---'
rg -n -C 6 \
'show-environment|CMUX_REMOTE_TMUX_MIRROR|CMUX_WORKSPACE_ID|CMUX_SURFACE_ID|CMUX_TERMINAL_LIFECYCLE_ID' \
Sources CLI cmuxTests 2>/dev/null | head -700 || trueRepository: manaflow-ai/cmux
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- exact mirror marker references ---'
rg -n -C 12 'CMUX_REMOTE_TMUX_MIRROR' . 2>/dev/null | head -250 || true
printf '%s\n' '--- environment setter call sites ---'
rg -n -C 8 'setMirrorEnvironment\(' Sources cmuxTests 2>/dev/null | head -350 || true
printf '%s\n' '--- remote tmux attach/bootstrap references ---'
rg -n -C 8 \
'tmux -CC|ssh-tmux|RemoteTmux|remote tmux|control-mode|attach-session|new-session' \
Sources/RemoteTmux*.swift Sources/SSH*.swift Sources/TerminalController+RemoteTmux.swift 2>/dev/null \
| head -600 || true
printf '%s\n' '--- repository state ---'
git status --short
git diff --stat -- Sources/RemoteTmuxControlConnection+Commands.swift Sources/RemoteTmuxControlConnection.swift Sources/RemoteTmuxControlConnection+CommandResults.swift Sources/RemoteTmuxController.swift cmuxTests/RemoteTmuxMirrorEnvironmentPushTests.swiftRepository: manaflow-ai/cmux
Length of output: 50372
🌐 Web query:
tmux man page set-environment -u session environment variable show-environment
💡 Result:
In tmux, environment variables are managed through two levels: the global environment and the session-specific environment [1][2]. When the tmux server starts, it initializes the global environment [1][3]. When a new window or pane is created, tmux merges the global and current session environments—with the session-specific value taking precedence if a variable exists in both—to form the environment for that new process [1][2]. Key commands for managing these variables include: set-environment (alias: setenv) This command is used to set, unset, or modify environment variables [1]. Usage: tmux set-environment [-gru] [-t target-session] name [value] [1] -g: Applies the change to the global environment [1][4]. Without this flag, it applies to the specified session (or the current one) [1][4]. -u: Unsets (removes) the specified variable [1]. -r: Indicates the variable should be removed from the environment before starting a new process [1]. show-environment (alias: showenv) This command displays the environment variables for a specific session or the global environment [1][4]. Usage: tmux show-environment [-g] [-t target-session] [name] [1] -g: Displays the global environment [1][4]. -s: (Often used) Formats the output as shell commands (e.g., export VAR=val), which can be evaluated directly by a shell to update the current environment [5]. Important Considerations: 1. Retroactive Updates: Commands like set-environment only affect new windows, panes, or processes created after the command is run [6][7]. They do not retroactively modify the environment of already running shells [6][7]. 2. Synchronizing Existing Shells: To update an existing shell within tmux, you can manually source the output of the show-environment command [7][5]. A common technique is to use a shell hook, such as PROMPT_COMMAND in Bash, to automatically run eval "$(tmux show-environment -s)" each time the prompt is displayed [5]. 3. update-environment: This is a session option that controls which environment variables are automatically copied from the client into the session environment when a new session is created or reattached [1][7]. You can modify this list using set-option -g update-environment "VAR1 VAR2" [7][2].
Citations:
- 1: https://linux.die.net/man/1/tmux
- 2: https://aj.codes/blog/be-careful-using-tmux-and-environment-variables/
- 3: https://unix.stackexchange.com/questions/687543/global-and-session-environment-for-the-first-session-and-following
- 4: https://stackoverflow.com/questions/20701757/tmux-setting-environment-variables-for-sessions
- 5: https://www.johntobin.ie/blog/updating_environment_variables_from_tmux/
- 6: https://adhdecode.com/articles/tmux/tmux-environment-variables/
- 7: Unable to inherit environment variables from parent process (kitty) tmux/tmux#2834
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- concise repository state ---'
git status --short 2>/dev/null || true
git diff --stat -- Sources/RemoteTmuxControlConnection+Commands.swift \
Sources/RemoteTmuxControlConnection.swift \
Sources/RemoteTmuxControlConnection+CommandResults.swift \
Sources/RemoteTmuxController.swift \
cmuxTests/RemoteTmuxMirrorEnvironmentPushTests.swift 2>/dev/null || true
printf '%s\n' '--- relevant remote socket consumers ---'
rg -n -C 5 \
'CMUX_SOCKET_PATH|CMUX_SOCKET|CMUX_REMOTE_TMUX_MIRROR|show-environment' \
Sources/RemoteTmux*.swift Sources/RemoteInteractiveShellBootstrapBuilder.swift \
Sources/Workspace+RemoteDisconnectPlaceholder.swift \
CLI/CMUXCLI+TmuxCompat*.swift 2>/dev/null | head -400 || true
printf '%s\n' '--- exact environment-push source and tests ---'
nl -ba Sources/RemoteTmuxControlConnection+Commands.swift | sed -n '15,65p'
nl -ba cmuxTests/RemoteTmuxMirrorEnvironmentPushTests.swift | sed -n '115,215p'Repository: manaflow-ai/cmux
Length of output: 8174
Clear stale CMUX_SOCKET_PATH in the mirror session.
set-environment does not remove omitted variables. A previously stored path can remain visible through show-environment and route remote cmux commands to a dead socket. Send set-environment -u -t <target> CMUX_SOCKET_PATH in the same batch, and update the test to seed and assert removal.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/RemoteTmuxControlConnection`+Commands.swift around lines 44 - 48,
Update the mirror-session command flow around mirrorEnvironmentCommands to
include a set-environment -u command removing CMUX_SOCKET_PATH for the target in
the same batch, while preserving the existing environment assignments. Extend
the relevant test to seed CMUX_SOCKET_PATH and assert that the mirror operation
removes it.
Comment on lines
+44
to
+45
| private static let notifyPrefix = Array("777;notify;".utf8) | ||
| private static let osc9Prefix = Array("9;".utf8) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
OSC 9 subcommands are not discriminated, in the filter or in the tests. osc9Prefix is 9;, so every OSC 9 subcommand is classified as an iTerm2 growl notification, and the suite only exercises 9;<text>. The suite already pins the equivalent OSC 777 behavior with nonNotifyOsc777SubcommandPassesVerbatim, so the OSC 9 gap let the over-match ship untested.
Sources/RemoteTmuxNotificationOSCFilter.swift#L44-L45: add a progress-subcommand prefix (9;4;) and exclude it in bothfinishCandidateandreclassifyCandidate, so the sequence streams through instead of being stripped and reported.cmuxTests/RemoteTmuxNotificationOSCFilterTests.swift#L155-L160: add an OSC 9 counterpart tononNotifyOsc777SubcommandPassesVerbatimthat assertsESC ] 9;4;1;50 BELpasses verbatim and reports no notification.
📍 Affects 2 files
Sources/RemoteTmuxNotificationOSCFilter.swift#L44-L45(this comment)cmuxTests/RemoteTmuxNotificationOSCFilterTests.swift#L155-L160
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/RemoteTmuxNotificationOSCFilter.swift` around lines 44 - 45, Update
Sources/RemoteTmuxNotificationOSCFilter.swift lines 44-45 by defining the OSC 9
progress prefix 9;4; and excluding it in both finishCandidate and
reclassifyCandidate so progress sequences pass through unchanged. Update
cmuxTests/RemoteTmuxNotificationOSCFilterTests.swift lines 155-160 by adding a
counterpart to nonNotifyOsc777SubcommandPassesVerbatim that verifies ESC ]
9;4;1;50 BEL is preserved verbatim and produces no notification.
Comment on lines
+122
to
+125
| /// Per-pane filter that intercepts OSC 777/9 desktop-notification escapes | ||
| /// from `%output` (stateful across chunk boundaries) so a remote process | ||
| /// inside the mirrored session can notify locally (issue #833). | ||
| var notificationFilters: [Int: RemoteTmuxNotificationOSCFilter] = [:] |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
notificationFilters is never pruned, so it grows for the life of the mirror.
titleFilters is pruned in rebuildTopology at Line 393 with titleFilters = titleFilters.filter { livePanes.contains($0.key) }. The new notificationFilters map has no equivalent prune. tmux pane ids never recur, so every closed pane leaves a permanent entry. Each entry can also retain up to RemoteTmuxNotificationOSCFilter.maxBufferedBytes buffered candidate bytes, so a long-lived mirror with pane churn keeps both the entry count and the retained bytes growing.
The disconnect-edge removeAll() at Line 244 does not cover this: a connection can stay connected across unlimited pane churn.
Add the same prune next to the existing one.
As per coding guidelines: "Avoid repeated full scans, sorting, filtering, or per-item nested scans over scalable collections in production code ... Use sets, dictionaries, indexes, grouped queries, single-pass plans, caches, pagination, or explicit size bounds."
🐛 Proposed fix in `rebuildTopology` (Line 393)
cwdByPane = cwdByPane.filter { livePanes.contains($0.key) }
titleFilters = titleFilters.filter { livePanes.contains($0.key) }
+ notificationFilters = notificationFilters.filter { livePanes.contains($0.key) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/RemoteTmuxSessionMirror.swift` around lines 122 - 125, Update
rebuildTopology to prune notificationFilters alongside titleFilters, retaining
only entries whose pane IDs are present in livePanes. Place the cleanup next to
the existing titleFilters filtering and preserve the disconnect removeAll
behavior.
Source: Coding guidelines
Comment on lines
+7
to
+10
| /// Bounded handoff shared by every mirror, mirroring how the Ghostty OSC | ||
| /// callback funnels into ``GhosttyDesktopNotificationIngress`` (same | ||
| /// delivery, retargeting, and flood policy). | ||
| static let paneNotificationIngress = GhosttyDesktopNotificationIngress() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Locate the ingress type, its isolation, and every instance of it.
set -euo pipefail
# The type declaration and its actor isolation.
fd -i 'GhosttyDesktopNotification' --extension swift --exec ast-grep outline {} --items all
# Every construction site and every stored/shared instance.
rg -nP -C4 '\bGhosttyDesktopNotificationIngress\s*\(' --type=swift
rg -nP -C4 '(static|let|var)[^=]*\bGhosttyDesktopNotificationIngress\b' --type=swift
# How the Ghostty OSC desktop-notification callback reaches the ingress.
rg -nP -C6 '\.submit\(GhosttyDesktopNotificationRequest' --type=swiftRepository: manaflow-ai/cmux
Length of output: 873
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- ingress implementation ---'
cat -n Sources/GhosttyDesktopNotificationIngress.swift
printf '%s\n' '--- mirror routing file ---'
cat -n Sources/RemoteTmuxSessionMirror+OutputRouting.swift
printf '%s\n' '--- ingress references and submit call sites ---'
rg -n -P -C6 '\bGhosttyDesktopNotificationIngress\b|\.submit\(' Sources --glob '*.swift'
printf '%s\n' '--- Ghostty OSC notification callback references ---'
rg -n -P -C8 'GhosttyDesktopNotificationRequest|desktop.?notification|notification.*ingress|OSC' Sources --glob '*.swift'Repository: manaflow-ai/cmux
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- GhosttyApp owner and isolation ---'
sed -n '300,365p' Sources/GhosttyTerminalView.swift
sed -n '3238,3272p' Sources/GhosttyTerminalView.swift
printf '%s\n' '--- RemoteTmuxSessionMirror declarations and initializers ---'
rg -n -P -C5 '^(final |class |struct |actor )?RemoteTmuxSessionMirror|func init\(' Sources/RemoteTmuxSessionMirror*.swift
printf '%s\n' '--- all ingress construction sites, excluding broad call context ---'
rg -n '\bGhosttyDesktopNotificationIngress\s*\(' --type=swift
printf '%s\n' '--- relevant declarations for static/shared owners ---'
rg -n -P -C3 'desktopNotificationIngress|paneNotificationIngress|static let shared|NotificationDebugEmitter' Sources/GhosttyTerminalView.swift Sources/RemoteTmuxSessionMirror*.swift Sources/NotificationDebugEmitter.swiftRepository: manaflow-ai/cmux
Length of output: 4774
Reuse the existing GhosttyApp.desktopNotificationIngress instead of creating a second ingress. The new instance has an independent 256-request buffer, so local and mirrored notifications do not share the flood budget. Pass the existing ingress through the composition path and remove this type-level runtime instance. GhosttyDesktopNotificationIngress is not actor-isolated; only this property is isolated by the @MainActor extension.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/RemoteTmuxSessionMirror`+OutputRouting.swift around lines 7 - 10,
Replace the type-level paneNotificationIngress instance with the existing
GhosttyApp.desktopNotificationIngress, threading that ingress through the mirror
composition path and updating affected callers or initializers. Preserve the
shared delivery, retargeting, and flood-policy behavior, and rely on the
property’s existing `@MainActor` isolation without adding actor isolation to
GhosttyDesktopNotificationIngress.
Source: Coding guidelines
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.
Fixes #833 for ssh-tmux mirrors. Follows up on the mechanism documented in #8382's dogfood comment and the design sketch in #833's discussion.
Problem
In a pure ssh-tmux mirror every remote shell runs inside tmux, so shell integration only ever pulls
CMUX_*values — nothing publishes. The tmux session environment goes permanently stale, and OSC 777/9 notification escapes from remote agents reach a mirror surface with no local process TTY to attribute them by. The failure is silent: agent hooks report "sent", nothing arrives, the user concludes the agent is still working.Changes
Both in the mirror layer — no remote daemon, no user tmux config (
allow-passthroughnot required):1. Session-scoped env push on attach + every reconnect. A new
pushMirrorSessionEnvironment()runs at the post-attach drain point (pendingPostAttachActionconsumption), covering first connect and every reconnect. Two deliberate deviations from the local publisher, both verified empirically against tmux 3.3a:set-environment -t), not-g: the shell-integration refresh path runs a session-scopedshow-environment, which does not surface global values. A-gpush (as originally sketched in the issue) would be invisible to the pull path.CMUX_SOCKET_PATHis NOT pushed: the ssh-tmux transport has no relay, so a local Mac socket path would be dead on the remote. Pushed instead:CMUX_REMOTE_TMUX_MIRROR=1marker +CMUX_WORKSPACE_ID/CMUX_TAB_IDidentity.2. Pane-scoped OSC 777/9 interception.
RemoteTmuxNotificationOSCFilter— a chunk-split-safe FSM modeled onRemoteTmuxScreenTitleFilter, wired beside it inrouteOutput— stripsOSC 777;notify;<title>;<body>andOSC 9;<body>(BEL/ST) from mirrored pane output and delivers through the sharedGhosttyDesktopNotificationIngress, attributed pane→surface→workspace. Pane identity is ground truth in mirror mode; launch-time env vars never were (this also sidesteps the cross-workspace misrouting family in the issue thread).Tests
RemoteTmuxNotificationOSCFilterTests(18 cases): BEL/ST termination, chunk splits at every interesting boundary (post-ESC, mid-payload, pre-terminator), non-notify OSC 777 subcommands pass through, >4KB overflow passthrough, interleaved plain output integrity, UTF-8 bodies.RemoteTmuxMirrorEnvironmentPushTests(6 cases): exact wire format of theset-environmentcommand lines (pure builder), CR/LF-hostile value rejection, marker inclusion.Caveats
main; FSM logic was cross-validated with a line-equivalent port against all test scenarios, and every API call was checked against the callee's source, but I could not compile or run the app locally. Happy to iterate on anything the macOS build surfaces.xcodeprojwiring for the 3 new files is included (lint-pbxproj-test-wiring passes).cmux notifyfrom inside long-running remote processes) is out of scope here; the OSC path covers agent hook notifications without any env dependency.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Delivers OSC 777/9 desktop notifications from ssh‑tmux mirrored panes and keeps the remote tmux session identity fresh. Previously notifications were parsed but dropped and
CMUX_*in the tmux session went stale after relaunch; now pane output is intercepted and delivered locally, and session‑scoped env is pushed on attach and reconnect.Key changes
CMUX_REMOTE_TMUX_MIRROR=1,CMUX_WORKSPACE_ID, andCMUX_TAB_ID; excludesCMUX_SOCKET_PATH. Builder sorts/quotes pairs, drops unsafe values, and targets by stable session id or quoted name. Seed the connection’s mirror identity from the workspace at creation so the first push has correct IDs.RemoteTmuxNotificationOSCFilterbeside the title filter: interceptsOSC ] 777;notify;…andOSC ] 9;…(BEL/ST), strips them, and delivers viaGhosttyDesktopNotificationIngressattributed to the pane’s surface and workspace. Reseed strips without re‑delivering; unfinished buffer capped at 4KB; all other OSC and overflows pass through byte‑identical.Review and tests
cmux sshworkspaces are unchanged.RemoteTmuxControlConnection(including immediate push viasetMirrorEnvironment) and output/seed routing inRemoteTmuxSessionMirror.RemoteTmuxMirrorEnvironmentPushTests(command format and safety) andRemoteTmuxNotificationOSCFilterTests(terminators, chunk splits, overflow, pass‑through, UTF‑8).Written for commit 9fa5bed. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes