Repository navigation
Fix helper read-screen terminal materialization - #6236
Closed
azooz2003-bit wants to merge 17 commits into
Closed
azooz2003-bit wants to merge 17 commits into
azooz2003-bit wants to merge 17 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds cold-terminal materialization support for ChangesCold Surface Materialization and Retry
Sequence DiagramsequenceDiagram
participant CLI as cmux CLI
participant Socket as SocketClient
participant Coord as ControlCommandCoordinator
participant TC as TerminalController
participant Surface as TerminalSurface
CLI->>CLI: readScreenPayload(params, start_if_needed=true)
loop up to 60 retries
CLI->>Socket: surface.read_text(start_if_needed=true)
Socket->>Coord: surfaceReadText(start_if_needed=true)
Coord->>TC: controlSurfaceReadText(startIfNeeded=true)
TC->>TC: ensureTerminalSurfaceReadyForRead preflight
alt surface cold
TC->>Surface: requestReadDemandSurfaceStartIfNeeded()
TC->>TC: re-check readDemand live surface
TC-->>Coord: .terminalNotReady(surfaceId)
Coord-->>Socket: err(terminal_not_ready, retryable=true)
Socket-->>CLI: CLIError "terminal_not_ready: ..."
CLI->>CLI: sleep 100ms, retry
else surface ready
TC-->>Coord: .success(snapshot)
Coord-->>Socket: ok(payload)
Socket-->>CLI: payload
CLI-->>CLI: return payload
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors, 1 warning)
✅ Passed checks (17 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 |
Contributor
Greptile SummaryThis PR fixes cold-start materialization for background helper terminals:
Confidence Score: 4/5Safe to merge after adding the missing locale translations for The new Resources/Localizable.xcstrings — missing locale translations for the new Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant CLI as CLI (read-screen)
participant SW as Socket Worker
participant RW as SurfaceReadTextReadinessWaiter
participant MA as MainActor / Coordinator
participant TS as TerminalSurface
CLI->>SW: "surface.read_text {start_if_needed: true}"
SW->>MA: v2MainSync → controlSurfaceReadText(startIfNeeded: true)
MA->>TS: ensureTerminalSurfaceReadyForRead → requestReadDemandSurfaceStartIfNeeded()
TS-->>MA: "surface == nil → scheduleHeadlessStart (async)"
MA-->>SW: .terminalNotReady(surfaceID)
SW->>RW: prepareWait(for: surfaceID) → waiter(.pending)
SW->>MA: v2MainSync → controlSurfaceReadText (retry, still cold)
MA-->>SW: .terminalNotReady(surfaceID) again
SW->>RW: wait(readinessWait) — awaits continuation
TS-->>MA: surface becomes ready
MA->>SW: NotificationCenter posts terminalSurfaceDidBecomeReady
SW->>RW: signalReady → resume continuation(true)
RW-->>SW: wait() → true
SW->>MA: v2MainSync → controlSurfaceReadText (final read)
MA-->>SW: .read(text, base64, …)
SW-->>CLI: "OK {text: …}"
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant CLI as CLI (read-screen)
participant SW as Socket Worker
participant RW as SurfaceReadTextReadinessWaiter
participant MA as MainActor / Coordinator
participant TS as TerminalSurface
CLI->>SW: "surface.read_text {start_if_needed: true}"
SW->>MA: v2MainSync → controlSurfaceReadText(startIfNeeded: true)
MA->>TS: ensureTerminalSurfaceReadyForRead → requestReadDemandSurfaceStartIfNeeded()
TS-->>MA: "surface == nil → scheduleHeadlessStart (async)"
MA-->>SW: .terminalNotReady(surfaceID)
SW->>RW: prepareWait(for: surfaceID) → waiter(.pending)
SW->>MA: v2MainSync → controlSurfaceReadText (retry, still cold)
MA-->>SW: .terminalNotReady(surfaceID) again
SW->>RW: wait(readinessWait) — awaits continuation
TS-->>MA: surface becomes ready
MA->>SW: NotificationCenter posts terminalSurfaceDidBecomeReady
SW->>RW: signalReady → resume continuation(true)
RW-->>SW: wait() → true
SW->>MA: v2MainSync → controlSurfaceReadText (final read)
MA-->>SW: .read(text, base64, …)
SW-->>CLI: "OK {text: …}"
Reviews (12): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+4780
to
+4790
| while true { | ||
| do { | ||
| payload = try client.sendV2(method: "surface.read_text", params: params) | ||
| break | ||
| } catch let error as CLIError | ||
| where error.message.hasPrefix("terminal_not_ready:") && readAttempt < 60 { | ||
| readAttempt += 1 | ||
| usleep(100_000) | ||
| continue | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
usleep polling loop in production Swift — The retry loop uses usleep(100_000) up to 60 times (≤ 6 s of wall-clock blocking) to synchronize on terminal startup. This is a fixed sleep used as synchronization, which the cmux-swift-blocking-runtime rule prohibits in shipped Swift code regardless of duration. The real readiness event here is TerminalSurface.surface transitioning from nil to non-nil after the queued Task executes; the CLI is polling across a socket boundary instead of waiting on that signal. A conforming approach would have the server block internally until the surface is ready (or a caller-supplied deadline expires) before responding, removing the need for any timed retry on the client side.
Rule Used: Flag new blocking or timing-based synchronization ... (source)
Comment on lines
5008
to
5017
| private func readTerminalTextBase64(terminalPanel: TerminalPanel, includeScrollback: Bool = false, lineLimit: Int? = nil) -> String { | ||
| if terminalPanel.surface.liveSurfaceForGhosttyAccess(reason: "readTerminalTextBase64.preflight") == nil { | ||
| terminalPanel.surface.requestReadDemandSurfaceStartIfNeeded() | ||
| guard terminalPanel.surface.liveSurfaceForGhosttyAccess(reason: "readTerminalTextBase64.readDemand") != nil else { | ||
| return "ERROR: Terminal surface not found" | ||
| } | ||
| } | ||
| guard terminalPanel.surface.liveSurfaceForGhosttyAccess(reason: "readTerminalTextBase64") != nil else { | ||
| return "ERROR: Terminal surface not found" | ||
| } |
Contributor
There was a problem hiding this comment.
requestReadDemandSurfaceStartIfNeeded() schedules surface creation inside a Task { @MainActor } and returns immediately; the surface is never synchronously available when control returns. Because this is a synchronous MainActor context with no await, the surface cannot change between the requestReadDemandSurfaceStartIfNeeded() call and the guard … != nil check on the next line — it will always be nil for a cold surface. The guard on line 5011 is therefore dead code; and the pre-existing guard on line 5015 is also unreachable in the warm path (already guarded by skipping the entire if block). Collapsing these to a single early-return after the startup request would remove the misleading synchronous-check appearance.
azooz2003-bit
force-pushed
the
issue-helper-terminal-lifecycle
branch
2 times, most recently
from
June 16, 2026 05:12
b493a4d to
cce912b
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: 2
🤖 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
`@Packages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Surface/ControlCommandCoordinator`+Surface2.swift:
- Around line 315-320: The error message "Terminal surface is starting" in the
`.terminalNotReady` case of the ControlCommandCoordinator+Surface2.swift file is
a bare English string literal that must be localized. Replace this string with a
call to String(localized:defaultValue:) using an appropriate localization key,
then add matching entries for that key to the Resources/*.xcstrings file for all
supported locales, following the project's localization guidelines for
user-visible error messages.
In `@Sources/TerminalController.swift`:
- Around line 5015-5017: The guard statement checking
liveSurfaceForGhosttyAccess in the readTerminalTextBase64 method is redundant
because an earlier guard (lines 5011-5013) already validates that the surface is
non-nil and returns early if it fails. Remove the guard at lines 5015-5017 and
call liveSurfaceForGhosttyAccess directly without the nil-check, since execution
only reaches this point when the preflight check has already confirmed the
surface exists.
🪄 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: 0adcab21-d4a2-4edd-8815-c66d68b7d116
📒 Files selected for processing (8)
CLI/cmux.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Surface/ControlCommandCoordinator+Surface2.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Surface/ControlSurfaceReadTextResolution.swiftPackages/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorSurfaceReadTextTests.swiftPackages/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface+BackgroundStartLifecycle.swiftSources/TerminalController+ControlSurfaceContext3.swiftSources/TerminalController.swifttests_v2/test_cli_background_terminal_helpers_read_screen_starts_pty.py
azooz2003-bit
force-pushed
the
issue-helper-terminal-lifecycle
branch
from
June 16, 2026 05:18
cce912b to
8451005
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CLI/CMUXCLI+TmuxCompatSupport.swift (1)
143-147:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a shorter retry budget for OMX HUD probe calls.
This probe path now inherits the full
readScreenPayloadretry window (~6s), which can stall tmux-compat commands on cold surfaces even though this function already has a nil fallback. Please make retry policy configurable and pass a small budget (or zero retries) here.Proposed change
--- a/CLI/CMUXCLI+ReadScreen.swift +++ b/CLI/CMUXCLI+ReadScreen.swift @@ - func readScreenPayload( + func readScreenPayload( client: SocketClient, - params: [String: Any] + params: [String: Any], + maxRetries: Int = 60, + retryDelayMicros: useconds_t = 100_000 ) throws -> [String: Any] { var readAttempt = 0 while true { do { return try client.sendV2(method: "surface.read_text", params: params) } catch let error as CLIError - where error.message.hasPrefix("terminal_not_ready:") && readAttempt < 60 { + where error.message.hasPrefix("terminal_not_ready:") && readAttempt < maxRetries { readAttempt += 1 - usleep(100_000) + usleep(retryDelayMicros) } } }--- a/CLI/CMUXCLI+TmuxCompatSupport.swift +++ b/CLI/CMUXCLI+TmuxCompatSupport.swift @@ - guard let payload = try? readScreenPayload(client: client, params: [ + guard let payload = try? readScreenPayload(client: client, params: [ "workspace_id": workspaceId, "surface_id": surfaceId, "lines": 4 - ]), + ], maxRetries: 3), let text = payload["text"] as? String else { return nil }As per coding guidelines,
algorithmic-complexity.mdrequires flagging unbenchmarked/high-latency choices in production paths over user data, and this change introduces a fixed multi-second retry loop into a heuristic CLI path.🤖 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 `@CLI/CMUXCLI`+TmuxCompatSupport.swift around lines 143 - 147, The readScreenPayload function call in the OMX HUD probe path inherits the full retry window (~6s) which can unnecessarily stall tmux-compat commands on cold surfaces, despite the nil fallback already being in place. Make the retry policy configurable in the readScreenPayload function signature by adding a parameter for retry budget or retry count, then pass a small retry budget (or zero retries) when calling readScreenPayload from this probe location to minimize latency while maintaining the nil fallback behavior.Source: Coding guidelines
🤖 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_cli_background_terminal_helpers_read_screen_starts_pty.py`:
- Around line 119-122: The test at lines 119-122 validates success by checking
that error phrases are not present in the output, but this causes false failures
when terminal text legitimately contains those strings. Since the exit code
check at line 118 already enforces success, remove the substring checks for
"Terminal surface not found" and "Failed to read terminal text" from the _must
assertion and rely solely on the exit code validation for the success path.
---
Outside diff comments:
In `@CLI/CMUXCLI`+TmuxCompatSupport.swift:
- Around line 143-147: The readScreenPayload function call in the OMX HUD probe
path inherits the full retry window (~6s) which can unnecessarily stall
tmux-compat commands on cold surfaces, despite the nil fallback already being in
place. Make the retry policy configurable in the readScreenPayload function
signature by adding a parameter for retry budget or retry count, then pass a
small retry budget (or zero retries) when calling readScreenPayload from this
probe location to minimize latency while maintaining the nil fallback behavior.
🪄 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: 64d725be-4873-4979-a191-803ecb9692f8
📒 Files selected for processing (13)
CLI/CMUXCLI+ReadScreen.swiftCLI/CMUXCLI+TmuxCompatSupport.swiftCLI/cmux.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Surface/ControlCommandCoordinator+Surface2.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Surface/ControlSurfaceReadTextResolution.swiftPackages/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorSurfaceReadTextTests.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/ComposerDictationTextMerge.swiftPackages/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface+BackgroundStartLifecycle.swiftSources/TerminalController+ControlSurfaceContext3.swiftSources/TerminalController+TerminalReadDemand.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxprojtests_v2/test_cli_background_terminal_helpers_read_screen_starts_pty.py
💤 Files with no reviewable changes (1)
- Sources/TerminalController.swift
Comment on lines
+119
to
+122
| _must( | ||
| "Terminal surface not found" not in output and "Failed to read terminal text" not in output, | ||
| f"{label}: read did not materialize {surface_ref}: {output!r}", | ||
| ) |
There was a problem hiding this comment.
Avoid payload substring checks for success-path terminal output.
At Line 120, the test treats successful stdout terminal content as an error signal by scanning for legacy error phrases. That can cause false failures when terminal text legitimately contains those strings. Line 118 already enforces success via exit code.
Suggested patch
def _assert_cli_read_materializes(cli: str, args: List[str], surface_ref: str, label: str) -> None:
code, output = _run_cli(
cli,
args,
check=False,
timeout=10.0,
)
_must(code == 0, f"{label}: read failed for {surface_ref}: {output!r}")
- _must(
- "Terminal surface not found" not in output and "Failed to read terminal text" not in output,
- f"{label}: read did not materialize {surface_ref}: {output!r}",
- )📝 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
| _must( | |
| "Terminal surface not found" not in output and "Failed to read terminal text" not in output, | |
| f"{label}: read did not materialize {surface_ref}: {output!r}", | |
| ) | |
| def _assert_cli_read_materializes(cli: str, args: List[str], surface_ref: str, label: str) -> None: | |
| code, output = _run_cli( | |
| cli, | |
| args, | |
| check=False, | |
| timeout=10.0, | |
| ) | |
| _must(code == 0, f"{label}: read failed for {surface_ref}: {output!r}") |
🤖 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 `@tests_v2/test_cli_background_terminal_helpers_read_screen_starts_pty.py`
around lines 119 - 122, The test at lines 119-122 validates success by checking
that error phrases are not present in the output, but this causes false failures
when terminal text legitimately contains those strings. Since the exit code
check at line 118 already enforces success, remove the substring checks for
"Terminal surface not found" and "Failed to read terminal text" from the _must
assertion and rely solely on the exit code validation for the success path.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Packages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Surface/ControlCommandCoordinator+Surface2.swift (1)
315-320:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLocalize the newly introduced
terminal_not_readymessage text.
"Terminal surface is starting"is newly added protocol-visible text and is still a bare English literal. Route it through the project’s localization flow and add matching catalog entries for all supported locales.As per coding guidelines, “All user-facing strings must be localized” and API/user-facing text changes must be localized across supported locales.
Suggested direction
- case .terminalNotReady(let id): + case .terminalNotReady(let id): return .err( code: "terminal_not_ready", - message: "Terminal surface is starting", + message: String(localized: "control.surface.readText.terminalNotReady", + defaultValue: "Terminal surface is starting"), data: .object(["surface_id": .string(id.uuidString)]) )Then add
control.surface.readText.terminalNotReadytoResources/Localizable.xcstringswith translations for all supported locales.🤖 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 `@Packages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Surface/ControlCommandCoordinator`+Surface2.swift around lines 315 - 320, The message string "Terminal surface is starting" in the terminalNotReady case of the error response is a user-facing string that must be localized. Replace the hardcoded string literal with a localization key reference using the project's localization system, then add the corresponding entry to Resources/Localizable.xcstrings with translations for all supported locales. The localization key should follow the naming convention suggested in the comment (such as control.surface.readText.terminalNotReady) to maintain consistency with the codebase's localization structure.Source: Coding guidelines
🤖 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 `@CLI/cmux.swift`:
- Around line 23027-23031: The contentSearchTerminalText function call on line
23030 is performing retry-heavy RPC work inside the per-workspace iteration
loop, causing O(workspaces × retry window) latency scaling as workspace count
grows. Fix this by first adding a short-circuit condition to skip the content
search when titleMatch is already true (since a title match is sufficient), then
refactor the contentSearchTerminalText implementation itself to use a bounded or
non-retrying read path instead of the current retryable operations, or
alternatively batch the content lookups across multiple workspaces to avoid
per-workspace RPC calls.
---
Duplicate comments:
In
`@Packages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Surface/ControlCommandCoordinator`+Surface2.swift:
- Around line 315-320: The message string "Terminal surface is starting" in the
terminalNotReady case of the error response is a user-facing string that must be
localized. Replace the hardcoded string literal with a localization key
reference using the project's localization system, then add the corresponding
entry to Resources/Localizable.xcstrings with translations for all supported
locales. The localization key should follow the naming convention suggested in
the comment (such as control.surface.readText.terminalNotReady) to maintain
consistency with the codebase's localization structure.
🪄 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: 0cf1b30f-6102-4322-afbf-bdd5b8c0ab37
📒 Files selected for processing (13)
CLI/CMUXCLI+ReadScreen.swiftCLI/CMUXCLI+TmuxCompatSupport.swiftCLI/cmux.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Surface/ControlCommandCoordinator+Surface2.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Surface/ControlSurfaceReadTextResolution.swiftPackages/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorSurfaceReadTextTests.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/ComposerDictationTextMerge.swiftPackages/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface+BackgroundStartLifecycle.swiftSources/TerminalController+ControlSurfaceContext3.swiftSources/TerminalController+TerminalReadDemand.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxprojtests_v2/test_cli_background_terminal_helpers_read_screen_starts_pty.py
azooz2003-bit
force-pushed
the
issue-helper-terminal-lifecycle
branch
2 times, most recently
from
June 16, 2026 05:54
9f4956f to
dd23350
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
CLI/cmux.swift (1)
23027-23031:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSkip content-read RPC when title already matches in the workspace scan.
At Line 23029,
contentSearchTerminalText(...)still runs even whentitleMatchis already true, adding avoidable per-workspace read calls on scalable lists (≈1000 workspaces).Suggested minimal fix
- if includeContent && !query.isEmpty, let wsId = ws["id"] as? String { + if includeContent && !query.isEmpty && !titleMatch, let wsId = ws["id"] as? String { let text = contentSearchTerminalText(workspaceId: wsId, client: client) ?? "" contentMatch = text.localizedCaseInsensitiveContains(query) }As per coding guidelines,
algorithmic-complexity.mdrequires avoiding per-target rescans/repeated expensive work in scalable workspace paths.🤖 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 `@CLI/cmux.swift` around lines 23027 - 23031, The contentSearchTerminalText(...) function call at line 23029 executes unnecessarily when titleMatch is already true, causing avoidable per-workspace read RPC calls. Add a !titleMatch check to the condition that guards the contentSearchTerminalText invocation so that the expensive content search is only performed when the title does not match the query.Source: Coding guidelines
🤖 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
`@Packages/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorSurfaceReadTextTests.swift`:
- Around line 56-68: The test startIfNeededTrueIsForwarded only verifies
surfaceID and startIfNeeded parameters, while other similar tests verify all
relevant forwarded parameters including includeScrollback and lineLimit. Add
assertions for includeScrollback and lineLimit by adding `#expect` statements that
verify context.lastRead?.includeScrollback and context.lastRead?.lineLimit match
the expected values sent in the request to make the test more thorough and
consistent with the other parameter verification tests.
---
Duplicate comments:
In `@CLI/cmux.swift`:
- Around line 23027-23031: The contentSearchTerminalText(...) function call at
line 23029 executes unnecessarily when titleMatch is already true, causing
avoidable per-workspace read RPC calls. Add a !titleMatch check to the condition
that guards the contentSearchTerminalText invocation so that the expensive
content search is only performed when the title does not match the query.
🪄 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: 2751911b-6e07-4164-9263-5c22d28d8bde
📒 Files selected for processing (16)
CLI/CMUXCLI+ReadScreen.swiftCLI/CMUXCLI+TmuxCompatSupport.swiftCLI/cmux.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Surface/ControlCommandCoordinator+Surface2.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Surface/ControlSurfaceContext.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Surface/ControlSurfaceReadTextResolution.swiftPackages/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandContextTestStubs.swiftPackages/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorSurfaceReadTextTests.swiftPackages/CmuxControlSocket/Tests/CmuxControlSocketTests/FakeSurfaceReadTextControlCommandContext.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/ComposerDictationTextMerge.swiftPackages/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface+BackgroundStartLifecycle.swiftSources/TerminalController+ControlSurfaceContext3.swiftSources/TerminalController+TerminalReadDemand.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxprojtests_v2/test_cli_background_terminal_helpers_read_screen_starts_pty.py
Comment on lines
+56
to
+68
| @Test func startIfNeededTrueIsForwarded() { | ||
| let (coordinator, context) = makeCoordinator() | ||
| let surfaceID = UUID() | ||
| context.readResolution = .terminalNotReady(surfaceID) | ||
|
|
||
| _ = coordinator.handle(request("surface.read_text", [ | ||
| "surface_id": .string(surfaceID.uuidString), | ||
| "start_if_needed": .bool(true), | ||
| ])) | ||
|
|
||
| #expect(context.lastRead?.surfaceID == surfaceID) | ||
| #expect(context.lastRead?.startIfNeeded == true) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider verifying all forwarded parameters for consistency.
Tests 1 and 2 verify all relevant forwarded parameters (surfaceID, includeScrollback, lineLimit, startIfNeeded), while this test only checks surfaceID and startIfNeeded. For consistency and more thorough verification, consider adding assertions for includeScrollback and lineLimit here as well.
📋 Suggested additions for consistency
`#expect`(context.lastRead?.surfaceID == surfaceID)
+ `#expect`(context.lastRead?.includeScrollback == false)
+ `#expect`(context.lastRead?.lineLimit == nil)
`#expect`(context.lastRead?.startIfNeeded == true)🤖 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
`@Packages/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorSurfaceReadTextTests.swift`
around lines 56 - 68, The test startIfNeededTrueIsForwarded only verifies
surfaceID and startIfNeeded parameters, while other similar tests verify all
relevant forwarded parameters including includeScrollback and lineLimit. Add
assertions for includeScrollback and lineLimit by adding `#expect` statements that
verify context.lastRead?.includeScrollback and context.lastRead?.lineLimit match
the expected values sent in the request to make the test more thorough and
consistent with the other parameter verification tests.
azooz2003-bit
force-pushed
the
issue-helper-terminal-lifecycle
branch
from
June 16, 2026 06:24
dd23350 to
20e704c
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests_v2/test_cli_background_terminal_helpers_read_screen_starts_pty.py (1)
119-122:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove brittle success-path substring filtering after exit-code success.
At Line 118 you already assert success (
code == 0). The extra phrase filter at Lines 119-122 can produce false failures when terminal content legitimately contains those strings.Suggested patch
def _assert_cli_read_materializes(cli: str, args: List[str], surface_ref: str, label: str) -> None: code, output = _run_cli( cli, args, check=False, timeout=10.0, ) _must(code == 0, f"{label}: read failed for {surface_ref}: {output!r}") - _must( - "Terminal surface not found" not in output and "Failed to read terminal text" not in output, - f"{label}: read did not materialize {surface_ref}: {output!r}", - )🤖 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 `@tests_v2/test_cli_background_terminal_helpers_read_screen_starts_pty.py` around lines 119 - 122, Remove the _must call that performs substring filtering at lines 119-122 (the call checking that "Terminal surface not found" and "Failed to read terminal text" are not in output). Since the exit code check at line 118 already validates success with code == 0, this additional substring-based validation is redundant and brittle, as it can produce false failures when the terminal output legitimately contains those strings. The exit code assertion is sufficient to confirm the operation succeeded.
🤖 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.
Duplicate comments:
In `@tests_v2/test_cli_background_terminal_helpers_read_screen_starts_pty.py`:
- Around line 119-122: Remove the _must call that performs substring filtering
at lines 119-122 (the call checking that "Terminal surface not found" and
"Failed to read terminal text" are not in output). Since the exit code check at
line 118 already validates success with code == 0, this additional
substring-based validation is redundant and brittle, as it can produce false
failures when the terminal output legitimately contains those strings. The exit
code assertion is sufficient to confirm the operation succeeded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4ef215dd-d162-450c-a0df-2cbdfa8cc9d1
📒 Files selected for processing (15)
CLI/CMUXCLI+ReadScreen.swiftCLI/CMUXCLI+TmuxCompatSupport.swiftCLI/cmux.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Surface/ControlCommandCoordinator+Surface2.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Surface/ControlSurfaceContext.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Surface/ControlSurfaceReadTextResolution.swiftPackages/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandContextTestStubs.swiftPackages/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorSurfaceReadTextTests.swiftPackages/CmuxControlSocket/Tests/CmuxControlSocketTests/FakeSurfaceReadTextControlCommandContext.swiftPackages/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface+BackgroundStartLifecycle.swiftSources/TerminalController+ControlSurfaceContext3.swiftSources/TerminalController+TerminalReadDemand.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxprojtests_v2/test_cli_background_terminal_helpers_read_screen_starts_pty.py
Comment on lines
+17
to
+37
| let semaphore = DispatchSemaphore(value: 0) | ||
| let observer = NotificationCenter.default.addObserver( | ||
| forName: .terminalSurfaceDidBecomeReady, | ||
| object: nil, | ||
| queue: nil | ||
| ) { notification in | ||
| guard notification.userInfo?["surfaceId"] as? UUID == surfaceID else { return } | ||
| semaphore.signal() | ||
| } | ||
| defer { | ||
| NotificationCenter.default.removeObserver(observer) | ||
| } | ||
|
|
||
| let secondResult = socketWorkerCoordinatorResult(for: request) | ||
| guard Self.retryableReadDemandSurfaceID(from: secondResult, request: request) == surfaceID else { | ||
| return surfaceReadTextSocketWorkerEncoder.response(id: request.id, secondResult) | ||
| } | ||
|
|
||
| guard semaphore.wait(timeout: .now() + 5) == .success else { | ||
| return surfaceReadTextSocketWorkerEncoder.response(id: request.id, secondResult) | ||
| } |
Contributor
There was a problem hiding this comment.
DispatchSemaphore.wait blocks production socket worker thread
DispatchSemaphore is explicitly prohibited by the cmux-swift-blocking-runtime rule in production Swift. This call blocks the socket worker thread for up to 5 seconds waiting for terminalSurfaceDidBecomeReady. While moving the wait off the main actor is the right direction, using a semaphore here creates a blocking thread, which can exhaust the thread pool if multiple concurrent cold-start read requests arrive simultaneously. The correct shape is to declare this function async, replace v2MainSync with await-based main-actor hops, and replace the semaphore with withCheckedContinuation (or an AsyncChannel) listening on terminalSurfaceDidBecomeReady.
Rule Used: Flag new blocking or timing-based synchronization ... (source)
lawrencecchen
added
the
stale-revisit
This was referenced Nov 2, 2026
Open
Open
Draft
Merged
Hover rules in Rust, step B: Swift calls the Rust hover rules; the Swift reducers are deleted
#31836
Open
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
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 #1472
Summary:
surface.read_textstays non-materializing by default.start_if_needed: trueand retry the transientterminal_not_readyerror while cold helper terminals start.find-window --contentand legacy tmux HUD metadata sniffing, passstart_if_needed: falseso scans do not spawn hidden PTYs.ensureTerminalSurfaceReadyForRead.terminal_not_readyandstart_if_needed, plus a live CLI regression for cold background helper panes.Red/green:
test: cover helper read-screen materializationfix: materialize helper terminals for read-screenread-screenagainst a cold background helper could fail withinternal_error: Failed to read terminal text; a later manual retry worked after the PTY appeared.read-screen,capture-pane, andpipe-panematerialize separate cold helper surfaces, while tmux pane-command metadata probing remains non-materializing.Checks run after rebasing onto current
origin/main:swift test --package-path Packages/CmuxControlSocketswift test --package-path Packages/CmuxTerminal --filter TerminalSurfaceRestoreSpawnSchedulerTestspython3 -m py_compile tests_v2/test_cli_background_terminal_helpers_read_screen_starts_pty.pypython3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv./scripts/lint-ios-package-conventions.sh./tests/test_ci_pbxproj_test_wiring.sh./scripts/check-pbxproj.shgit diff --check../../scripts/reload-cloud.sh --tag hread(no builder fleet configured; fell back to local tagged build)CMUX_SOCKET_PATH=/tmp/cmux-debug-hread.sock CMUXTERM_CLI="$HOME/Library/Developer/Xcode/DerivedData/cmux-hread/Build/Products/Debug/cmux DEV hread.app/Contents/Resources/bin/cmux" python3 tests_v2/test_cli_background_terminal_helpers_read_screen_starts_pty.pyCMUX_SOCKET_PATH=/tmp/cmux-debug-hread.sock CMUXTERM_CLI="$HOME/Library/Developer/Xcode/DerivedData/cmux-hread/Build/Products/Debug/cmux DEV hread.app/Contents/Resources/bin/cmux" python3 tests_v2/test_cli_background_terminal_helpers_start_pty.py/Users/abdulazizalbahar/Dev/Manaflow/cmuxterm-hq/skills/autoreview/scripts/autoreview --mode branch --base origin/mainTagged proof:
hread/tmp/cmux-debug-hread.sockand was moved toLG HDR 4K.cmux-assets/issue-helper-terminal-lifecycle/rebased-hread-20260615-232646/transcript.txtshowsread-screenmaterializingsurface:18whilein_window=false, sendingHREAD_REBASED_1781591206, reading the token output back, andtopreporting zsh/gitstatus processes for the helper surface.cmux-assets/issue-helper-terminal-lifecycle/rebased-hread-web-helper-20260615-233344/transcript.txtshows a focus-false helper pane (surface:19) in the livehreadworkspace accepting the same-worktreewebdev-server command, starting local Postgres, applying migrations, reaching Next.jsReady, and then exiting visibly because local Stack secret keys are missing. Finalsurface-healthkeepssurface:19 in_window=trueandtopreports zsh/gitstatus processes for it.workspace:52,pane:105,surface:304for the cold-background transcript andsurface:305for the visible web-helper transcript./cmux-assetswas read-only on this machine, so durable artifacts use the worktree fallback undercmux-assets/issue-helper-terminal-lifecycle/.screencaptureproduced a black image and Computer Use could not attach to the tagged CG window. Socket/read-screen proof is complete.No merge until explicit dogfood approval.