Skip to content

Speed up app-host test CI - #6914

Closed
austinywang wants to merge 10 commits into
mainfrom
issue-6134-
Closed

austinywang wants to merge 10 commits into
mainfrom
issue-6134-

Conversation

@austinywang

@austinywang austinywang commented Jun 26, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #6134

Summary

  • cache app-host unit-test DerivedData on a stable per-shard path
  • build the cmux app-host test bundle once per shard with build-for-testing
  • run focused app-host gates and shard tests with test-without-building

Local validation

  • ruby -e "require 'yaml'; YAML.load_file('.github/workflows/ci.yml'); puts 'ci.yml parsed'"
  • git diff --check
  • ./tests/test_ci_unit_test_spm_retry.sh
  • ./tests/test_ci_self_hosted_guard.sh

View with Codesmith Autofix with Codesmith
Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.


Summary by cubic

Speeds up app-host unit-test CI by caching per-shard DerivedData and reusing one build via test-without-building. Cuts duplicate builds and flakiness; fixes #6134.

  • Refactors

    • Cache app-host unit-test DerivedData at a stable per-shard path via actions/cache, keyed on Xcode version, Ghostty revision, Package.resolved, and project; add broader restore fallbacks.
    • Build the app-host test bundle once per shard with xcodebuild build-for-testing; run focused gates and shard tests with test-without-building.
    • Move SwiftPM package resolution and the one-shot retry to the build step; drop the unreachable retry from the unit-test step; stop clearing DerivedData during test runs.
    • Regenerate Swift file length budget.
  • Bug Fixes

    • Make the shell PATH-preservation test deterministic on CI by setting PATH inside the shell command to avoid zsh path_helper rewrites.

Written for commit 2bf31b9. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Bug Fixes
    • Improved reliability of macOS CI test runs by isolating build artifacts per shard and reusing smarter caches.
    • Added a safer build-and-retry flow when package dependencies can’t be resolved, reducing flaky failures.
    • Switched selected test runs to use prebuilt app-host tests for more consistent execution.

@vercel

vercel Bot commented Jun 26, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
cmux Ready Ready Preview, Comment Jul 5, 2026 2:34am
cmux-staging Building Building Preview, Comment Jul 5, 2026 2:34am

@coderabbitai

coderabbitai Bot commented Jun 26, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The app-host-unit-tests CI job gains a shard-specific DerivedData directory cached by Xcode version, Ghostty revision, and dependency manifests. A new "Build app-host tests" step runs xcodebuild build-for-testing with package resolution disabled and a one-shot retry on SwiftPM resolution failures. All subsequent test invocations—focused regressions and the main runner—switch to test-without-building, and the old inline retry block is removed.

Changes

App-host unit-test CI workflow

Layer / File(s) Summary
Per-shard DerivedData cache setup
.github/workflows/ci.yml
Creates a shard-specific DerivedData path under $RUNNER_TEMP and caches it keyed by .xcode-version, Ghostty revision, Package.resolved, and project.pbxproj, replacing run-id/attempt-based naming.
Build app-host tests with SwiftPM retry
.github/workflows/ci.yml
Adds a build-for-testing step with -disableAutomaticPackageResolution and CMUX_SKIP_ZIG_BUILD=1; captures full build output and retries once after clearing ~/Library/Caches/org.swift.swiftpm and recreating DerivedData when "Could not resolve package dependencies" is detected.
Switch test runs to test-without-building and remove inline retry
.github/workflows/ci.yml
Browser proxy mirror and Option/Alt regression steps, plus the main backgrounded unit-test runner, switch from test to test-without-building; the inline resolution retry block is removed and replaced with a comment pointing to the new build step.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • manaflow-ai/cmux#6464: Implements shard-specific DERIVED_DATA_PATH and the sharded xcodebuild invocation that this PR builds upon.
  • manaflow-ai/cmux#6243: Previously added SwiftPM cache-clearing and retry logic in the unit-test step that this PR replaces with the new build step.
  • manaflow-ai/cmux#6403: Adjusted the resolve verification/retry flow around .ci-source-packages, touching the same dependency self-healing path this PR restructures.

Suggested reviewers

  • lawrencecchen

🐇 One build to rule them all, one cache to find it,
No more rebuilding — the DerivedData's primed!
test-without-building hops light on its feet,
The retry's tucked early, so failures are neat.
From 25 minutes to something much sweeter — 🥕

