Repository navigation
Add Blacksmith iOS video recording mode - #6479
lawrencecchen wants to merge 22 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:
📝 WalkthroughWalkthroughTwo new CI recording platforms— ChangesiOS and Sync-Video Recording Workflow
Sequence Diagram(s)sequenceDiagram
actor Trigger as Workflow Dispatch
participant Workflow as reload-build.yml
participant Simulator as iOS Simulator
participant cmuxMacOS as cmux macOS debug CLI
participant ffmpeg as ffmpeg / simctl recordVideo
participant Artifact as artifact/
Trigger->>Workflow: platform=sync-video or ios-video, device_family, test_filter
alt ios-video
Workflow->>Simulator: select or create simulator (Python + xcrun simctl)
Workflow->>Simulator: boot + dark appearance
Workflow->>Workflow: xcodebuild build-for-testing
Workflow->>Simulator: simctl io recordVideo (background)
Workflow->>Workflow: xcodebuild test-without-building (test_filter)
Workflow->>Artifact: .xcresult + screenshot.png + metadata.json
else sync-video
Workflow->>Workflow: grant TCC + ensure ffmpeg
Workflow->>cmuxMacOS: enable iOS pairing + reload macOS side
Workflow->>Simulator: boot + launch iOS debug bundle
cmuxMacOS->>cmuxMacOS: create workspace/surface + mint attach URL
Workflow->>Simulator: open attach URL
Workflow->>ffmpeg: start macOS screen capture
Workflow->>Simulator: simctl io recordVideo
cmuxMacOS->>cmuxMacOS: send clear / desktop text / sync marker
Workflow->>ffmpeg: SIGINT stop recorders
ffmpeg->>Artifact: macOS.mp4 + ios.mp4
Workflow->>ffmpeg: stitch 1920x1080 combined MP4
Workflow->>Artifact: metadata.json + stitched video
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 2 warnings)
✅ Passed checks (19 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| rm -rf "$RESULT_BUNDLE" "$VIDEO_PATH" "$SCREENSHOT_PATH" | ||
|
|
||
| xcrun simctl shutdown "$SIMULATOR_ID" >/dev/null 2>&1 || true | ||
| xcrun simctl erase "$SIMULATOR_ID" |
There was a problem hiding this comment.
Erase wipes shared simulators
Medium Severity
The ios-video step picks an existing available simulator when a preferred device already exists, then runs xcrun simctl erase on that UDID. On persistent self-hosted runners, that wipes a shared simulator other jobs rely on, not only a dedicated cmux Video instance.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 4d97f30. Configure here.
| "tag": "$BUILD_TAG", | ||
| "platform": "ios-video", | ||
| "runner": "${{ inputs.runner }}", | ||
| "sourceRef": "${{ inputs.ref }}", |
There was a problem hiding this comment.
Metadata omits resolved source ref
Low Severity
metadata.json sets sourceRef from ${{ inputs.ref }} only. When the dispatch leaves ref empty, checkout still uses inputs.ref || github.ref, but metadata records an empty sourceRef instead of the ref that was actually built.
Reviewed by Cursor Bugbot for commit 4d97f30. Configure here.
| } >> "$GITHUB_STEP_SUMMARY" | ||
|
|
||
| - name: Upload artifact | ||
| if: ${{ always() }} |
There was a problem hiding this comment.
Failed builds upload partial artifacts
Low Severity
Adding if: ${{ always() }} to the upload step makes failed macos or ios runs publish an artifact directory that often contains only timings.json, not app.zip or archive.zip, which can look like a successful dev build to download logic that only checks for an artifact.
Reviewed by Cursor Bugbot for commit 4d97f30. Configure here.
Greptile SummaryThis PR adds
Confidence Score: 4/5Safe to merge with one runtime fix needed in record-real-sync-video.sh. The only new finding is scripts/ci/record-real-sync-video.sh — the Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant WF as reload-build.yml
participant SH as record-real-sync-video.sh
participant SIM as iOS Simulator
participant MAC as Tagged macOS cmux
participant IOS as iOS cmux app
WF->>SH: invoke (sync-video mode)
SH->>SIM: boot simulator
SH->>MAC: build + launch tagged macOS app
MAC-->>SH: socket ready (wait_for_socket)
SH->>MAC: workspace create (cmux_tagged)
SH->>MAC: mint attach URL (mobile.attach_ticket.create)
SH->>IOS: build + install (ios/scripts/reload.sh --no-launch)
SH->>SIM: seed CMUX_DOGFOOD_ATTACH_URL into UserDefaults/launchctl
SH->>SIM: launch iOS app (simctl launch)
Note over SH,IOS: sleep 18 (fixed wait for attachment)
SH->>SH: start_macos_recording + start_ios_recording
SH->>MAC: send terminal keystrokes (cmux_tagged send)
IOS-->>SIM: mirrors terminal content
SH->>SH: stop_recorders
SH->>SH: stitch_videos (ffmpeg side-by-side)
SH->>WF: metadata.json + MP4 artifacts
%%{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 WF as reload-build.yml
participant SH as record-real-sync-video.sh
participant SIM as iOS Simulator
participant MAC as Tagged macOS cmux
participant IOS as iOS cmux app
WF->>SH: invoke (sync-video mode)
SH->>SIM: boot simulator
SH->>MAC: build + launch tagged macOS app
MAC-->>SH: socket ready (wait_for_socket)
SH->>MAC: workspace create (cmux_tagged)
SH->>MAC: mint attach URL (mobile.attach_ticket.create)
SH->>IOS: build + install (ios/scripts/reload.sh --no-launch)
SH->>SIM: seed CMUX_DOGFOOD_ATTACH_URL into UserDefaults/launchctl
SH->>SIM: launch iOS app (simctl launch)
Note over SH,IOS: sleep 18 (fixed wait for attachment)
SH->>SH: start_macos_recording + start_ios_recording
SH->>MAC: send terminal keystrokes (cmux_tagged send)
IOS-->>SIM: mirrors terminal content
SH->>SH: stop_recorders
SH->>SH: stitch_videos (ffmpeg side-by-side)
SH->>WF: metadata.json + MP4 artifacts
Reviews (18): Last reviewed commit: "Open iOS workspace in sync recording" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| VIDEO_PATH="$GITHUB_WORKSPACE/artifact/cmux-ios-${BUILD_TAG}.mp4" | ||
| SCREENSHOT_PATH="$GITHUB_WORKSPACE/artifact/cmux-ios-${BUILD_TAG}.png" |
There was a problem hiding this comment.
BUILD_TAG used raw in file paths without slug sanitization
The ios build step (line 141) sanitizes inputs.tag into a filesystem-safe slug before using it in paths (tr -c 'a-z0-9-' '-'). The ios-video step uses BUILD_TAG directly in VIDEO_PATH and SCREENSHOT_PATH. If the tag contains / (e.g. feature/my-branch), these paths will reference a non-existent subdirectory and xcrun simctl io recordVideo will fail, then the final [ -s "$VIDEO_PATH" ] guard will report "video not produced" without explaining the real cause. The metadata "video" key will also receive only the basename of the broken path.
| if [ -n "${TEST_FILTER:-}" ]; then | ||
| ONLY_TESTING=(-only-testing:"$TEST_FILTER") | ||
| else | ||
| ONLY_TESTING=(-only-testing:cmuxUITests/cmuxUITests/testStackAuthEntryUsesStableIdentifiers) | ||
| fi |
There was a problem hiding this comment.
else branch in ONLY_TESTING
TEST_FILTER is always defined in the step's env: block from inputs.test_filter, which has a non-empty default value. The else branch (and the duplicate fallback in metadata.json) are therefore dead code. Removing the redundant branch makes the intent clearer and avoids the confusion of two apparently authoritative default values.
| if [ -n "${TEST_FILTER:-}" ]; then | |
| ONLY_TESTING=(-only-testing:"$TEST_FILTER") | |
| else | |
| ONLY_TESTING=(-only-testing:cmuxUITests/cmuxUITests/testStackAuthEntryUsesStableIdentifiers) | |
| fi | |
| ONLY_TESTING=(-only-testing:"$TEST_FILTER") |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| cat > "$GITHUB_WORKSPACE/artifact/metadata.json" <<JSON | ||
| { | ||
| "tag": "$BUILD_TAG", | ||
| "platform": "ios-video", | ||
| "runner": "${{ inputs.runner }}", | ||
| "sourceRef": "${{ inputs.ref }}", | ||
| "deviceFamily": "$DEVICE_FAMILY", | ||
| "simulatorId": "$SIMULATOR_ID", | ||
| "simulatorName": "$SIMULATOR_NAME", | ||
| "testFilter": "${TEST_FILTER:-cmuxUITests/cmuxUITests/testStackAuthEntryUsesStableIdentifiers}", | ||
| "video": "$(basename "$VIDEO_PATH")", | ||
| "testExitCode": $test_rc | ||
| } | ||
| JSON |
There was a problem hiding this comment.
$SIMULATOR_NAME, $TEST_FILTER, and $BUILD_TAG are expanded directly into the JSON heredoc. If any value contains " or \ (e.g. a test filter path hand-edited by the dispatcher), the output file will be malformed and the downstream HQ parser may fail silently. The timings.json heredoc above has the same pattern with ${{ inputs.tag }} and ${{ inputs.ref }} (also unescaped), so the risk is already present there, but metadata.json adds more free-form shell variables. Generating the JSON with python3 -c "import json, os, sys; ..." or a jq -n call would guarantee well-formed output regardless of input characters.
![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 @.github/workflows/reload-build.yml:
- Around line 266-267: The VIDEO_PATH and SCREENSHOT_PATH variables are using
the raw BUILD_TAG value which can contain invalid path characters like forward
slashes, spaces, or shell special characters that will cause artifact creation
to fail. Sanitize the BUILD_TAG variable by applying the same
slugging/sanitization pattern that is already being used elsewhere in the iOS
archive step of the workflow, and use the sanitized version when constructing
both VIDEO_PATH and SCREENSHOT_PATH.
- Around line 331-343: The metadata.json generation in the heredoc uses direct
variable expansion which can break JSON validity and enable injection attacks if
inputs contain special characters or newlines. Replace the current heredoc
approach with jq -n to safely construct the JSON object, passing all dynamic
values (BUILD_TAG, DEVICE_FAMILY, SIMULATOR_ID, SIMULATOR_NAME, TEST_FILTER, the
basename of VIDEO_PATH, and test_rc) either as environment variables or using jq
--arg flags to ensure proper escaping and prevent injection vulnerabilities
while maintaining JSON integrity.
🪄 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: 49bca67f-dd18-443b-8f14-b2a0ef27227c
📒 Files selected for processing (1)
.github/workflows/reload-build.yml
| VIDEO_PATH="$GITHUB_WORKSPACE/artifact/cmux-ios-${BUILD_TAG}.mp4" | ||
| SCREENSHOT_PATH="$GITHUB_WORKSPACE/artifact/cmux-ios-${BUILD_TAG}.png" |
There was a problem hiding this comment.
Sanitize BUILD_TAG before using it in artifact filenames.
VIDEO_PATH/SCREENSHOT_PATH use raw BUILD_TAG; tags containing /, spaces, or shell-special chars can create invalid/nested paths and fail artifact creation. Reuse the slugging pattern already used in the iOS archive step.
Suggested patch
- VIDEO_PATH="$GITHUB_WORKSPACE/artifact/cmux-ios-${BUILD_TAG}.mp4"
- SCREENSHOT_PATH="$GITHUB_WORKSPACE/artifact/cmux-ios-${BUILD_TAG}.png"
+ SAFE_TAG="$(printf '%s' "$BUILD_TAG" | tr '[:upper:]' '[:lower:]' | tr -c 'a-z0-9-' '-' | sed 's/-\{2,\}/-/g; s/^-//; s/-$//')"
+ VIDEO_PATH="$GITHUB_WORKSPACE/artifact/cmux-ios-${SAFE_TAG}.mp4"
+ SCREENSHOT_PATH="$GITHUB_WORKSPACE/artifact/cmux-ios-${SAFE_TAG}.png"🤖 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 @.github/workflows/reload-build.yml around lines 266 - 267, The VIDEO_PATH
and SCREENSHOT_PATH variables are using the raw BUILD_TAG value which can
contain invalid path characters like forward slashes, spaces, or shell special
characters that will cause artifact creation to fail. Sanitize the BUILD_TAG
variable by applying the same slugging/sanitization pattern that is already
being used elsewhere in the iOS archive step of the workflow, and use the
sanitized version when constructing both VIDEO_PATH and SCREENSHOT_PATH.
| cat > "$GITHUB_WORKSPACE/artifact/metadata.json" <<JSON | ||
| { | ||
| "tag": "$BUILD_TAG", | ||
| "platform": "ios-video", | ||
| "runner": "${{ inputs.runner }}", | ||
| "sourceRef": "${{ inputs.ref }}", | ||
| "deviceFamily": "$DEVICE_FAMILY", | ||
| "simulatorId": "$SIMULATOR_ID", | ||
| "simulatorName": "$SIMULATOR_NAME", | ||
| "testFilter": "${TEST_FILTER:-cmuxUITests/cmuxUITests/testStackAuthEntryUsesStableIdentifiers}", | ||
| "video": "$(basename "$VIDEO_PATH")", | ||
| "testExitCode": $test_rc | ||
| } |
There was a problem hiding this comment.
Harden metadata.json generation against template/script injection and malformed JSON.
Dynamic fields are injected directly into an unquoted heredoc (${{ ... }} and shell vars). This can break JSON and may allow template/script injection if dispatch inputs contain control characters/newlines. Build the JSON with jq -n (or Python) and pass all values via env/--arg instead of inline expansion.
Suggested patch
- name: Record iOS simulator video
if: ${{ inputs.platform == 'ios-video' }}
env:
BUILD_TAG: ${{ inputs.tag }}
DEVICE_FAMILY: ${{ inputs.device_family }}
TEST_FILTER: ${{ inputs.test_filter }}
+ INPUT_RUNNER: ${{ inputs.runner }}
+ INPUT_REF: ${{ inputs.ref }}
run: |
@@
- cat > "$GITHUB_WORKSPACE/artifact/metadata.json" <<JSON
- {
- "tag": "$BUILD_TAG",
- "platform": "ios-video",
- "runner": "${{ inputs.runner }}",
- "sourceRef": "${{ inputs.ref }}",
- "deviceFamily": "$DEVICE_FAMILY",
- "simulatorId": "$SIMULATOR_ID",
- "simulatorName": "$SIMULATOR_NAME",
- "testFilter": "${TEST_FILTER:-cmuxUITests/cmuxUITests/testStackAuthEntryUsesStableIdentifiers}",
- "video": "$(basename "$VIDEO_PATH")",
- "testExitCode": $test_rc
- }
- JSON
+ jq -n \
+ --arg tag "$BUILD_TAG" \
+ --arg platform "ios-video" \
+ --arg runner "$INPUT_RUNNER" \
+ --arg sourceRef "$INPUT_REF" \
+ --arg deviceFamily "$DEVICE_FAMILY" \
+ --arg simulatorId "$SIMULATOR_ID" \
+ --arg simulatorName "$SIMULATOR_NAME" \
+ --arg testFilter "${TEST_FILTER:-cmuxUITests/cmuxUITests/testStackAuthEntryUsesStableIdentifiers}" \
+ --arg video "$(basename "$VIDEO_PATH")" \
+ --argjson testExitCode "$test_rc" \
+ '{tag:$tag,platform:$platform,runner:$runner,sourceRef:$sourceRef,deviceFamily:$deviceFamily,simulatorId:$simulatorId,simulatorName:$simulatorName,testFilter:$testFilter,video:$video,testExitCode:$testExitCode}' \
+ > "$GITHUB_WORKSPACE/artifact/metadata.json"🧰 Tools
🪛 zizmor (1.25.2)
[error] 335-335: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 336-336: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 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 @.github/workflows/reload-build.yml around lines 331 - 343, The metadata.json
generation in the heredoc uses direct variable expansion which can break JSON
validity and enable injection attacks if inputs contain special characters or
newlines. Replace the current heredoc approach with jq -n to safely construct
the JSON object, passing all dynamic values (BUILD_TAG, DEVICE_FAMILY,
SIMULATOR_ID, SIMULATOR_NAME, TEST_FILTER, the basename of VIDEO_PATH, and
test_rc) either as environment variables or using jq --arg flags to ensure
proper escaping and prevent injection vulnerabilities while maintaining JSON
integrity.
Source: Linters/SAST tools
| xcrun simctl delete "$SIMULATOR_ID" >/dev/null 2>&1 || true | ||
| fi | ||
| } | ||
| trap cleanup_video_run EXIT |
There was a problem hiding this comment.
EXIT trap registered too late
Medium Severity
The ios-video step boots and erases a simulator, then runs build-for-testing, but the EXIT trap that shuts down (and deletes created) simulators is not installed until after that build. Any failure from boot through build-for-testing exits with set -e without running cleanup, leaving a simulator running on the runner.
Reviewed by Cursor Bugbot for commit 23ee095. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@ios/cmuxUITests/cmuxUITests.swift`:
- Around line 89-99: Replace the three fixed Thread.sleep calls in the test
method with predicate-based waits to make the test deterministic. For the first
sleep before input.tap(), wait for the input element to be hittable using
waitForExistence or hitPoint checks. For the second sleep after
typeText(command) and before typeText("\r"), remove the sleep since the previous
wait and tap operations should complete the input preparation. For the third
sleep after the terminal assertions at the end of the method, replace it with a
wait on an XCTNSPredicate that verifies the terminal's label contains the
expected marker rather than using a fixed timeout. Use XCUIApplication's
predicate waiting mechanisms with appropriate timeouts to ensure the UI is in
the expected state before proceeding.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileRootView.swift`:
- Around line 65-72: The shouldShowSyncTypingDemo property and the sync-typing
demo routing logic should be moved out of the production Sources directory since
they are debug/test-only seams that do not gate real production behavior.
Extract the shouldShowSyncTypingDemo property and the conditional branching at
the root-content path in CMUXMobileRootView from the production source code and
relocate this debug-only logic to an appropriate test or debug-only
module/configuration. This ensures the production code path remains clean and
debug facilities are properly isolated as per project guidelines.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/SyncTypingDemoView.swift`:
- Around line 11-14: The user-facing strings in the output array ("cmux iOS
terminal", "host: cloud macOS runner", and the string containing "$ \(draft)")
are not localized and must be wrapped with String(localized:...,
defaultValue:...) to comply with localization guidelines. For each string in the
array, wrap it with String(localized:) using an appropriate localization key and
set the defaultValue parameter to the current English string. Ensure the string
interpolation for draft is preserved within the localized string.
🪄 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: ba80b26b-0df8-4b04-b09c-55efa7f83773
📒 Files selected for processing (5)
.github/workflows/reload-build.ymlPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileRootView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/SyncTypingDemoView.swiftPackages/iOS/CmuxMobileSupport/Sources/CmuxMobileSupport/UITestConfig.swiftios/cmuxUITests/cmuxUITests.swift
| Thread.sleep(forTimeInterval: 1.0) | ||
| input.tap() | ||
| input.typeText(command) | ||
| Thread.sleep(forTimeInterval: 1.0) | ||
| input.typeText("\r") | ||
|
|
||
| let terminal = app.otherElements["MobileSyncDemoTerminal"] | ||
| XCTAssertTrue(terminal.waitForExistence(timeout: 4)) | ||
| XCTAssertTrue(terminal.label.contains(marker), "Expected demo terminal to contain \(marker). Label: \(terminal.label)") | ||
| Thread.sleep(forTimeInterval: 2.0) | ||
| } |
There was a problem hiding this comment.
Replace fixed sleeps with readiness-driven waits.
Line 89, Line 92, and Line 98 use fixed Thread.sleep, which makes this UI test timing-dependent and flaky under CI load. Wait on real predicates (focus/label state) instead.
As per coding guidelines, tests must not use fixed sleep to wait for async readiness; they should wait on completion signals or deadline-bounded predicate polls.
Proposed fix
- Thread.sleep(forTimeInterval: 1.0)
input.tap()
input.typeText(command)
- Thread.sleep(forTimeInterval: 1.0)
input.typeText("\r")
let terminal = app.otherElements["MobileSyncDemoTerminal"]
XCTAssertTrue(terminal.waitForExistence(timeout: 4))
- XCTAssertTrue(terminal.label.contains(marker), "Expected demo terminal to contain \(marker). Label: \(terminal.label)")
- Thread.sleep(forTimeInterval: 2.0)
+ let expectation = XCTNSPredicateExpectation(
+ predicate: NSPredicate { object, _ in
+ guard let terminal = object as? XCUIElement else { return false }
+ return terminal.label.contains(marker)
+ },
+ object: terminal
+ )
+ XCTAssertEqual(XCTWaiter.wait(for: [expectation], timeout: 6), .completed)🤖 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 `@ios/cmuxUITests/cmuxUITests.swift` around lines 89 - 99, Replace the three
fixed Thread.sleep calls in the test method with predicate-based waits to make
the test deterministic. For the first sleep before input.tap(), wait for the
input element to be hittable using waitForExistence or hitPoint checks. For the
second sleep after typeText(command) and before typeText("\r"), remove the sleep
since the previous wait and tap operations should complete the input
preparation. For the third sleep after the terminal assertions at the end of the
method, replace it with a wait on an XCTNSPredicate that verifies the terminal's
label contains the expected marker rather than using a fixed timeout. Use
XCUIApplication's predicate waiting mechanisms with appropriate timeouts to
ensure the UI is in the expected state before proceeding.
Source: Coding guidelines
| xcrun simctl terminate "$SIMULATOR_ID" "$IOS_BUNDLE_ID" >/dev/null 2>&1 || true | ||
| xcrun simctl launch "$SIMULATOR_ID" "$IOS_BUNDLE_ID" >/dev/null | ||
| sleep 2 | ||
| xcrun simctl openurl "$SIMULATOR_ID" "$ATTACH_URL" |
There was a problem hiding this comment.
iOS install uses different simulator
Medium Severity
Sync-video selects a simulator by UDID, boots it, and records from that UDID, but ios/scripts/reload.sh is invoked with --simulator name only and resolves the first available device with that name. When multiple simulators share the same display name, the built app may be installed on a different UDID than the one launched, opened, and recorded.
Reviewed by Cursor Bugbot for commit 435b4cc. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@scripts/ci/record-real-sync-video.sh`:
- Around line 291-294: The sleep 2 (after the xcrun simctl launch command for
"$SIMULATOR_ID" and "$IOS_BUNDLE_ID") and sleep 5 (after the xcrun simctl
openurl command) are fixed wall-clock delays used to wait for app-launch and
URL-handling readiness, which are unreliable and cause flakiness. Replace these
fixed delays with proper readiness signals by polling the app's log state,
checking the simulator process state, or verifying the attach handshake has
completed, rather than waiting for elapsed time to pass.
- Around line 196-198: The `sleep 2` followed by a single `kill -0` check is an
unreliable wall-clock synchronization hack that does not actually verify the
macOS recorder is ready; replace this with active polling of the MAC_RECORD_LOG
file to check for ffmpeg's actual readiness signal (such as "Recording started"
output), similar to how the `start_ios_recording` function handles process
readiness. Remove the `sleep 2` and `kill -0` commands and instead implement a
polling loop that reads MAC_RECORD_LOG and waits until the expected ffmpeg
startup message appears or a timeout is reached.
- Around line 32-34: Replace BUILD_TAG with TAG_SLUG in the variable assignments
for MAC_RAW_VIDEO, IOS_RAW_VIDEO, and FINAL_VIDEO. The raw BUILD_TAG can contain
special characters like slashes, spaces, or other characters that create invalid
file paths and cause artifact upload failures, while TAG_SLUG (already computed
earlier in the script) is the sanitized version designed for safe use in
filenames.
- Around line 247-248: The `eval "$(select_simulator)"` pattern swallows
failures because command substitution doesn't trigger errexit. Instead, capture
the output of select_simulator into a temporary variable or file, check its exit
status explicitly, and only then eval or source that output. This ensures that
if select_simulator fails (e.g., when no iOS simulator runtime is available),
the script will exit with an error rather than proceeding with empty
SIMULATOR_ID, SIMULATOR_NAME, and SIMULATOR_CREATED variables that cause
confusing downstream failures in xcrun simctl erase and recording steps.
- Around line 318-338: The Python here-document passes shell variables
(BUILD_TAG, SIMULATOR_NAME, WORKSPACE_ID, SYNC_MARKER, etc.) through direct
interpolation into the JSON payload, creating a security risk where special
characters or command substitutions in these variables could corrupt the JSON or
execute arbitrary code. Instead of shell-expanding variables directly into the
Python source, pass them as command-line arguments to the Python script using
sys.argv or via os.environ, then reference them within the Python code using
these safe mechanisms rather than relying on shell interpolation.
🪄 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: 2f35e361-1563-4b70-a26e-618eaeec5abd
📒 Files selected for processing (2)
.github/workflows/reload-build.ymlscripts/ci/record-real-sync-video.sh
| eval "$(select_simulator)" | ||
| export SIMULATOR_ID SIMULATOR_NAME SIMULATOR_CREATED |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
eval "$(select_simulator)" swallows failures under set -e.
A command substitution failure in this position does not trigger errexit. If select_simulator raises SystemExit (e.g. "No available iOS simulator runtime"), stdout is empty, eval is a no-op, and the script proceeds with an empty SIMULATOR_ID into xcrun simctl erase "" and recording — producing confusing downstream failures. Capture and check explicitly (the ios-video yml step avoids this by redirecting to a file and .-sourcing it).
🛠️ Proposed fix
-eval "$(select_simulator)"
+sim_env="$(select_simulator)" || { echo "simulator selection failed" >&2; exit 1; }
+eval "$sim_env"
export SIMULATOR_ID SIMULATOR_NAME SIMULATOR_CREATED
+[[ -n "$SIMULATOR_ID" ]] || { echo "no simulator id resolved" >&2; exit 1; }📝 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.
| eval "$(select_simulator)" | |
| export SIMULATOR_ID SIMULATOR_NAME SIMULATOR_CREATED | |
| sim_env="$(select_simulator)" || { echo "simulator selection failed" >&2; exit 1; } | |
| eval "$sim_env" | |
| export SIMULATOR_ID SIMULATOR_NAME SIMULATOR_CREATED | |
| [[ -n "$SIMULATOR_ID" ]] || { echo "no simulator id resolved" >&2; exit 1; } |
🤖 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 `@scripts/ci/record-real-sync-video.sh` around lines 247 - 248, The `eval
"$(select_simulator)"` pattern swallows failures because command substitution
doesn't trigger errexit. Instead, capture the output of select_simulator into a
temporary variable or file, check its exit status explicitly, and only then eval
or source that output. This ensures that if select_simulator fails (e.g., when
no iOS simulator runtime is available), the script will exit with an error
rather than proceeding with empty SIMULATOR_ID, SIMULATOR_NAME, and
SIMULATOR_CREATED variables that cause confusing downstream failures in xcrun
simctl erase and recording steps.
| xcrun simctl shutdown "$SIMULATOR_ID" >/dev/null 2>&1 || true | ||
| xcrun simctl erase "$SIMULATOR_ID" | ||
| xcrun simctl boot "$SIMULATOR_ID" >/dev/null 2>&1 || true | ||
| xcrun simctl bootstatus "$SIMULATOR_ID" -b |
There was a problem hiding this comment.
Simulator boot lacks timeout
Medium Severity
The ios-video step runs xcrun simctl bootstatus … -b with no timeout, while sync-video wraps the same wait in timeout 120s. A stuck simulator boot can hold the job until the 60-minute workflow limit instead of failing fast with logs.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 67e7023. Configure here.
| const { attachURL } = buildAttachURL(JSON.parse(process.env.PAYLOAD), { routeKind: "debug_loopback" }); | ||
| process.stdout.write(attachURL); | ||
| NODE | ||
| return 0 |
There was a problem hiding this comment.
Attach URL mint skips retries
Medium Severity
mint_attach_url treats any non-empty RPC payload as success and returns immediately after node, without checking that a URL was written or retrying when buildAttachURL fails (e.g. debug_loopback route not ready). Transient failures abort the whole sync run instead of polling like dev-setup.sh.
Reviewed by Cursor Bugbot for commit 67e7023. Configure here.
| MAC_RAW_VIDEO="$ARTIFACT_DIR/cmux-macos-${BUILD_TAG}.mp4" | ||
| IOS_RAW_VIDEO="$ARTIFACT_DIR/cmux-ios-${BUILD_TAG}.mp4" | ||
| FINAL_VIDEO="$ARTIFACT_DIR/cmux-real-sync-left-right-${BUILD_TAG}.mp4" |
There was a problem hiding this comment.
$BUILD_TAG used raw in video output paths while TAG_SLUG is already computed just above for filesystem-safe slugs. A tag containing / (e.g. feature/my-branch) produces a path like $ARTIFACT_DIR/cmux-macos-feature/my-branch.mp4 whose parent directory never gets created, causing ffmpeg and simctl to fail with a misleading "no such file or directory" error.
| MAC_RAW_VIDEO="$ARTIFACT_DIR/cmux-macos-${BUILD_TAG}.mp4" | |
| IOS_RAW_VIDEO="$ARTIFACT_DIR/cmux-ios-${BUILD_TAG}.mp4" | |
| FINAL_VIDEO="$ARTIFACT_DIR/cmux-real-sync-left-right-${BUILD_TAG}.mp4" | |
| MAC_RAW_VIDEO="$ARTIFACT_DIR/cmux-macos-${TAG_SLUG}.mp4" | |
| IOS_RAW_VIDEO="$ARTIFACT_DIR/cmux-ios-${TAG_SLUG}.mp4" | |
| FINAL_VIDEO="$ARTIFACT_DIR/cmux-real-sync-left-right-${TAG_SLUG}.mp4" |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 8 total unresolved issues (including 7 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit af7537a. Configure here.
| set -e | ||
| if ! grep -Eq 'Executed [1-9][0-9]* tests?, with 0 failures' "$TEST_LOG"; then | ||
| echo "ios-video did not execute any selected UI tests" >&2 | ||
| test_rc=65 |
There was a problem hiding this comment.
Failed UI tests misreported
Medium Severity
The post-test grep treats any log line that is not Executed … with 0 failures as “no UI tests ran,” so a failing test overwrites xcodebuild’s exit code with 65 and prints the wrong stderr message. metadata.json’s testExitCode can misreport real UI failures.
Reviewed by Cursor Bugbot for commit af7537a. Configure here.
| run_with_timeout 90 scripts/mobile-dev-launch.sh --tag "$BUILD_TAG" --simulator "$SIMULATOR_NAME" --attach --detach | ||
| unset CMUX_DOGFOOD_ATTACH_URL | ||
| sleep 10 |
There was a problem hiding this comment.
mobile-dev-launch.sh is called with --detach, meaning it exits before the iOS app has established its terminal connection. The hard-coded sleep 10 that follows is the only mechanism ensuring the app is attached before start_ios_recording and the terminal-send sequence begin. On a slow macOS runner — or any runner where the iOS simulator takes longer than 10 s to mount the app and complete attachment — recording begins with an empty/disconnected iOS screen, producing a demo video that silently shows the wrong state. There is no fallback: if the app is not attached within 10 s, the script continues regardless.
A bounded readiness loop polling a real signal (e.g. cmux_tagged read-screen succeeding on the iOS surface, or querying the iOS app's terminal session status via the RPC) would guarantee attachment before recording starts, matching the pattern already used for the macOS workspace at lines 358-363.
| python3 - "$METADATA_PATH" <<PY | ||
| import json | ||
| import pathlib | ||
| import sys | ||
|
|
||
| path = pathlib.Path(sys.argv[1]) | ||
| path.write_text(json.dumps({ | ||
| "tag": "$BUILD_TAG", | ||
| "platform": "sync-video", | ||
| "mode": "real-cmux-desktop-ios", | ||
| "deviceFamily": "$DEVICE_FAMILY", | ||
| "simulatorId": "$SIMULATOR_ID", | ||
| "simulatorName": "$SIMULATOR_NAME", | ||
| "workspaceId": "$WORKSPACE_ID", | ||
| "surfaceId": "$SURFACE_ID", | ||
| "syncMarker": "$SYNC_MARKER", | ||
| "video": "$(basename "$FINAL_VIDEO")", | ||
| "macVideo": "$(basename "$MAC_RAW_VIDEO")", | ||
| "iosVideo": "$(basename "$IOS_RAW_VIDEO")", | ||
| }, indent=2) + "\n") | ||
| PY |
There was a problem hiding this comment.
The metadata heredoc uses <<PY (unquoted), so the shell expands $BUILD_TAG, $SIMULATOR_NAME, $WORKSPACE_ID, $SURFACE_ID, and friends directly into Python string literals before Python ever runs. If any of those values contains a backslash, double-quote, or newline — $BUILD_TAG is user-controlled via inputs.tag — the generated Python source is syntactically invalid, Python exits non-zero, metadata.json is never written, and the workflow step fails with a confusing SyntaxError.
Compare mint_attach_url at line 192, which correctly uses <<'PY' (quoted, suppresses expansion) and passes values through sys.argv[]. The same pattern should be used here: quote the delimiter and thread each shell variable in via a positional argument or an env-var key that Python reads with os.environ.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Adds an
ios-videomode to the existingreload-build.ymlworkflow.The mode boots an iOS Simulator on a Blacksmith macOS runner, builds cmux for testing, starts
xcrun simctl io recordVideo, runs a selected XCUITest to drive the app, stops the recorder, and uploads the mp4 plus logs/metadata.Verified with local hq dispatcher:
/Users/lawrence/fun/cmuxterm-hq/artifacts/ios-video/blacksmith-27859047400/cmux-ios-vid.mp4cmuxUITests/cmuxUITests/testStackAuthEntryUsesStableIdentifiersNeed help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Mostly dispatch-only CI, but it changes DEBUG mobile launch/pairing behavior and runs privileged TCC DB writes on macOS runners during sync-video.
Overview
Extends
reload-buildwithios-videoandsync-videoplatform choices, plustest_filteranddevice_familyinputs for simulator-driven recordings.ios-videoboots a simulator, runs a selected UI test whilesimctl recordVideocaptures H.264, and uploads MP4, logs,xcresult, andmetadata.json(failing the job if no tests ran).sync-videogrants screen-capture TCC forffmpeg, then runs newscripts/ci/record-real-sync-video.shto build tagged macOS and iOS apps, mint an attach URL, auto-launch iOS with fixture auth, record Mac frames via debug screenshots and iOS viasimctl, and stitch a side-by-side demo MP4. Artifact upload now runsalways()so partial logs survive failures.On iOS DEBUG,
CMUX_DOGFOOD_ATTACH_URLcan be read from env,--cmux-dogfood-attach-url, orUserDefaults(with tests for precedence).connectUITestAttachURLIfNeededno longer requires Stack sign-in for raw attach tickets—it uses the same attach-ticket path as.onOpenURL—and runs fromonAppearbefore stored-Mac reconnect, withOSLogaround attach handling.Reviewed by Cursor Bugbot for commit 0af62f1. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds
ios-videoandsync-videomodes toreload-build.ymlto record iOS simulator runs and synchronized desktop+iOS demos. Also auto‑opens the selected workspace on iOS after attach and improves attach URL sourcing (env,--cmux-dogfood-attach-url, orUserDefaults) for reliable cloud pairing.New Features
ios-video: records H.264 viasimctl io recordVideowhile a selected UI test runs; uploads MP4, screenshot, logs,xcresult, andmetadata.json(includes test exit code). Inputs:test_filter,device_family.sync-video: provisionsGhosttyKit, grants TCC forffmpeg/screencapture, builds tagged macOS+iOS apps (iOS with--no-launch), mints an attach URL, seeds it into iOS defaults/launch args/env, auto‑attaches, opens the selected workspace on iOS, records macOS via terminal snapshots and iOS viasimctl, then stitches a 24s 1080p side‑by‑side MP4 withscripts/ci/record-real-sync-video.sh.CMUX_DOGFOOD_ATTACH_URLfrom env, launch arg--cmux-dogfood-attach-url, orUserDefaults; launch‑time raw tickets go through the sameconnectAttachURLpath as.onOpenURL, bypassing Stack sign‑in, with added attach‑flow logging.Bug Fixes
bootstatus, artifact upload underalways().Written for commit 0af62f1. Summary will update on new commits.
Summary by CodeRabbit