Harden E2E recording and dual-Xcode CI routing - #7835
Closed
lawrencecchen wants to merge 15 commits into
Closed
lawrencecchen wants to merge 15 commits into
lawrencecchen wants to merge 15 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:
📝 WalkthroughWalkthroughThe PR updates Tart E2E capture startup, switches package tests to a dual-Xcode runner, documents the runner variable, and adds replay-barrier-aware terminal resynchronization with parameterized terminal test fixtures. ChangesE2E video capture
Dual-Xcode runner configuration
Terminal resynchronization
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)Frontmost-aware capture startupsequenceDiagram
participant TartCaptureLoop
participant FrontmostHelper
participant CmuxApp
participant ScreenCapture
TartCaptureLoop->>FrontmostHelper: Check frontmost bundle identifier
FrontmostHelper->>CmuxApp: Observe active application
FrontmostHelper-->>TartCaptureLoop: Report cmuxterm.app.debug is frontmost
TartCaptureLoop->>ScreenCapture: Start JPEG frame capture
TartCaptureLoop->>TartCaptureLoop: Create RECORD_READY and verify recorder liveness
Terminal resync flowsequenceDiagram
participant ResyncTerminalOutput
participant MobileShellComposite
participant TerminalReplay
ResyncTerminalOutput->>MobileShellComposite: Request resync for surface
MobileShellComposite->>MobileShellComposite: Check replay barrier ownership
MobileShellComposite->>TerminalReplay: Request replay with new barrier token
Possibly related PRs
🚥 Pre-merge checks | ✅ 23 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (23 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 hardens E2E recording, updates macOS CI routing, and adjusts mobile terminal resync behavior.
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (12): Last reviewed commit: "Split legacy replay compatibility test" | 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 @.github/workflows/test-e2e.yml:
- Around line 405-430: Update the screencapture invocation in the recording loop
following the frontmost-application check to avoid terminating the subshell on a
single failure. Replace the `|| exit 1` behavior with diagnostic logging and
continuation to the next iteration, while preserving the loop’s liveness checks
and allowing subsequent frames to be captured.
🪄 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: 85d4e3c6-dce9-492b-9938-5e8fefcc4919
📒 Files selected for processing (1)
.github/workflows/test-e2e.yml
lawrencecchen
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?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 @.github/workflows/test-e2e.yml:
- Around line 445-457: Make the recorder liveness check non-fatal under set -e
so initialization failures reach the existing diagnostic block. Update the kill
-0 check immediately after RECORD_PID is assigned to tolerate a nonzero result,
then continue polling RECORD_READY and emit the existing error log and captured
recorder output when initialization fails.
🪄 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: 4225473a-2208-4779-b1af-4826bcd01c73
📒 Files selected for processing (4)
.github/workflows/test-e2e.ymlPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalReplayLifecycle.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftios/cmuxPackage/Tests/cmuxFeatureTests/cmuxFeatureTests.swift
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1ece89e. Configure here.
lawrencecchen
added
the
stale-revisit
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
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.
Recorded XCUITest could leave cmux
Running Backgroundbecause screen capture began before the app became frontmost. Compile a small AppKit probe and arm frame capture until the expected cmux bundle owns the foreground. Capture pauses whenever cmux is not frontmost, then resumes without a persistent AVFoundation session.The Tart image has Xcode 26.3 only, while
swift-package-testsneeds SDK 15 for the universal Ghostty helper and SDK 26 for package tests. Route that single dual-Xcode job throughMACOS_RUNNER_DUAL_XCODE, set to Blacksmith macOS 15.The iPhone suite also exposed cross-suite output contamination because multiple tests used
live-terminal. Give the resync test and router a unique surface ID.Recorder failure before fix: https://github.com/manaflow-ai/cmux/actions/runs/29082615968
Recorder pass on the same AWS host after fix: https://github.com/manaflow-ai/cmux/actions/runs/29083465895
iPhone failure before isolation: https://github.com/manaflow-ai/cmux/actions/runs/29083696229
iPhone pass after isolation: https://github.com/manaflow-ai/cmux/actions/runs/29084943707
Validation:
actionlint .github/workflows/test-e2e.yml;bash tests/test_ci_self_hosted_guard.sh. PR CI validates the final Tart/Blacksmith routing.Note
Medium Risk
Mobile terminal replay barrier coordination affects live output recovery on connected devices; changes are scoped and covered by new tests. CI/workflow edits are mostly runner routing and recording harness behavior.
Overview
Hardens macOS E2E screen recording so JPEG capture runs only while
com.cmuxterm.app.debugis frontmost (small AppKit probe + gatedscreencapture), avoiding early capture that left cmux Running Background. Startup checks use a ready marker instead of assuming the first frame succeeded.Routes
swift-package-teststhroughMACOS_RUNNER_DUAL_XCODEso the job can build the Ghostty helper on SDK 15 and run package tests on SDK 26; documents the new repo variable. Adds Node 22 on the focused app-host shard before CLI no-socket regressions.On mobile terminal sync, adds
requestTerminalResyncso recovery can defer behind an active replay barrier (or start a new barrier when none owns the surface); legacy hosts withoutterminal.replay.v1keep the unbarriered path.input_seq_behindresync uses deferral on raw-bytes transport. Tests cover legacy live output during deferred resync and tighten the sequence-ahead resync scenario with manual acknowledgements and an isolated surface ID.Reviewed by Cursor Bugbot for commit b1904ab. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Bug Fixes
Tests
Chores