🚥 Pre-merge checks | ✅ 25
✅ Passed checks (25 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The workflow changes address the main goals by caching DerivedData and building once before test-without-building runs.
Out of Scope Changes check ✅ Passed The changes stay focused on the CI workflow and related test stability, with no clear unrelated scope added.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Cmux Swift Actor Isolation ✅ Passed Only .github/workflows/ci.yml changed; no Swift sources or actor-isolation-sensitive declarations were introduced or worsened.
Cmux Swift Blocking Runtime ✅ Passed Branch-only diff changes only .github/workflows/ci.yml; no Swift production files or blocking/timing sync primitives were introduced.
Cmux Browser Automation Off-Main ✅ Passed PR only changes .github/workflows/ci.yml; no browser socket automation code or policy coverage is touched, so this rule isn’t implicated.
Cmux Expensive Synchronous Load ✅ Passed Workflow-only PR: no production Swift files changed, so the expensive synchronous agent-load rule is not applicable.
Cmux Cache Substitution Correctness ✅ Passed Only .github/workflows/ci.yml changed; no production Swift/TS/JS persistence/history/snapshot code was touched, so the cache-substitution rule is N/A.
Cmux No Hacky Sleeps ✅ Passed PASS: The PR only changes .github/workflows/ci.yml, which the rule explicitly exempts; the diff removes a retry block and adds no new sleeps/waits.
Cmux Algorithmic Complexity ✅ Passed Diff only touches .github/workflows/ci.yml, so no production Swift/TS/JS/shell/runtime algorithmic-complexity path changed.
Cmux Swift Concurrency ✅ Passed The diff only changes .github/workflows/ci.yml; no Swift source files were modified, so it introduces no new legacy async patterns in cmux-owned Swift code.
Cmux Swift @Concurrent ✅ Passed PR only changes .github/workflows/ci.yml; no Swift files or concurrency annotations are touched, so the Swift concurrent rule is not applicable.
Cmux Swift File And Package Boundaries ✅ Passed The PR tip only edits .github/workflows/ci.yml; no production Swift files were changed, so the Swift boundary rule is not implicated.
Cmux Swiftpm Lockfiles ✅ Passed Only .github/workflows/ci.yml changed; no cmux-owned .gitignore, Package.resolved, Package.swift, or xcodeproj package-reference files were touched.
Cmux Swift Logging ✅ Passed PR changes are workflow-only; no Swift source files appear in the diff, so the Swift logging rule doesn’t apply.
Cmux User-Facing Error Privacy ✅ Passed Only .github/workflows/ci.yml changed; its new strings are CI log/comments, not user-facing errors or sensitive output.
Cmux Full Internationalization ✅ Passed Only .github/workflows/ci.yml changed; it’s CI operational config, and the i18n policy allows non-user-facing operational docs/tests. No locale assets changed.
Cmux Swiftui State Layout ✅ Passed PR-only diff changes .github/workflows/ci.yml only; no SwiftUI sources or layout/state code are touched, so the SwiftUI state-layout rule is not applicable.
Cmux Architecture Rethink ✅ Passed The PR only changes .github/workflows/ci.yml; it doesn’t alter Swift ownership/lifecycle architecture or introduce the banned patterns in Swift code.
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PR only changes .github/workflows/ci.yml; no Swift/UI window code or cmuxAuxiliaryWindowIdentifiers changes are present, so the rule isn’t triggered.
Cmux Source Artifacts ✅ Passed Only changed path is .github/workflows/ci.yml, a CI config; no local output, caches, build artifacts, or scratch dirs were added to source control.
Cmux No Test Or Debug Seam In Production Source ✅ Passed PR-specific diff vs the target branch only changes .github/workflows/ci.yml; no production Sources/** Swift files were modified, so no debug seam was introduced.
Cmux No Ambient Global State ✅ Passed PR only changes .github/workflows/ci.yml; no production Swift code or ambient global-state additions are present.
Title check ✅ Passed The title is concise and clearly summarizes the main change: speeding up app-host test CI.
Description check ✅ Passed The description covers Summary, Testing, Review Trigger, and Checklist, with enough detail for this workflow change.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch issue-6134-

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.

@greptile-apps

greptile-apps Bot commented Jun 26, 2026 •

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR speeds up the app-host unit-test CI job by caching DerivedData at a stable per-shard path and splitting the workflow into a single build-for-testing step followed by test-without-building in all downstream test steps, eliminating redundant builds across the focused gates and shard tests.

  • DerivedData caching: A stable path (cmux-deriveddata-app-host-unit-tests-shard-<N>) replaces the run-unique path, and actions/cache restores/saves with a full key (Xcode, Ghostty SHA, Package.resolved, project.pbxproj) plus three progressively broader restore-keys fallbacks for incremental cache hits.
  • Build/test separation: A new "Build app-host tests" step runs build-for-testing once per shard (with the existing SPM retry logic moved here), and all three test steps (BrowserSystemProxyMirrorTests, GhosttyOptionAsAltModsTests, Run unit tests) switch from test to test-without-building, removing their own redundant builds and the now-unreachable SPM retry.
  • Budget update: swift-file-length-budget.tsv is updated to reflect GhosttySurfaceView.swift shrinking from 4283 to 3964 lines.

Confidence Score: 5/5

Safe to merge — the changes are CI-infrastructure only, and the build/test separation logic is correct.

The workflow restructuring is well-reasoned: stable DerivedData paths enable caching, build-for-testing guarantees a complete bundle before any test-without-building step, and the SPM retry is correctly repositioned in the build step where it can actually recover. The three restore-key fallbacks address the previous-comment concern about full-key misses. No production Swift code is touched.

No files require special attention.

Important Files Changed

Filename Overview
.github/workflows/ci.yml Restructures the app-host CI job to cache DerivedData at a stable per-shard path, build the test bundle once with build-for-testing, and run all test steps with test-without-building; SPM retry is correctly moved to the build step; logic is sound.
.github/swift-file-length-budget.tsv Updates GhosttySurfaceView.swift line-count budget from 4283 to 3964, reordering the table to keep it sorted; no issues.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Prep as Prepare DerivedData
    participant Cache as Cache DerivedData (restore)
    participant Resolve as Resolve Swift packages
    participant Build as Build app-host tests<br/>(build-for-testing)
    participant Gate1 as BrowserSystemProxyMirrorTests<br/>(test-without-building)
    participant Gate2 as GhosttyOptionAsAltModsTests<br/>(test-without-building)
    participant Tests as Run unit tests<br/>(test-without-building)
    participant CacheSave as Cache DerivedData (save)

    Prep->>Prep: rm -rf stable path, mkdir
    Cache-->>Prep: restore DerivedData (if cache hit)
    Resolve->>Resolve: xcodebuild -resolvePackageDependencies (up to 3 attempts)
    Build->>Build: xcodebuild build-for-testing
    alt SPM resolution failure
        Build->>Build: clear SwiftPM cache + DerivedData, retry once
    end
    Build-->>Gate1: pre-built .xctest bundle
    Build-->>Gate2: pre-built .xctest bundle
    Build-->>Tests: pre-built .xctest bundle
    Gate1->>Gate1: test-without-building (shard gate only)
    Gate2->>Gate2: test-without-building (shard gate only)
    Tests->>Tests: test-without-building (shard slice)
    Tests-->>CacheSave: completed DerivedData
    CacheSave-->>Cache: save (on cache miss + success)
Loading
%%{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 Prep as Prepare DerivedData
    participant Cache as Cache DerivedData (restore)
    participant Resolve as Resolve Swift packages
    participant Build as Build app-host tests<br/>(build-for-testing)
    participant Gate1 as BrowserSystemProxyMirrorTests<br/>(test-without-building)
    participant Gate2 as GhosttyOptionAsAltModsTests<br/>(test-without-building)
    participant Tests as Run unit tests<br/>(test-without-building)
    participant CacheSave as Cache DerivedData (save)

    Prep->>Prep: rm -rf stable path, mkdir
    Cache-->>Prep: restore DerivedData (if cache hit)
    Resolve->>Resolve: xcodebuild -resolvePackageDependencies (up to 3 attempts)
    Build->>Build: xcodebuild build-for-testing
    alt SPM resolution failure
        Build->>Build: clear SwiftPM cache + DerivedData, retry once
    end
    Build-->>Gate1: pre-built .xctest bundle
    Build-->>Gate2: pre-built .xctest bundle
    Build-->>Tests: pre-built .xctest bundle
    Gate1->>Gate1: test-without-building (shard gate only)
    Gate2->>Gate2: test-without-building (shard gate only)
    Tests->>Tests: test-without-building (shard slice)
    Tests-->>CacheSave: completed DerivedData
    CacheSave-->>Cache: save (on cache miss + success)
Loading

Reviews (7): Last reviewed commit: "Regenerate Swift file length budget" | Re-trigger Greptile

Comment thread .github/workflows/ci.yml Outdated
austinywang and others added 3 commits June 29, 2026 15:52
test_shell_integration_preserves_empty_path_components sourced the shell
integration with the crafted PATH supplied via the subprocess environment.
On some self-hosted macOS runners /etc/zshenv runs path_helper even under
`zsh -f`, rewriting PATH from /etc/paths* before the `-c` body runs, so the
integration script saw the runner's real PATH instead of the test value and
the assertion failed (shard 4/4 "Run CLI no-socket regressions"):

  zsh expected PATH '<shim>::<first>::<last>:'
  got        '<shim>:/Users/runner/.cargo/bin:...:<first>:<last>'

Set PATH inside the `-c` body (after all shell startup files have run)
instead of through the environment, so the integration script
deterministically sees the intended PATH on every runner. The code path
under test — empty-component preservation during shim reordering, which uses
only shell builtins — is exercised identically.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
With the build/test split, the "Run unit tests" step runs the prebuilt
bundle via test-without-building with -disableAutomaticPackageResolution,
so it can never emit "Could not resolve package dependencies": the
resolution retry there was unreachable and its comment (still mentioning
DerivedData clearing, which an earlier commit removed) was stale. Package
resolution and its one-shot retry already live in the "Build app-host
tests" step.

Addresses Greptile review feedback on #6914.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
# Conflicts:
#	tests/test_claude_wrapper_user_binary_resolution.py
@lawrencecchen lawrencecchen added the stale-revisit Closed after 30+ days without activity; preserved for possible revisit or reopening. label Sep 23, 2026
@github-project-automation github-project-automation Bot moved this from Todo to Done in cmux backlog Sep 23, 2026

This branch was successfully deployed

1 active deployment
Preview – cmux — 2bf31b90 Deployed Jul 5, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale-revisit Closed after 30+ days without activity; preserved for possible revisit or reopening.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Speed up the tests CI job (currently 21–25 min, gates every PR)

3 participants