cmuxTests: unbreak the build and re-sync the remote-tmux reorder/targeting suites - #8427
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe changes update RemoteTmux tests to drain queued incidental commands before correlating responses and assertions. A browser screenshot evaluator test now explicitly invokes its instance payload helper. ChangesRemoteTmux FIFO correlation
Browser test helper qualification
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 25✅ Passed checks (25 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 |
ejc3
marked this pull request as ready for review
July 18, 2026 19:26
Contributor
Greptile SummaryThis PR unblocks the
Confidence Score: 5/5Safe to merge — all changes are confined to the test target with no production source touched. Every change is test-only: a one-line compiler disambiguation in BrowserDesignModeScreenshotEvaluatorTests, new private drain helpers in RemoteTmuxWindowReorderTests, and an updated drain loop in RemoteTmuxMirrorTargetingTests. The product binary is unaffected. Both review-thread concerns were addressed in this revision. Guard limits prevent infinite loops in case of product regressions. Files Needing Attention: No files require special attention. Important Files Changed
Reviews (8): Last reviewed commit: "tests: stop the reorder drains from swal..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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 `@cmuxTests/RemoteTmuxMirrorTargetingTests.swift`:
- Around line 209-225: The test drain loop must process command results in FIFO
order, matching the command consumed by pendingCommandKindsForTesting.first.
Update the loop around pendingCommandKindsForTesting and handleMessageForTesting
so it examines only the queue head, handles it only when it is a .paneRects
command, and stops or waits when an incidental command precedes it; preserve the
existing windowId-to-paneId and size mapping.
🪄 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: fe6886f7-d31b-482a-b747-cbafc5f643ec
📒 Files selected for processing (3)
cmuxTests/BrowserDesignModeScreenshotEvaluatorTests.swiftcmuxTests/RemoteTmuxMirrorTargetingTests.swiftcmuxTests/RemoteTmuxWindowReorderTests.swift
Contributor
Author
|
@austinywang heads up — this PR fixes a Compile break (verified): #8393. It added Why nothing caught it: automatic CI has been paused since #8029 (the test workflows are Test drift: this PR also re-syncs Net: with this PR the |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmuxTests/RemoteTmuxWindowReorderTests.swift (1)
56-67: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBreak on non-incidental commands to avoid swallowing interleaved meaningful commands.
Both
drainPendingSetupCommandsandpublishWindowsblindly reply[]to any command that isn't.paneRects, which swallows unexpected meaningful commands (like.listWindows) instead of leaving them in the queue. They should explicitly stop on unexpected commands, just likedrainLeadingOtherdoes.
cmuxTests/RemoteTmuxWindowReorderTests.swift#L56-L67: Update the switch to handle.otherexplicitly andbreakondefault.cmuxTests/RemoteTmuxWindowReorderTests.swift#L124-L133: Replace this duplicate drain loop entirely with a call todrainLeadingOther(connection).🛠 Proposed fixes
For
drainPendingSetupCommands:private func drainPendingSetupCommands(_ connection: RemoteTmuxControlConnection) { var guardCount = 0 - while guardCount < 16, let kind = connection.pendingCommandKindsForTesting.first { + loop: while guardCount < 16, let kind = connection.pendingCommandKindsForTesting.first { guardCount += 1 switch kind { case .paneRects: reply(connection, lines: ["%0 0 0 80 24 1 off :0 \"ejc3-mac\""]) + case .other: + reply(connection, lines: []) default: - reply(connection, lines: []) + break loop } } }For
publishWindows:private func publishWindows(_ connection: RemoteTmuxControlConnection, order: [Int]) { reply(connection, lines: windowLines(order)) // Drain every follow-up (per-window paneRects AND the trailing `.other` // push `#7315` added) so the FIFO is empty before the reorder scenario; a // leftover would mis-correlate later positional replies. See // ``drainPendingSetupCommands``. - var guardCount = 0 - while guardCount < 16, let kind = connection.pendingCommandKindsForTesting.first { - guardCount += 1 - if case let .paneRects(windowId, _) = kind { - reply(connection, lines: ["%\(windowId * 10) 0 0 80 24 1 off :zsh"]) - } else { - reply(connection, lines: []) - } - } + drainLeadingOther(connection) }🤖 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/RemoteTmuxWindowReorderTests.swift` around lines 56 - 67, Update drainPendingSetupCommands in cmuxTests/RemoteTmuxWindowReorderTests.swift#L56-L67 to handle .other explicitly and stop on default instead of replying to unexpected commands; update publishWindows at cmuxTests/RemoteTmuxWindowReorderTests.swift#L124-L133 by replacing its duplicate drain loop with drainLeadingOther(connection), preserving meaningful queued commands.
🤖 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.
Outside diff comments:
In `@cmuxTests/RemoteTmuxWindowReorderTests.swift`:
- Around line 56-67: Update drainPendingSetupCommands in
cmuxTests/RemoteTmuxWindowReorderTests.swift#L56-L67 to handle .other explicitly
and stop on default instead of replying to unexpected commands; update
publishWindows at cmuxTests/RemoteTmuxWindowReorderTests.swift#L124-L133 by
replacing its duplicate drain loop with drainLeadingOther(connection),
preserving meaningful queued commands.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 08c4547e-616f-4a28-98b3-f6b11e06dace
📒 Files selected for processing (2)
cmuxTests/RemoteTmuxMirrorTargetingTests.swiftcmuxTests/RemoteTmuxWindowReorderTests.swift
ejc3
changed the title
ejc3
force-pushed
the
fix-cmuxtests-compile
branch
from
July 19, 2026 11:18
2203bbb to
0a58b6b
Compare
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 `@cmuxTests/RemoteTmuxWindowReorderTests.swift`:
- Around line 58-66: Update both command-draining loops in
cmuxTests/RemoteTmuxWindowReorderTests.swift:58-66 and
cmuxTests/RemoteTmuxWindowReorderTests.swift:124-132 to use loop labels and stop
on unhandled command kinds instead of replying with empty lines. In the first
loop, let the default switch case break the labeled loop, optionally handling
.other explicitly; in the second, replace the conditional with a switch that
replies only for .paneRects and .other, breaking by default for all other
commands.
- Around line 82-96: Update drainLeadingOther(_:) to track a guard count and
stop after a finite maximum number of iterations, while preserving its existing
handling for .other and .paneRects and its break behavior for other command
kinds. Ensure the loop cannot continue indefinitely if processing re-enqueues
commands.
🪄 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: bafb3021-929b-4aa6-a940-9c7d8621719d
📒 Files selected for processing (3)
cmuxTests/BrowserDesignModeScreenshotEvaluatorTests.swiftcmuxTests/RemoteTmuxMirrorTargetingTests.swiftcmuxTests/RemoteTmuxWindowReorderTests.swift
ejc3
force-pushed
the
fix-cmuxtests-compile
branch
2 times, most recently
from
July 20, 2026 01:56
6dd3bd5 to
169954d
Compare
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
ejc3
force-pushed
the
fix-cmuxtests-compile
branch
3 times, most recently
from
July 25, 2026 04:57
3d13f4f to
2aed00f
Compare
…anaflow-ai#7315) manaflow-ai#7315 (exact feed-forward sizing / verified pane geometry) changed a mirror window to publish only when its own paneRects reply lands, and those fetches are enqueued incrementally — window @2's fetch appears after @1 resolves. The test replied to a single snapshot of pending paneRects, so @2 never published, the mirror built one tab instead of two, and the reorder + windowOrder assertions failed (panelIds.count == 1, not 2). The product is correct — the sibling mirror suites and the multiplex fuzzer build multi-window mirrors green. This is a stale test setup: drain every paneRects fetch (bounded loop) so both windows publish, then the two-tab reorder holds. Red/green: on clean main the test fails with panelIds.count → 1 == 2; with the drain it passes (1 test). Test-only change; no product code touched.
…ndowReorderTests manaflow-ai#7315 (verified pane geometry) and the pane-border-status work changed the control-command stream the reorder/close state machine emits: a window-list publish now also enqueues a per-window paneRects refetch, and closing a window issues a border-status unsubscribe (a plain send(), kind .other). The suite drives the connection with positional commandNumber:0 replies, so an undrained follow-up sits at the FIFO head and swallows the reply meant for the reorder/ close list-windows recovery — the batch never recovers, the connection never reconnects, and retained panes never release. All 33 assertions across 9 tests failed on clean main for this one reason. Fix is test-only: publish helpers drain every follow-up (paneRects + .other), a drainLeadingOther helper clears them ahead of each correlated reply, and the exact-pending assertions compare with those incidental follow-ups filtered out. The product is correct — the multiplex fuzzer and the sibling mirror suites build multi-window mirrors and reorder/close them green. Red/green: clean main fails the suite with 33 issues; with this it passes 14/14. No product code changed. Broke in manaflow-ai#7315.
From the CodeRabbit/Greptile pass: - drainLeadingOther replied to every paneRects with a hardcoded `%0`; a re-published @2/@3 needs its own pane id (the `windowId * 10` convention publishWindows stages), or its pending layout can't publish. - reorderPending filtered incidentals globally, so a paneRects landing BETWEEN two list-windows (an ordering anomaly) would be elided and the equality assertion would still pass. Trim only TRAILING incidental follow-ups; an interleaved one now survives and fails the assertion. - The mirror-targeting rects drain iterated a stale snapshot while each reply consumes the FIFO head, so an incidental preceding a fetch could mis-correlate pane data. Drain strictly from the head and stop at the first correlated command.
Both drain helpers replied to whatever sat at the FIFO head, so a `listWindows` or `windowReorder` arriving early was consumed with an empty reply and its later positional result mis-correlated — the failure the drains exist to prevent. Each now answers only the incidental follow-ups (`paneRects`, `.other`) and stops at the first correlated command. `drainLeadingOther` also gains the bounded guard the other drains already had.
ejc3
force-pushed
the
fix-cmuxtests-compile
branch
from
July 31, 2026 06:07
2aed00f to
01f4372
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this fixes
Three failures in the
cmuxTeststarget that are invisible on a clean checkout because the target no longer compiles.1 — The test target doesn't build.
BrowserDesignModeScreenshotEvaluatorTestshas a locallet payload = try payload(from: prompt)that shadows thepayload(from:)helper, so a latertry payload(from: reducedPrompt)fails to compile ("cannot call value of non-function type"). That one line fails the whole target, which hides every other unit result. The shadowing call came in with #8393. Fixed by qualifying the second call asself.payload(from:).2 —
RemoteTmuxMirrorTargetingTests.programmaticMirrorReorder…builds one tab from two windows (panelIds.count == 1, not 2), so the reorder andwindowOrder == [2,1]assertions fail.3 —
RemoteTmuxWindowReorderTests— the suite fails (33 assertions across 9 tests): reorder/close recoveries never fire, the connection never reconnects, retained panes never release.Why 2 and 3 fail
Both suites drive the mirror connection with positional replies and assert on the pending-command queue. The mirror's control-command stream has moved since these setups were last updated: a window-list publish now also enqueues a per-window
paneRectsrefetch (a window publishes only when its rects land), and closing a window issues a border-status unsubscribe (a plainsend(), kind.other). An undrained follow-up sits at the FIFO head and swallows the reply meant for the next correlated command, so the state machine stalls.Nothing caught the drift on merge: automated CI has been paused since #8029 (the test workflows are
workflow_dispatch-only), and the compile break above means even a manual run dies before these tests execute.The product is correct — the multiplex fuzzer and the sibling mirror suites build and reorder multi-window mirrors green. These are stale test setups. The fixes are test-only:
paneRectswith its own window's pane id (windowId * 10) and stop at the first correlated command;Red / green
main(with only the one-line compile fix so the target builds):programmaticMirrorReorder…failspanelIds.count → 1 == 2, andRemoteTmuxWindowReorderTestsfails with 33 issues.xcodebuild test— 33 tests across the 3 affected suites,TEST SUCCEEDED.Summary by CodeRabbit