Repository navigation
Fix display harness manifest handoff in UI test - #1724
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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:
📝 WalkthroughWalkthroughReplaces per-run HARNESS_DIR plumbing with a fixed helper binary at Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI Workflow
participant FS as Filesystem (/tmp)
participant Xcode as xcodebuild / Test Runner
participant Test as DisplayResolutionRegressionUITests
CI->>FS: build/link helper -> /tmp/create-virtual-display
CI->>Xcode: invoke xcodebuild with CMUX_UI_TEST_DISPLAY_HELPER_BINARY_PATH=/tmp/create-virtual-display
Xcode->>Test: launch tests (env includes helper path or manifest path)
Test->>FS: check env CMUX_UI_TEST_DISPLAY_HELPER_BINARY_PATH
alt helper present
Test->>FS: execute/interact with helper at /tmp/create-virtual-display
else helper absent
Test->>FS: read manifest from CMUX_UI_TEST_DISPLAY_HARNESS_MANIFEST_PATH or default
alt manifest provides helperBinaryPath
Test->>FS: execute/interact with helper from manifest
else fallback
Test->>FS: use per-test tempPrefix artifact paths (ready/start/done/log) as before
end
end
Test-->>Xcode: tests proceed with harness integrated
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmuxUITests/DisplayResolutionRegressionUITests.swift">
<violation number="1" location="cmuxUITests/DisplayResolutionRegressionUITests.swift:125">
P1: The test now deletes externally provided helper binaries by storing the env path in `helperBinaryPath` and later removing it during teardown.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/test-e2e.yml">
<violation number="1" location=".github/workflows/test-e2e.yml:220">
P1: This heredoc terminator is indented, so Bash does not close the heredoc and the workflow step fails with a syntax error.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Comment on lines
220
to
+222
| cat >"$MANIFEST_PATH" <<EOF | ||
| {"readyPath":"$READY_PATH","displayIDPath":"$DISPLAY_ID_PATH","startPath":"$START_PATH","donePath":"$DONE_PATH","logPath":"$LOG_PATH"} | ||
| EOF | ||
| echo "Display harness manifest: $MANIFEST_PATH" | ||
| cat "$MANIFEST_PATH" | ||
|
|
||
| DISPLAY_ENV_PREFIX=( | ||
| CMUX_UI_TEST_DISPLAY_READY_PATH="$READY_PATH" | ||
| CMUX_UI_TEST_DISPLAY_ID_PATH="$DISPLAY_ID_PATH" | ||
| CMUX_UI_TEST_DISPLAY_START_PATH="$START_PATH" | ||
| CMUX_UI_TEST_DISPLAY_DONE_PATH="$DONE_PATH" | ||
| CMUX_UI_TEST_DISPLAY_LOG_PATH="$LOG_PATH" | ||
| ) | ||
| {"helperBinaryPath":"$HELPER_PATH"} | ||
| EOF |
There was a problem hiding this comment.
P1: This heredoc terminator is indented, so Bash does not close the heredoc and the workflow step fails with a syntax error.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/test-e2e.yml, line 220:
<comment>This heredoc terminator is indented, so Bash does not close the heredoc and the workflow step fails with a syntax error.</comment>
<file context>
@@ -209,13 +209,17 @@ jobs:
- DISPLAY_ENV_PREFIX=(
- CMUX_UI_TEST_DISPLAY_HELPER_BINARY_PATH="$HELPER_PATH"
- )
+ cat >"$MANIFEST_PATH" <<EOF
+ {"helperBinaryPath":"$HELPER_PATH"}
+ EOF
</file context>
Suggested change
| cat >"$MANIFEST_PATH" <<EOF | |
| {"readyPath":"$READY_PATH","displayIDPath":"$DISPLAY_ID_PATH","startPath":"$START_PATH","donePath":"$DONE_PATH","logPath":"$LOG_PATH"} | |
| EOF | |
| echo "Display harness manifest: $MANIFEST_PATH" | |
| cat "$MANIFEST_PATH" | |
| DISPLAY_ENV_PREFIX=( | |
| CMUX_UI_TEST_DISPLAY_READY_PATH="$READY_PATH" | |
| CMUX_UI_TEST_DISPLAY_ID_PATH="$DISPLAY_ID_PATH" | |
| CMUX_UI_TEST_DISPLAY_START_PATH="$START_PATH" | |
| CMUX_UI_TEST_DISPLAY_DONE_PATH="$DONE_PATH" | |
| CMUX_UI_TEST_DISPLAY_LOG_PATH="$LOG_PATH" | |
| ) | |
| {"helperBinaryPath":"$HELPER_PATH"} | |
| EOF | |
| printf '{"helperBinaryPath":"%s"}\n' "$HELPER_PATH" >"$MANIFEST_PATH" |
![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
🧹 Nitpick comments (2)
.github/workflows/test-e2e.yml (1)
211-222: Pass the manifest path explicitly intoxcodebuildto avoid default-path coupling.The manifest is created at
MANIFEST_PATH, but the test process still depends on the default path becauseDISPLAY_ENV_PREFIXis never populated withCMUX_UI_TEST_DISPLAY_HARNESS_MANIFEST_PATH.Proposed patch
if [ "$TEST_FILTER" = "DisplayResolutionRegressionUITests" ]; then HELPER_PATH="/tmp/create-virtual-display" MANIFEST_PATH="/tmp/cmux-ui-test-display-harness.json" rm -f "$MANIFEST_PATH" trap 'rm -f "$MANIFEST_PATH"' EXIT clang -framework Foundation -framework CoreGraphics \ -o "$HELPER_PATH" scripts/create-virtual-display.m cat >"$MANIFEST_PATH" <<EOF {"helperBinaryPath":"$HELPER_PATH"} EOF + + DISPLAY_ENV_PREFIX=("CMUX_UI_TEST_DISPLAY_HARNESS_MANIFEST_PATH=$MANIFEST_PATH") fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test-e2e.yml around lines 211 - 222, The test manifest is written to MANIFEST_PATH but the manifest path is never passed into xcodebuild because DISPLAY_ENV_PREFIX / CMUX_UI_TEST_DISPLAY_HARNESS_MANIFEST_PATH is not set; update the workflow so when invoking xcodebuild you explicitly inject the manifest path environment variable (CMUX_UI_TEST_DISPLAY_HARNESS_MANIFEST_PATH) or set DISPLAY_ENV_PREFIX to include that mapping, referencing MANIFEST_PATH and the xcodebuild invocation so the test process reads the correct manifest instead of relying on a default path..github/workflows/ci.yml (1)
468-486: ExportCMUX_UI_TEST_DISPLAY_HARNESS_MANIFEST_PATHin this step for deterministic handoff.Right now the UI test depends on the in-test fallback path. Passing the manifest path explicitly removes that implicit dependency.
Proposed patch
cat >"$MANIFEST_PATH" <<EOF {"helperBinaryPath":"$HELPER_PATH"} EOF - xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux -configuration Debug \ + CMUX_UI_TEST_DISPLAY_HARNESS_MANIFEST_PATH="$MANIFEST_PATH" \ + xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux -configuration Debug \ -clonedSourcePackagesDirPath "$SOURCE_PACKAGES_DIR" \ -disableAutomaticPackageResolution \ -destination "platform=macOS" \ -only-testing:cmuxUITests/DisplayResolutionRegressionUITests \ test🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 468 - 486, Export the manifest path so the UI test receives it deterministically: set and export the environment variable CMUX_UI_TEST_DISPLAY_HARNESS_MANIFEST_PATH to the MANIFEST_PATH used in this step (where MANIFEST_PATH and HELPER_PATH are defined) before invoking xcodebuild, ensuring the xcodebuild/test invocation (the xcodebuild command and the only-testing cmuxUITests/DisplayResolutionRegressionUITests) picks up the explicit manifest instead of relying on the in-test fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxUITests/DisplayResolutionRegressionUITests.swift`:
- Around line 124-133: The teardown currently always removes helperBinaryPath
even when it was provided externally; update the flow that sets helperBinaryPath
(in branches using loadExternalHarnessFromEnvironment or
loadExternalHarnessFromManifest and when helperBinaryPath is non-empty) to mark
ownership (e.g., set a Boolean flag like isHelperExternallyOwned or store an
ownership enum alongside helperBinaryPath), and change the teardown/cleanup
logic (the method that currently removes helperBinaryPath after
launchDisplayHelper) to skip deleting the file when the ownership flag indicates
an external/prebuilt helper; ensure symbols to update include helperBinaryPath,
loadExternalHarnessFromEnvironment, loadExternalHarnessFromManifest,
launchDisplayHelper, and the teardown/cleanup method.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 468-486: Export the manifest path so the UI test receives it
deterministically: set and export the environment variable
CMUX_UI_TEST_DISPLAY_HARNESS_MANIFEST_PATH to the MANIFEST_PATH used in this
step (where MANIFEST_PATH and HELPER_PATH are defined) before invoking
xcodebuild, ensuring the xcodebuild/test invocation (the xcodebuild command and
the only-testing cmuxUITests/DisplayResolutionRegressionUITests) picks up the
explicit manifest instead of relying on the in-test fallback.
In @.github/workflows/test-e2e.yml:
- Around line 211-222: The test manifest is written to MANIFEST_PATH but the
manifest path is never passed into xcodebuild because DISPLAY_ENV_PREFIX /
CMUX_UI_TEST_DISPLAY_HARNESS_MANIFEST_PATH is not set; update the workflow so
when invoking xcodebuild you explicitly inject the manifest path environment
variable (CMUX_UI_TEST_DISPLAY_HARNESS_MANIFEST_PATH) or set DISPLAY_ENV_PREFIX
to include that mapping, referencing MANIFEST_PATH and the xcodebuild invocation
so the test process reads the correct manifest instead of relying on a default
path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5766b1b6-0fe8-41cc-b94a-090590fd3520
📒 Files selected for processing (3)
.github/workflows/ci.yml.github/workflows/test-e2e.ymlcmuxUITests/DisplayResolutionRegressionUITests.swift
Comment on lines
+124
to
+133
| if let helperBinaryPath = loadPrebuiltHelperBinaryPath(env) { | ||
| self.helperBinaryPath = helperBinaryPath | ||
| try launchDisplayHelper() | ||
| return | ||
| } | ||
| if let externalHarness = loadExternalHarnessFromEnvironment(env) ?? loadExternalHarnessFromManifest(env) { | ||
| if let helperBinaryPath = externalHarness.helperBinaryPath, !helperBinaryPath.isEmpty { | ||
| self.helperBinaryPath = helperBinaryPath | ||
| try launchDisplayHelper() | ||
| return |
There was a problem hiding this comment.
Do not delete externally provided helper binaries during teardown.
When helperBinaryPath comes from env/manifest, it is externally owned, but teardown still removes helperBinaryPath. This can break retries or subsequent tests that expect the same prebuilt helper path.
Proposed patch (track helper ownership before cleanup)
final class DisplayResolutionRegressionUITests: XCTestCase {
private let defaultDisplayHarnessManifestPath = "/tmp/cmux-ui-test-display-harness.json"
@@
private var helperBinaryPath = ""
+ private var shouldCleanupHelperBinary = true
@@
override func setUp() {
super.setUp()
continueAfterFailure = false
@@
helperBinaryPath = "\(tempPrefix)-helper"
helperLogPath = "\(tempPrefix)-helper.log"
+ shouldCleanupHelperBinary = true
removeTestArtifacts()
}
@@
private func prepareDisplayHarnessIfNeeded() throws {
let env = ProcessInfo.processInfo.environment
if let helperBinaryPath = loadPrebuiltHelperBinaryPath(env) {
self.helperBinaryPath = helperBinaryPath
+ shouldCleanupHelperBinary = false
try launchDisplayHelper()
return
}
if let externalHarness = loadExternalHarnessFromEnvironment(env) ?? loadExternalHarnessFromManifest(env) {
if let helperBinaryPath = externalHarness.helperBinaryPath, !helperBinaryPath.isEmpty {
self.helperBinaryPath = helperBinaryPath
+ shouldCleanupHelperBinary = false
try launchDisplayHelper()
return
}
@@
private func removeTestArtifacts() {
- for path in [
+ var paths = [
diagnosticsPath,
displayReadyPath,
displayIDPath,
displayStartPath,
displayDonePath,
- helperBinaryPath,
helperLogPath,
- ] {
+ ]
+ if shouldCleanupHelperBinary {
+ paths.append(helperBinaryPath)
+ }
+ for path in paths {
guard !path.isEmpty else { continue }
try? FileManager.default.removeItem(atPath: path)
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxUITests/DisplayResolutionRegressionUITests.swift` around lines 124 - 133,
The teardown currently always removes helperBinaryPath even when it was provided
externally; update the flow that sets helperBinaryPath (in branches using
loadExternalHarnessFromEnvironment or loadExternalHarnessFromManifest and when
helperBinaryPath is non-empty) to mark ownership (e.g., set a Boolean flag like
isHelperExternallyOwned or store an ownership enum alongside helperBinaryPath),
and change the teardown/cleanup logic (the method that currently removes
helperBinaryPath after launchDisplayHelper) to skip deleting the file when the
ownership flag indicates an external/prebuilt helper; ensure symbols to update
include helperBinaryPath, loadExternalHarnessFromEnvironment,
loadExternalHarnessFromManifest, launchDisplayHelper, and the teardown/cleanup
method.
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
…ness-manifest-env Fix display harness manifest handoff in UI test
7 tasks done
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the post-merge failure from #1598 where \ moved its external harness manifest out of \ but still only looked for the old fallback path when the direct env handoff was missing.\n\nThis passes the manifest path into the UI test runner explicitly and teaches the test to read it before falling back to the legacy default.
Summary by cubic
Fixes UI test failures by launching the virtual display helper inside the UI test sandbox and using stable
/tmpprefixes for rendezvous files. The UI test reads a prebuilt helper path fromCMUX_UI_TEST_DISPLAY_HELPER_BINARY_PATHor a manifest at/tmp/cmux-ui-test-display-harness.json(override withCMUX_UI_TEST_DISPLAY_HARNESS_MANIFEST_PATH), and CI now just compiles the helper and writeshelperBinaryPathto that manifest.@MainActorwhere needed.WindowAndDragTestsbehindcompiler(>=6.2)to restore builds on older compilers.Written for commit e99bc40. Summary will update on new commits.
Summary by CodeRabbit