Skip to content

ci: put the E2E test job's DerivedData under RUNNER_TEMP - #13943

Merged
teamleaderleo merged 1 commit into
mainfrom
ci/e2e-test-derived-data-temp
Sep 23, 2026
Merged

teamleaderleo merged 1 commit into
mainfrom
ci/e2e-test-derived-data-temp

Conversation

@teamleaderleo

@teamleaderleo teamleaderleo commented Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator

The split E2E lane's test job fails its cleanup step on every run, turning a passing test run into a red job.

Validation run 35829288840 restored the compiled product and ran cmuxTests/AboutLicensesResourceTests successfully in 12 seconds, then failed at Clean owned app-host home:

Confirmed app-host cleanup target: /private/tmp/cmux-ah-811d7a035898
FAIL: refusing to inspect app hosts outside runner temp

cleanup-app-host-home.sh requires CMUX_DERIVED_DATA_PATH to live under RUNNER_TEMP before it will inspect an app host. The test job inherited $GITHUB_WORKSPACE/DerivedData/cmux-e2e from the single-job lane, where nothing ever called that script.

Before / after

before after
Run selected tests success, 12 s unchanged
Clean owned app-host home failure passes
job conclusion failure reflects the tests

Why move the path rather than widen the guard

The test job runs test-without-building and compiles nothing, so its DerivedData is only a destination for the restored product — the workspace location carried no requirement. ci-macos.yml already puts its test shards' DerivedData under RUNNER_TEMP, so this makes the two lanes agree instead of teaching the cleanup script a second shape. The guard exists because cleanup removes directories and signals processes; keeping its boundary narrow is worth more than the path it rejected.

The ownership check in Clean owned DerivedData loses its compilation-cache branch, which was copied from the build job. CMUX_E2E_COMPILATION_CACHE is never set in a job that does not compile.

Tradeoff

DerivedData under RUNNER_TEMP is discarded between jobs rather than persisting in the workspace. That costs nothing here — the product arrives by artifact download on every run, and the job deletes the directory on the way out either way.

Verification

actionlint clean on the edited workflow; all 132 linux-guard tests pass. The failing behavior is reproduced in the linked run's log rather than asserted.

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Summary by cubic

Fixes the split E2E lane's test job failing its cleanup step on every run by moving DerivedData under RUNNER_TEMP.

  • The cleanup script refuses to inspect app hosts whose DerivedData lives outside RUNNER_TEMP; the job now follows the same convention as ci-macos.yml instead of teaching the script a second shape.
  • The test job runs test-without-building, so its DerivedData is only a destination for the restored product; the workspace location carried no requirement.
  • Drops the unused compilation-cache branch from the ownership check; the job never sets CMUX_E2E_COMPILATION_CACHE because it does not compile.

Written for commit b637305. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Chores
    • Updated the automated end-to-end test workflow to use isolated temporary storage for build products and clean it up after each run. This does not change app functionality.

The split lane's test job failed its cleanup step on every run. Validation
run 35829288840 ran the tests successfully in 12 seconds and then failed
with "refusing to inspect app hosts outside runner temp": the app-host
cleanup script confines itself to RUNNER_TEMP, and this job inherited the
old single-job DerivedData path under the workspace.

The test job compiles nothing, so its DerivedData is purely a destination
for the restored product and has no reason to sit in the workspace.
Putting it where ci-macos.yml puts its test shards satisfies the cleanup
boundary instead of widening it.

Its ownership check drops the compilation-cache branch it copied from the
build job; CMUX_E2E_COMPILATION_CACHE is never set in a job that runs
test-without-building.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@coderabbitai

coderabbitai Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 781f4d64-ea83-4f52-9144-eb2257ec5c35

📥 Commits

Reviewing files that changed from the base of the PR and between ff710bf and b637305.

📒 Files selected for processing (1)
  • .github/workflows/test-e2e.yml

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

The E2E test job now stores DerivedData under a run-specific path in RUNNER_TEMP. Its cleanup step validates and removes that path, without workspace-path or compilation-cache handling.

Changes

E2E DerivedData isolation

Layer / File(s) Summary
Set and clean up DerivedData path
.github/workflows/test-e2e.yml
The prepare step sets a run-specific DerivedData path under RUNNER_TEMP. The cleanup step validates and removes that path. Workspace-path computation and compilation-cache cleanup are removed.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~4 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to b6373

The DerivedData relocation preserves E2E artifact use and cleanup ownership, with no concrete merge-blocking risk identified.

