Fix hold-Enter prompt spacer rows - #3466
Closed
austinywang wants to merge 10 commits into
Closed
austinywang wants to merge 10 commits into
austinywang wants to merge 10 commits into
Conversation
The zsh shell integration can write a physical spacer row before the next prompt. The regression locks that behavior through the shell helper so the follow-up fix can remove the pty byte source instead of masking it in the renderer. Constraint: Local test execution is disabled by repo policy; this commit is test-only for CI to prove red before the fix. Confidence: high Scope-risk: narrow Tested: Not run locally (repo policy) Not-tested: Local XCTest execution
Prompt redraw and wrapping are already represented by Ghostty semantic prompt markers. The cmux zsh metadata hook was adding a physical blank row for long wrapped prompts, which made prompt density depend on shell-integration bookkeeping instead of terminal output. Constraint: Keep the fix in the cmux wrapper because the byte source is cmux-owned shell integration, not Swift rendering or upstream OSC 133 parsing. Rejected: Hide blank prompt rows in Terminal.zig | renderer masking would preserve bad pty bytes and risk prompt-aware selection behavior Confidence: high Scope-risk: narrow Directive: Shell integration precmd/preexec metadata hooks must not print layout bytes; use OSC markers or app socket metadata only Tested: Not run locally (repo policy) Not-tested: Local XCTest execution; visual app verification pending reload launch
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR removes Ghostty semantic-patching and turns the zsh prompt-wrap guard into an unconditional no-op, updates keyboard-shortcut normalization to use conflict-aware resolution, adds test helpers and regression/unit tests, wires new test sources into the Xcode project, and makes a small visibility propagation change for hosted terminal views. ChangesZsh Shell Integration & Tests
Keyboard Shortcut Normalization & Tests
Terminal Window Portal Visibility & Tests
Sequence Diagram(s)sequenceDiagram
participant Zsh as Zsh (interactive)
participant CMUX as cmux zsh-integration
participant Ghostty as Ghostty hook definitions
participant Terminal as Terminal/PTy
Zsh->>CMUX: call _cmux_prompt_wrap_guard before prompt
Note over CMUX: guard now immediately returns 0 (no spacer printed)
CMUX-->>Zsh: return 0
Zsh->>Ghostty: execute PREEXEC/PRECMD hooks (OSC 133 markers)
Ghostty->>Terminal: emit OSC 133 prompt-control sequences
Terminal->>Terminal: apply redraw/marker semantics (no extra spacer)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
Greptile SummaryThis PR fixes a regression where Confidence Score: 4/5Safe to merge; the only finding is a cosmetic dead-argument cleanup at the call site. Only P2 findings (unused arguments at the No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Shell as zsh precmd hook
participant Guard as _cmux_prompt_wrap_guard
participant PTY as PTY / terminal
participant Ghostty as Ghostty OSC 133
Note over Shell,Ghostty: Before fix — long $PWD path
Shell->>Guard: _cmux_prompt_wrap_guard "$cmd_start" "$pwd"
Guard->>PTY: builtin print -r -- "" (blank spacer line)
Guard-->>Shell: return
Ghostty->>PTY: OSC 133 prompt redraw
Note over Shell,Ghostty: After fix
Shell->>Guard: _cmux_prompt_wrap_guard "$cmd_start" "$pwd"
Guard-->>Shell: return 0 (no-op)
Ghostty->>PTY: OSC 133 prompt redraw (sole owner)
|
Settings file parsing runs while the global shortcut settings store may be under construction, so it must be independent of the currently installed store. This regression installs a conflicting active store and verifies a candidate settings file can still parse numbered shortcut bindings from its own contents. Constraint: Startup initializes KeyboardShortcutSettings.settingsFileStore through CmuxSettingsFileStore.shared. Rejected: Cover the crash only with manual launch logs | CI needs an executable parser-level invariant. Confidence: high Scope-risk: narrow Tested: Not run locally per repository testing policy. Not-tested: Local XCTest execution; CI will run the suite.
The settings file store initializes underneath KeyboardShortcutSettings.settingsFileStore. During that initialization, parsing persisted shortcut bindings must only apply action-local normalization; recorder conflict checks need the active global store and can recursively enter the same lazy singleton at startup. Constraint: cmux DEV launch crashed in dispatch_once when CmuxSettingsFileStore.shared parsed shortcuts and reentered KeyboardShortcutSettings.settingsFileStore. Rejected: Suppress the crash around static initialization | the parser should not depend on active shortcut globals at all. Confidence: high Scope-risk: narrow Directive: Settings-file parsing may normalize per-action syntax, but must not call APIs that resolve conflicts through KeyboardShortcutSettings.shortcut(for:). Tested: Not run locally per repository testing policy; covered by new parser regression. Not-tested: Local XCTest execution; CI will run the suite.
The prompt-spacing and launch-crash regressions need executable coverage, but placing those tests in existing oversized catchall files tripped the Swift file length guard. Move the zsh shell harness into a small support file and keep shortcut file-store regression coverage in its own focused test file so CI can validate the behavior without accepting more file-length debt. Constraint: Repository policy forbids local test runs; CI owns XCTest execution. Rejected: Refresh the file length budget | would bless new debt instead of removing it. Confidence: high Scope-risk: narrow Tested: python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv; git diff --check; plutil -lint GhosttyTabs.xcodeproj/project.pbxproj Not-tested: Local XCTest suite per repository testing policy
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxTests/ShellIntegrationTestSupport.swift`:
- Around line 100-104: In the block that calls process.terminate() followed by
process.waitUntilExit(), replace the unbounded wait with a bounded wait: after
calling terminate() on the Process instance (the process variable), poll
process.isRunning in a loop with a short sleep or use an XCTestExpectation with
a timeout (e.g., 5s); if the timeout elapses and process.isRunning is still
true, escalate by sending SIGKILL (or call process.kill equivalent) and then
call waitUntilExit() once to reap it, finally failing the test; apply the same
change to the other occurrences that call terminate() then waitUntilExit().
- Around line 90-107: The test can deadlock because stdout/stderr are only read
after process exit; instead install continuous readers while the process runs by
attaching readability handlers to stdout.fileHandleForReading and
stderr.fileHandleForReading that append incoming data into mutable Data/String
buffers, then run process.run() and wait as before; after the loop remove the
readability handlers, call readDataToEndOfFile() once more to collect remaining
bytes, and convert the accumulated buffers into the final output and error
strings (referencing process, stdout, stderr, output, error variables and the
existing deadline/timeout logic).
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fe924dfe-f780-4076-8a22-8bb023d9d5e6
📒 Files selected for processing (4)
GhosttyTabs.xcodeproj/project.pbxprojcmuxTests/GhosttyConfigTests.swiftcmuxTests/KeyboardShortcutSettingsFileStoreRegressionTests.swiftcmuxTests/ShellIntegrationTestSupport.swift
✅ Files skipped from review due to trivial changes (1)
- GhosttyTabs.xcodeproj/project.pbxproj
🚧 Files skipped from review as they are similar to previous changes (1)
- cmuxTests/GhosttyConfigTests.swift
Comment on lines
+90
to
+107
| let stdout = Pipe() | ||
| let stderr = Pipe() | ||
| process.standardOutput = stdout | ||
| process.standardError = stderr | ||
|
|
||
| try process.run() | ||
| let deadline = Date().addingTimeInterval(5) | ||
| while process.isRunning && Date() < deadline { | ||
| _ = RunLoop.current.run(mode: .default, before: Date().addingTimeInterval(0.01)) | ||
| } | ||
| if process.isRunning { | ||
| process.terminate() | ||
| process.waitUntilExit() | ||
| XCTFail("Timed out waiting for zsh to exit") | ||
| } | ||
|
|
||
| let output = String(data: stdout.fileHandleForReading.readDataToEndOfFile(), encoding: .utf8) ?? "" | ||
| let error = String(data: stderr.fileHandleForReading.readDataToEndOfFile(), encoding: .utf8) ?? "" |
There was a problem hiding this comment.
Drain stdout/stderr while the process is running to prevent pipe backpressure hangs.
On Line 95 you launch /bin/zsh, but Lines 106-107 read both pipes only after exit. If startup output fills a pipe buffer, zsh can block on write and never reach normal exit, causing a false timeout path.
Suggested fix
let stdout = Pipe()
let stderr = Pipe()
process.standardOutput = stdout
process.standardError = stderr
+
+ let outputLock = NSLock()
+ var outputData = Data()
+ let errorLock = NSLock()
+ var errorData = Data()
+
+ stdout.fileHandleForReading.readabilityHandler = { handle in
+ let chunk = handle.availableData
+ guard !chunk.isEmpty else {
+ handle.readabilityHandler = nil
+ return
+ }
+ outputLock.lock()
+ outputData.append(chunk)
+ outputLock.unlock()
+ }
+ stderr.fileHandleForReading.readabilityHandler = { handle in
+ let chunk = handle.availableData
+ guard !chunk.isEmpty else {
+ handle.readabilityHandler = nil
+ return
+ }
+ errorLock.lock()
+ errorData.append(chunk)
+ errorLock.unlock()
+ }
+ defer {
+ stdout.fileHandleForReading.readabilityHandler = nil
+ stderr.fileHandleForReading.readabilityHandler = nil
+ }
@@
- let output = String(data: stdout.fileHandleForReading.readDataToEndOfFile(), encoding: .utf8) ?? ""
- let error = String(data: stderr.fileHandleForReading.readDataToEndOfFile(), encoding: .utf8) ?? ""
+ outputLock.lock()
+ let output = String(data: outputData, encoding: .utf8) ?? ""
+ outputLock.unlock()
+ errorLock.lock()
+ let error = String(data: errorData, encoding: .utf8) ?? ""
+ errorLock.unlock()📝 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
| let stdout = Pipe() | |
| let stderr = Pipe() | |
| process.standardOutput = stdout | |
| process.standardError = stderr | |
| try process.run() | |
| let deadline = Date().addingTimeInterval(5) | |
| while process.isRunning && Date() < deadline { | |
| _ = RunLoop.current.run(mode: .default, before: Date().addingTimeInterval(0.01)) | |
| } | |
| if process.isRunning { | |
| process.terminate() | |
| process.waitUntilExit() | |
| XCTFail("Timed out waiting for zsh to exit") | |
| } | |
| let output = String(data: stdout.fileHandleForReading.readDataToEndOfFile(), encoding: .utf8) ?? "" | |
| let error = String(data: stderr.fileHandleForReading.readDataToEndOfFile(), encoding: .utf8) ?? "" | |
| let stdout = Pipe() | |
| let stderr = Pipe() | |
| process.standardOutput = stdout | |
| process.standardError = stderr | |
| let outputLock = NSLock() | |
| var outputData = Data() | |
| let errorLock = NSLock() | |
| var errorData = Data() | |
| stdout.fileHandleForReading.readabilityHandler = { handle in | |
| let chunk = handle.availableData | |
| guard !chunk.isEmpty else { | |
| handle.readabilityHandler = nil | |
| return | |
| } | |
| outputLock.lock() | |
| outputData.append(chunk) | |
| outputLock.unlock() | |
| } | |
| stderr.fileHandleForReading.readabilityHandler = { handle in | |
| let chunk = handle.availableData | |
| guard !chunk.isEmpty else { | |
| handle.readabilityHandler = nil | |
| return | |
| } | |
| errorLock.lock() | |
| errorData.append(chunk) | |
| errorLock.unlock() | |
| } | |
| defer { | |
| stdout.fileHandleForReading.readabilityHandler = nil | |
| stderr.fileHandleForReading.readabilityHandler = nil | |
| } | |
| try process.run() | |
| let deadline = Date().addingTimeInterval(5) | |
| while process.isRunning && Date() < deadline { | |
| _ = RunLoop.current.run(mode: .default, before: Date().addingTimeInterval(0.01)) | |
| } | |
| if process.isRunning { | |
| process.terminate() | |
| process.waitUntilExit() | |
| XCTFail("Timed out waiting for zsh to exit") | |
| } | |
| outputLock.lock() | |
| let output = String(data: outputData, encoding: .utf8) ?? "" | |
| outputLock.unlock() | |
| errorLock.lock() | |
| let error = String(data: errorData, encoding: .utf8) ?? "" | |
| errorLock.unlock() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxTests/ShellIntegrationTestSupport.swift` around lines 90 - 107, The test
can deadlock because stdout/stderr are only read after process exit; instead
install continuous readers while the process runs by attaching readability
handlers to stdout.fileHandleForReading and stderr.fileHandleForReading that
append incoming data into mutable Data/String buffers, then run process.run()
and wait as before; after the loop remove the readability handlers, call
readDataToEndOfFile() once more to collect remaining bytes, and convert the
accumulated buffers into the final output and error strings (referencing
process, stdout, stderr, output, error variables and the existing
deadline/timeout logic).
Comment on lines
+100
to
+104
| if process.isRunning { | ||
| process.terminate() | ||
| process.waitUntilExit() | ||
| XCTFail("Timed out waiting for zsh to exit") | ||
| } |
There was a problem hiding this comment.
Avoid unbounded waitUntilExit() after timeout-triggered termination.
On Line 101/239/258, the code calls terminate(), then Line 102/240/259 calls waitUntilExit() with no bound. If zsh ignores or delays SIGTERM, the test can hang indefinitely.
Suggested fix
+private func terminateProcessBounded(_ process: Process, graceSeconds: TimeInterval = 1.0) {
+ guard process.isRunning else { return }
+ process.terminate()
+ let killDeadline = Date().addingTimeInterval(graceSeconds)
+ while process.isRunning && Date() < killDeadline {
+ _ = RunLoop.current.run(mode: .default, before: Date().addingTimeInterval(0.01))
+ }
+ if process.isRunning {
+ kill(process.processIdentifier, SIGKILL)
+ }
+ process.waitUntilExit()
+}
@@
- process.terminate()
- process.waitUntilExit()
+ terminateProcessBounded(process)
@@
- process.terminate()
- process.waitUntilExit()
+ terminateProcessBounded(process)
@@
- process.terminate()
- process.waitUntilExit()
+ terminateProcessBounded(process)Also applies to: 239-240, 258-260
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxTests/ShellIntegrationTestSupport.swift` around lines 100 - 104, In the
block that calls process.terminate() followed by process.waitUntilExit(),
replace the unbounded wait with a bounded wait: after calling terminate() on the
Process instance (the process variable), poll process.isRunning in a loop with a
short sleep or use an XCTestExpectation with a timeout (e.g., 5s); if the
timeout elapses and process.isRunning is still true, escalate by sending SIGKILL
(or call process.kill equivalent) and then call waitUntilExit() once to reap it,
finally failing the test; apply the same change to the other occurrences that
call terminate() then waitUntilExit().
The remaining prompt-density mismatch comes from cmux mutating Ghostty's zsh OSC 133 marker into the Bash-only redraw=last form. Lock the zsh handoff to preserve the upstream fresh-prompt and prompt-start markers so CI can show the fix changes behavior rather than just moving shell glue around. Constraint: Repository policy forbids local XCTest runs; this commit intentionally adds the failing regression before the implementation change. Rejected: Keep asserting redraw=last | that codifies the divergence from Ghostty zsh behavior the issue is about. Confidence: high Scope-risk: narrow Tested: Not run; failing regression commit only. Not-tested: Local XCTest suite per repository testing policy
cmux was loading Ghostty's zsh integration and then rewriting its OSC 133 fresh-prompt marker to the Bash-only redraw=last form. That split ownership meant held Enter no longer matched Ghostty even after removing the explicit spacer line. Delete the rewrite path so cmux's zsh layer only contributes cmux metadata hooks while Ghostty remains the single source of truth for prompt semantics. Constraint: Ghostty zsh integration already uses OSC 133;A for real prompt transitions and OSC 133;P for redraw/editor fallback markers. Rejected: Keep a conditional rewrite for older Ghostty hooks | the bundled integration is copied from the current submodule at build time, and compatibility rewriting is the divergent behavior causing this class of issue. Confidence: high Scope-risk: narrow Directive: Do not mutate Ghostty zsh OSC 133 prompt markers from cmux shell integration; add cmux metadata hooks around them instead. Tested: zsh -n Resources/shell-integration/.zshenv Resources/shell-integration/cmux-zsh-integration.zsh Resources/shell-integration/cmux-bash-integration.bash; python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv; git diff --check; pty byte comparison confirmed no 133;A;redraw=last in cmux zsh bootstrap. Not-tested: Local XCTest suite per repository testing policy
The remaining Enter-hold mismatch is not in pty bytes: fresh socket screen reads are packed while the live AppKit view can still show stale glyphs through transparent terminal layers. This regression locks the portal lifecycle boundary that must retire hidden terminal entries immediately, before a later geometry sync happens.\n\nConstraint: Local XCTest execution is disabled by repo policy; this is committed as the red regression commit for CI.\nConfidence: high\nScope-risk: narrow\nDirective: Do not let updateEntryVisibility(false) become metadata-only; hidden terminal portal entries must stop drawing immediately.\nTested: git diff --check -- cmuxTests/TerminalAndGhosttyTests.swift\nNot-tested: XCTest not run locally per repo policy
Fresh pty output and socket screen reads already match Ghostty, so the remaining prompt gap came from stale portal-hosted terminal layers showing through transparent active terminal cells during workspace/host handoff. The terminal portal now treats visibleInUI=false as an immediate drawing invariant instead of metadata that waits for a later geometry sync.\n\nConstraint: Terminal views intentionally use transparent layers; stale portal views must be hidden rather than relying on opaque repainting.\nRejected: Make terminal layers opaque | would change the panel background/rendering contract and mask stale portal ownership instead of fixing it.\nConfidence: high\nScope-risk: narrow\nDirective: Keep reveal paths in bind/sync; only invisible updates should force immediate hosted-view hiding.\nTested: git diff --check -- Sources/TerminalWindowPortal.swift\nNot-tested: XCTest not run locally per repo policy
The portal regression belongs in a focused test file instead of growing TerminalAndGhosttyTests, and the runtime fix can enforce the invariant without extra debug-only lines. This keeps the large-file budget green while preserving the test-first coverage and the portal ownership change.\n\nConstraint: The Swift file length budget is enforced before launch.\nConfidence: high\nScope-risk: narrow\nTested: python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv\nTested: git diff --check\nTested: plutil -lint GhosttyTabs.xcodeproj/project.pbxproj\nNot-tested: XCTest not run locally per repo policy
This branch was successfully deployed
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
Fixes #3464
Testing
Note
Medium Risk
Changes shell-integration behavior for zsh prompts (TTY output) and tweaks shortcut parsing/portal visibility, which can affect interactive terminal UX; coverage is added but regressions could still show up in edge prompt/hook timing scenarios.
Overview
Prevents
cmuxzsh shell integration from writing extra spacer/newline output around prompts by neutering_cmux_prompt_wrap_guardand removing Ghostty OSC 133 “semantic redraw” patching so Ghostty owns prompt redraw/markers end-to-end.Adjusts settings-file shortcut parsing to normalize via
resolvedRecordedShortcutIgnoringConflicts(...)(avoids consulting active-store conflicts during load), and updatesTerminalWindowPortal.updateEntryVisibilityto immediately propagate hidden/occlusion state when an entry becomes invisible.Adds regression coverage and refactors test harness: new shared
ShellIntegrationTestSupport.swift, plus new tests for prompt spacer suppression, numbered shortcut parsing, and immediate portal hide behavior; Xcode project is updated to include these test files.Reviewed by Cursor Bugbot for commit 7e6ccf0. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Stops
cmuxzsh from printing a spacer row and from rewriting Ghostty’s zsh OSC 133 markers; prompt redraw stays owned by Ghostty. Also hides retiring terminal portals immediately and decouples settings‑file shortcut parsing from global conflicts; fixes #3464.Bug Fixes
action.resolvedRecordedShortcutIgnoringConflicts(...); add a regression for numbered bindings with unrelated conflicts.TerminalWindowPortalentries immediately onvisibleInUI=false; add a regression test.Refactors
ShellIntegrationTestSupport.swiftand keep new regressions in focused files (KeyboardShortcutSettingsFileStoreRegressionTests.swift,TerminalWindowPortalVisibilityTests.swift) to stay within CI file-length budgets.Written for commit 7e6ccf0. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Behavior Changes
Tests