Fix background workspace PTY startup for socket-created surfaces - #3876
Merged
Merged
Conversation
Socket clients can create terminal surfaces in workspaces that are not selected. The existing workspace-create coverage only proves the first hidden terminal starts; it does not cover a later split surface that receives input while its view is still off-window. This regression test drives the V2 socket API end to end: create a workspace without selecting it, split a terminal in that workspace, send text to the new surface, and assert the command executed by observing a marker file. On the 0.64.x regression path the input is accepted but remains queued until the user selects the workspace. Constraint: Repository policy runs socket/UI tests in CI or VM, not locally. Confidence: high Scope-risk: narrow Tested: Not run locally per repository policy; test is intended to fail before the fix in CI/VM. Not-tested: Local socket execution.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a standalone regression test that opens the cmux socket, creates a background split terminal in a new workspace, sends a python3 one-liner via ChangesBackground Split Send_text Regression Test
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (6 errors, 1 warning)
✅ Passed checks (9 passed)
✨ 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 adds a V2 socket regression test (
Confidence Score: 5/5Safe to merge — the change is a single new test file with no production code modifications. The test logic, assertions, command construction, and marker-file polling are all correct. The only gap is in cleanup error handling: a workspace-close failure during an otherwise-passing run is silently swallowed rather than surfaced to the runner. tests_v2/test_background_split_send_text_starts_terminal.py — specifically the finally block cleanup pattern. Important Files Changed
Sequence DiagramsequenceDiagram
participant T as Test
participant S as cmux Socket
participant BG as Background Workspace
T->>S: "workspace.create {}"
S-->>T: workspace_id (no focus change)
T->>S: "surface.list {workspace_id}"
S-->>T: surfaces (initial terminal surface)
T->>S: "surface.split {workspace_id, surface_id, direction:right, focus:false}"
S-->>T: split_surface_id (no focus change)
T->>S: "surface.send_text {workspace_id, surface_id:split_surface, text:command}"
S->>BG: start PTY (off-window, triggered by send_text)
S-->>T: "{surface_id: split_surface}"
BG->>BG: execute python3 -c ... write marker file
T->>T: _wait_for_file_text (poll 0.1s, timeout 8s)
T->>S: "workspace.current (assert == baseline)"
T->>S: "workspace.close {workspace_id}"
Reviews (10): Last reviewed commit: "Remove redundant background send asserti..." | Re-trigger Greptile |
austinywang
force-pushed
the
issue-3798-bg-workspace-pty
branch
from
May 12, 2026 01:14
dd745e5 to
2babcc3
Compare
Socket-driven terminal operations need to work as a control plane for background workspaces, but the previous eager off-window attach path made every model-only TerminalPanel start a PTY in unit tests. Keep the visual attach lifecycle lazy until an NSWindow exists, and route socket/read/send readiness through requestBackgroundSurfaceStartIfNeeded so explicit runtime demand can still create from the attached Ghostty view without selecting the workspace. Constraint: Do not run local tests or xcodebuild; validation must come from CI and lightweight local diff checks. Constraint: Background socket API must not mutate workspace focus. Rejected: Auto-select the target workspace before send_text | violates the focus allowlist and steals user focus. Rejected: Eagerly create every off-window attach | spawns PTYs for model-only/unit-test panels and timed out CircleCI unit tests. Confidence: high Scope-risk: moderate Directive: Do not reintroduce NSWindow membership as a prerequisite in requestBackgroundSurfaceStartIfNeeded; visual attach can be window-lazy, but explicit socket/API runtime demand must be able to start off-window. Tested: git diff --check Not-tested: Local socket/UI/unit tests per repository policy; CI will rerun the regression and unit suites.
austinywang
force-pushed
the
issue-3798-bg-workspace-pty
branch
from
May 12, 2026 02:01
2babcc3 to
85b5d88
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests_v2/test_background_split_send_text_starts_terminal.py`:
- Around line 109-112: The cleanup try/except that calls
c.close_workspace(created_workspace) currently swallows all exceptions; modify
the except block to log the failure (including exception details) instead of
silent pass — e.g., import logging (or use print(..., file=sys.stderr)) and call
logging.exception or logging.error with contextual text and the caught exception
when close_workspace fails so cleanup errors are visible during test runs.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c70a45f8-e557-4bc6-a654-6bdab2d51ade
📒 Files selected for processing (3)
Sources/GhosttyTerminalView.swiftSources/Panels/TerminalPanel.swifttests_v2/test_background_split_send_text_starts_terminal.py
austinywang
dismissed
coderabbitai[bot]’s stale review
May 13, 2026 20:38
Only requested change was addressed in 35ca0ba and acknowledged by CodeRabbit in discussion_r3225270799; dismissing stale bot review state.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
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="tests_v2/test_background_split_send_text_starts_terminal.py">
<violation number="1" location="tests_v2/test_background_split_send_text_starts_terminal.py:75">
P2: Assert that `surface.split` returns a new surface ID. The current non-empty check can let this regression test pass even if no new split surface was created.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
| }, | ||
| ) or {} | ||
| split_surface = str(split_payload.get("surface_id") or "") | ||
| _must(bool(split_surface), f"surface.split returned no surface_id: {split_payload}") |
There was a problem hiding this comment.
P2: Assert that surface.split returns a new surface ID. The current non-empty check can let this regression test pass even if no new split surface was created.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests_v2/test_background_split_send_text_starts_terminal.py, line 75:
<comment>Assert that `surface.split` returns a new surface ID. The current non-empty check can let this regression test pass even if no new split surface was created.</comment>
<file context>
@@ -0,0 +1,124 @@
+ },
+ ) or {}
+ split_surface = str(split_payload.get("surface_id") or "")
+ _must(bool(split_surface), f"surface.split returned no surface_id: {split_payload}")
+ _must(
+ c.current_workspace() == baseline_workspace,
</file context>
Merged
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
NSWindowmembership so a liveTerminalSurfacewith a concreteGhosttyNSViewcan start off-window without stealing focusportOrdinalintoTerminalSurfaceinitialization so eager off-window startup builds the correct environmentVerification
cmux new-split right --workspace workspace:43,cmux send --workspace workspace:43 --surface surface:117 "echo issue3798-repro-before-fix\n",cmux read-screen --workspace workspace:43 --surface surface:117returningTerminal surface not found, then selecting the workspace caused the queued echo to run.git diff --checkNotes
surface.create,surface.split, orsurface.send_text; background socket control remains non-focusing.Note
Low Risk
Low risk: this PR only adds a new v2 socket regression test and does not change production code paths.
Overview
Adds a new v2 socket regression test (
test_background_split_send_text_starts_terminal.py) that creates a workspace without selecting it, splits a terminal in that background workspace withfocus: false, then callssurface.send_textand verifies the command executed via a marker file.The test also asserts the currently selected workspace never changes across
workspace.create,surface.split, andsurface.send_text, and includes cleanup for the temporary marker file and created workspace.Reviewed by Cursor Bugbot for commit 0af5652. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Start background terminal PTYs for socket-created surfaces on explicit runtime demand from the attached
GhosttyNSView, even when off-window. Allowssurface.send_textto start and run background splits without changing workspace focus (fixes #3798).NSWindowexists; allowrequestBackgroundSurfaceStartIfNeeded()to start the runtime from an off-window attached view.portOrdinalinTerminalSurfaceinit so all startup paths use the correct per-workspace port range.surface.send_text; tighten assertions, remove a redundant check, and clean up logging.Written for commit 0af5652. Summary will update on new commits. Review in cubic
Summary by CodeRabbit