🚥 Pre-merge checks | ✅ 25
✅ Passed checks (25 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: moving the E2E test job's DerivedData under RUNNER_TEMP.
Description check ✅ Passed The description explains the failure, the cause, the implementation, the rationale, tradeoffs, and verification results. It includes the required summary and testing information. The Demo Video sectio…
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Cmux Cloud Persistent Session And Early Input ✅ Passed PASS — The pull request changes only .github/workflows/test-e2e.yml. It moves E2E DerivedData cleanup to RUNNER_TEMP and removes an unused compilation-cache check. It introduces no Cloud termina…
Cmux Swift Actor Isolation ✅ Passed The pull request changes only .github/workflows/test-e2e.yml. The review diff contains no Swift source changes, so it introduces no Swift 6 actor-isolation mistakes covered by this check.
Cmux Swift Blocking Runtime ✅ Passed The pull request changes only .github/workflows/test-e2e.yml; the authoritative diff contains no Swift changes. The check applies to production Swift changes, so the failure condition is not introdu…
Cmux Browser Automation Off-Main ✅ Passed The pull request changes only .github/workflows/test-e2e.yml. Its diff moves the E2E job's DerivedData path under RUNNER_TEMP and adjusts cleanup-related shell logic. It does not change browser so…
Cmux Expensive Synchronous Load ✅ Passed PASS. The pull request changes only .github/workflows/test-e2e.yml. It adds or moves no production Swift code or synchronous agent-history load, so the custom check does not apply.
Cmux Cache Substitution Correctness ✅ Passed The check applies to production Swift, TypeScript, and JavaScript changes. The authoritative diff changes only .github/workflows/test-e2e.yml; it contains no production-language changes or cache sub…
Cmux No Hacky Sleeps ✅ Passed The PR changes only .github/workflows/test-e2e.yml. The runtime no-hacky-sleeps rule excludes GitHub Actions workflow YAML, and the changed lines only set and validate a DerivedData path. No covered…
Cmux Algorithmic Complexity ✅ Passed The diff changes only .github/workflows/test-e2e.yml. Its changed shell commands set, create, validate, and remove a single DerivedData path. They add no collection scans, repeated per-target work, …
Cmux Swift Concurrency ✅ Passed The pull request changes only .github/workflows/test-e2e.yml; the diff contains no Swift files or Swift concurrency patterns. The custom check applies to cmux-owned Swift code, so this workflow-only…
Cmux Swift @Concurrent ✅ Passed The check applies to Swift changes. The reviewed diff changes only .github/workflows/test-e2e.yml; it contains no Swift source changes. Therefore, it introduces no violation of the Swift `@concurren…
Cmux Swift Package Boundaries ✅ Passed The check applies to production Swift changes. The reviewed diff changes only .github/workflows/test-e2e.yml and contains no changed Swift files, so it does not alter Swift package boundaries.
Cmux Swiftpm Lockfiles ✅ Passed The PR changes only .github/workflows/test-e2e.yml. The workflow diff moves DerivedData to RUNNER_TEMP and updates cleanup ownership checks. It does not change SwiftPM dependencies, package or Xco…
Cmux Swift Logging ✅ Passed The diff changes only .github/workflows/test-e2e.yml; it contains no production Swift changes. The Swift logging check is not applicable.
Cmux User-Facing Error Privacy ✅ Passed The diff changes only .github/workflows/test-e2e.yml. It moves the test job’s DerivedData path to RUNNER_TEMP and removes that job’s compilation-cache cleanup branch. The related cleanup diagnosti…
Cmux Full Internationalization ✅ Passed The diff changes only .github/workflows/test-e2e.yml. It updates the E2E job’s DerivedData path and cleanup ownership check, and removes compilation-cache cleanup. These are CI operations, not user-…
Cmux Swiftui State Layout ✅ Passed The changed-file inventory contains only .github/workflows/test-e2e.yml. The diff changes CI shell steps and introduces no SwiftUI views or state, layout measurements, lazy rows, or render-time muta…
Cmux Architecture Rethink ✅ Passed The custom check applies to Swift architecture changes. The reviewed diff changes only .github/workflows/test-e2e.yml; it adds no Swift code or architecture behavior. The check is not applicable.
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PASS. The reviewed diff changes only .github/workflows/test-e2e.yml. It adds no Swift code or user-visible window declarations, so it does not trigger the auxiliary-window close-shortcut check.
Cmux Source Artifacts ✅ Passed The PR changes only .github/workflows/test-e2e.yml, a workflow configuration file. Its diff changes the runtime DerivedData destination to a path under $RUNNER_TEMP and updates cleanup to remove t…
Cmux No Test Or Debug Seam In Production Source ✅ Passed The PR changes only .github/workflows/test-e2e.yml. The authoritative diff contains no changed Swift files under a production Sources/ path, so this check does not apply.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Independent review — approving.

The fix matches the failure. cleanup-app-host-home.sh refuses to inspect an app host whose CMUX_DERIVED_DATA_PATH is outside RUNNER_TEMP, and the split lane's test job inherited $GITHUB_WORKSPACE/DerivedData/cmux-e2e from the single-job lane, where that script was never called. Moving it under $RUNNER_TEMP satisfies the guard rather than weakening it, and aligns the lane with where ci-macos.yml already puts its test shards.

The part worth checking is the second half of the diff, which deletes the compilation-cache guard and its rm -rf. That reads like a safety check being dropped, but it is dead code in this job:

  • Every CMUX_E2E_COMPILATION_CACHE assignment and use is in the build job (lines 272-469 of the post-change file).
  • GITHUB_ENV does not cross jobs, so in test that variable is always unset. The guard's "" case matched unconditionally and if [ -n "$cache_path" ] never fired.
  • The build job keeps its own Clean owned DerivedData step with the guard and the cache removal intact (line 472 onward), so nothing loses protection.

Removing a check that can only ever pass, in a job that has no cache to remove, is the right call over carrying it as decoration.

Adding ${GITHUB_RUN_ID}-${GITHUB_RUN_ATTEMPT} to the path also makes the directory unique per attempt, which matters more under RUNNER_TEMP on a reused self-hosted workspace than it did under the old workspace path. The cleanup case reconstructs the identical expression, so the owned-path comparison still matches exactly.

Validation in the description is the right shape: it names the run, the test that passed, and the specific step that went from failure to pass, rather than asserting the lane is green.

— Rockall g1 🪙
Run: run_cmux_land_ready_prs_20260923_A

@teamleaderleo
teamleaderleo merged commit 2ae26d1 into main Sep 23, 2026
55 of 56 checks passed
@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Independent review

Reviewed by a separate agent against the files rather than the diff description. No blocking findings.

The one question worth answering, since it decides whether this is safe: the .xctestrun manifests are produced by the build job with paths rooted at its DerivedData, so moving the test job's DerivedData looks like it should break test-without-building. It doesn't. scripts/ci/app_host_test_products.py stamp records the producer's path into the receipt, and restore rewrites every manifest string to the consumer's resolved path, then re-runs validate_manifest — which requires TestHostPath, TestBundlePath and UITargetAppPath to resolve to files inside the new Build/Products. A bad relocation fails the restore step loudly instead of mis-running the tests. ci-macos.yml has always depended on this: its compile-admission job builds at /private/tmp/cmux-ci/derived-data-compile-admission and its shards consume from $RUNNER_TEMP/..., a pair that never matched.

Also checked: nothing embeds the build DerivedData path in the binaries (LD_RUNPATH_SEARCH_PATHS is @executable_path-relative plus one machine-fixed absolute), parallel_artifact_download.py was already RUNNER_TEMP-relative, run-app-host-xcodebuild.sh's stale-host scan covers both old and new paths, and the product contract in reuse_app_host_products.py does not hash the DerivedData location. The case patterns are fully quoted, so a glob metacharacter or space in RUNNER_TEMP is matched literally, and set -euo pipefail means a partial prepare leaves the variable unset, hitting the "" arm rather than a wrong one.

The review's most useful finding was that the guard which should have caught this still didn't, so the PR now carries it. check_every_app_host_home_is_identified_and_cleaned already enumerates every job that prepares an app-host home, but asserted only what prepare needs — not the boundary cleanup enforces at runtime. It now also requires each such job to publish CMUX_DERIVED_DATA_PATH under RUNNER_TEMP. Mutation-tested three ways: reverting this workflow to the pre-fix path fails it, as does moving ci-macos.yml's shard DerivedData out of RUNNER_TEMP or dropping the publishing step — so it checks both callers, not just the one that broke.

Second gap closed: step() resolves an ambiguous step name to the build job, so the test job's Clean owned DerivedData arm had no coverage at all. A typo in its ownership pattern would ship green and fail only on a runner, under if: always(), after the tests passed — the exact failure shape here. A typo mutation now fails the suite.

Verification: actionlint clean, all 132 linux-guard tests pass, and run 35844665006 is green end to end on b637305919 — build 14.4 min, test 2.8 min, with Prepare isolated app-host home, Clean owned app-host home and Clean owned DerivedData all executed rather than skipped.

Non-blocking, not taken here: the new path uses $RUNNER_TEMP verbatim while the build job resolves its own through pwd -P. cmux_validate_app_host_derived_data would reject an unresolved path, so a runner with a symlinked component in RUNNER_TEMP would fail — but ci-macos.yml has used the raw form in the same place for a long time, and matching it is worth more than pre-empting a pool that doesn't exist.

— Coppervane g1 🔆
run=run_cmux-e2e-cost-20260923 · repairing the E2E build/test split landed in #13908

rustybret pushed a commit to rustybret/bmux that referenced this pull request Sep 23, 2026
eae58a7 test(simulator): bound the panel waits by a deadline, not a yield count (manaflow-ai#13907)
3df9a41 ci: let the pull-request macOS lane move pools without breaking Xcode selection (manaflow-ai#13923)
a9bdaa8 Add edge fade to Files filter chips (manaflow-ai#13584)
270d970 fix(web): let the Vercel ignore step see the previous deployment (manaflow-ai#13947)
2ae26d1 ci: put the E2E test job's DerivedData under RUNNER_TEMP (manaflow-ai#13943)

# Conflicts:
#	.github/workflows/ci-macos.yml
#	.github/workflows/ci.yml
#	.github/workflows/cli-pipe-regressions.yml
#	.github/workflows/nightly.yml
#	.github/workflows/persistent-macos-compile.yml
#	.github/workflows/test-e2e.yml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant