Skip to content

test(cmux): add Phase 4e gate script - #5

Merged
EtanHey merged 2 commits into
mainfrom
feat/p4e-cmux-fork-orchestrator
Apr 27, 2026
Merged

EtanHey merged 2 commits into
mainfrom
feat/p4e-cmux-fork-orchestrator

Conversation

@EtanHey

@EtanHey EtanHey commented Apr 27, 2026 •

Copy link
Copy Markdown
Owner

Summary

  • Add scripts/run_tests.sh as the Phase 4e cmux fork gate.
  • Runs targeted xcodebuild for RapidSpawnKillFixtureTests, which invokes /usr/bin/leaks --atExit against tests/fixtures/rapid_spawn_kill.sh and enforces the IOSurface budget.
  • Uses explicit bitwise-OR exit aggregation and intentionally does not use set -e.

Verification

  • RED: test -x scripts/run_tests.sh failed before script creation.
  • shellcheck scripts/run_tests.sh.
  • bash -n scripts/run_tests.sh.
  • ! grep -n 'set -e' scripts/run_tests.sh.
  • CMUX_RUN_TESTS_DERIVED_DATA_PATH=/tmp/cmux-p4e-run-tests-derived scripts/run_tests.sh exited 0.
  • Extracted XCTest attachment from /tmp/cmux-p4e-run-tests-derived: rapid_spawn_kill_sample iterations 1-3 were 12.90 MB; final VM: IOSurface = 12.90 MB.

Notes

  • Local CodeRabbit CLI was quota-blocked before review: usage-based add-on required.

Note

Low Risk
Low risk: adds a new test runner script and does not change app/runtime code, but it can affect CI outcomes by initializing submodules/downloading GhosttyKit and enforcing stricter fixture-test gating.

Overview
Adds a new scripts/run_tests.sh “Phase 4e” gate that runs a single targeted xcodebuild test (RapidSpawnKillFixtureTests) with configurable env vars for iteration count, readiness timeout, and IOSurface budget.

The script validates the rapid_spawn_kill.sh fixture is executable, auto-initializes required submodules and downloads GhosttyKit.xcframework if missing, and aggregates failures via bitmask exit codes (intentionally avoiding set -e).

Reviewed by Cursor Bugbot for commit d56e4c9. Bugbot is set up for automated code reviews on this repo. Configure here.

@coderabbitai

coderabbitai Bot commented Apr 27, 2026 •

Copy link
Copy Markdown

Warning

Rate limit exceeded

@EtanHey has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 27 minutes and 32 seconds before requesting another review.

To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 8cf0d9b1-c510-44d4-8c8f-8a01974c8381

📥 Commits

Reviewing files that changed from the base of the PR and between ab68116 and d56e4c9.

📒 Files selected for processing (1)
  • scripts/run_tests.sh
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/p4e-cmux-fork-orchestrator

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 and usage tips.

@EtanHey

EtanHey commented Apr 27, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@EtanHey

EtanHey commented Apr 27, 2026

Copy link
Copy Markdown
Owner Author

@cursor @BugBot review

@coderabbitai

coderabbitai Bot commented Apr 27, 2026

Copy link
Copy Markdown
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cursor

cursor Bot commented Apr 27, 2026 •

Copy link
Copy Markdown

Perfect. Now let me create a comprehensive review summary. Based on my analysis:

🔍 Bugbot Review — Phase 4e Test Gate Script

I've completed a thorough review of the PR that adds scripts/run_tests.sh. Here's my analysis:

✅ Implementation Quality

Strong points:

  1. Proper exit code handling — Uses bitwise-OR aggregation (EXIT_STATUS |= mask) without set -e, exactly as documented. This allows the script to report multiple failure modes through distinct bits.
  2. Correct fixture integration — The script properly invokes the RapidSpawnKillFixtureTests XCTest case, which wraps /usr/bin/leaks --atExit around tests/fixtures/rapid_spawn_kill.sh.
  3. Environment flexibility — All critical parameters (iterations, timeout, IOSurface limit, destination, derived data path, Zig binary path) are overridable via environment variables with sensible defaults.
  4. Dependency bootstrap — Handles missing GhosttyKit.xcframework by initializing submodules and downloading the prebuilt framework (lines 63-69).
  5. Clean error reporting — The mark_failure function clearly logs which step failed with its bitmask.

🔍 Detailed Findings

Architecture:

  • Lines 30-35: The failure-marking mechanism correctly accumulates errors without early exit
  • Lines 37-52: The xcodebuild invocation passes through all environment variables the test expects (CMUX_RAPID_SPAWN_KILL_*)
  • Lines 59-76: Sequential validation with appropriate failure bits (1=fixture, 2=framework, 4=xcodebuild)

Test fixture integration:

  • The fixture (tests/fixtures/rapid_spawn_kill.sh) correctly implements the spawn/kill loop with IOSurface sampling
  • The XCTest (RapidSpawnKillFixtureTests.swift) properly parses the VM: IOSurface = X MB output and enforces the budget threshold
  • Default of 3 iterations (line 17) balances speed vs reliability for CI

Potential concerns:

  1. Line 66-68: The download-prebuilt-ghosttykit.sh call has failure handling, but submodule init (line 65) does not. If git submodule update fails, the script continues to the download step.

  2. Line 19: Hardcoded Zig 0.15 path assumes Homebrew on macOS (/opt/homebrew/opt/zig@0.15/bin). This won't work in Linux CI or non-Homebrew setups. The script should either:

    • Check command -v zig first
    • Make the Zig path optional (it's already wrapped in a conditional check on line 21)
    • Document that CI environments must have Zig in PATH or set CMUX_RUN_TESTS_ZIG_BIN
  3. Lines 42-50: The -quiet flag suppresses xcodebuild output. Consider capturing failure diagnostics to a file or allowing CMUX_RUN_TESTS_QUIET override for debugging.

🎯 Risk Assessment

Low Risk — This is a standalone test orchestrator that doesn't touch app runtime code. The main risks are:

  • CI environment assumptions (Zig path, submodule availability, framework download network flakiness)
  • Silent submodule init failures could cause confusing downstream errors

📋 Recommendations

Critical:

  1. Add error handling for git submodule update on line 65:
    if ! git submodule update --init --recursive ghostty vendor/bonsplit; then
      mark_failure 2 "submodule-init"
    fi

Optional improvements:
2. Make Zig path more portable by checking command -v zig before assuming Homebrew location
3. Add CMUX_RUN_TESTS_QUIET=${CMUX_RUN_TESTS_QUIET:-1} and conditionally pass -quiet to xcodebuild
4. Consider logging the derived data path for easier troubleshooting: log "Using derivedDataPath: $DERIVED_DATA_PATH"

✅ Verification Status

Per the PR description, the following checks passed:

  • ✅ Script is executable (test -x scripts/run_tests.sh)
  • ✅ Bash syntax valid (bash -n scripts/run_tests.sh)
  • ✅ No set -e present (confirmed via grep)
  • ✅ Local test run succeeded with expected IOSurface budget (12.90 MB)

The implementation correctly follows the Phase 4e requirements for a cmux fork gate script with IOSurface leak detection.


Overall verdict: Ready to merge with the optional recommendation to add error handling for submodule initialization. The core implementation is solid and meets the stated requirements.

Open in Web Open in Cursor 

@EtanHey

EtanHey commented Apr 27, 2026

Copy link
Copy Markdown
Owner Author

Fixed the actionable Bugbot finding: submodule initialization now has explicit failure handling before GhosttyKit download, preserving the bitwise-OR aggregate behavior. Re-ran shellcheck, bash -n, no-set-e grep, and diff check locally after the patch.

@EtanHey

EtanHey commented Apr 27, 2026

Copy link
Copy Markdown
Owner Author

@cursor @BugBot re-review

@cursor

cursor Bot commented Apr 27, 2026 •

Copy link
Copy Markdown

The PR description was edited by a human, so I can't update it. Let me post my review as a comment instead and commit my findings:

🔍 Bugbot Re-Review — All Findings Addressed ✅

I've completed the re-review of commit d56e4c9 which addresses the critical finding from my initial review.

✅ Fix Verification

Change Applied:
The submodule initialization now has explicit error handling before attempting the GhosttyKit download:

  if [ ! -d "$REPO_ROOT/GhosttyKit.xcframework" ]; then
    log "GhosttyKit.xcframework missing; initializing submodules and downloading prebuilt framework"
    if ! git submodule update --init --recursive ghostty vendor/bonsplit; then
      mark_failure 2 "submodule-init"
    elif ! "$REPO_ROOT/scripts/download-prebuilt-ghosttykit.sh"; then
      mark_failure 2 "ghosttykit-download"
    fi
  fi

Why This Fix is Correct:

  1. Explicit failure detection — Submodule init failure now triggers mark_failure 2 "submodule-init" immediately
  2. Short-circuit semantics — The elif ensures the download script only runs if submodule init succeeds
  3. Error aggregation preserved — Both failures use bitmask 2 (framework-setup failures), maintaining the documented contract: 1=fixture, 2=framework, 4=xcodebuild
  4. No early exit — Failures are marked but execution continues, consistent with the "no set -e" design and bitwise-OR aggregation

✅ Verification Complete

  • ✅ Bash syntax valid (bash -n)
  • ✅ No set -e present (confirmed)
  • ✅ Submodule error handling in place (line 66)
  • ✅ Proper if/elif chain prevents download when submodule init fails
  • ✅ Both failure paths correctly mark bitmask 2 with distinct labels

📋 Remaining Observations (Optional, Non-Blocking)

These are minor points that don't affect correctness:

  • Line 19: Zig path assumes macOS Homebrew (/opt/homebrew/opt/zig@0.15/bin). This is acceptable since line 21's conditional check makes it optional—CI can set CMUX_RUN_TESTS_ZIG_BIN or use system PATH.
  • Line 43: -quiet flag is hardcoded. Consider exposing CMUX_RUN_TESTS_QUIET for debugging sessions, but not critical for gate deployment.

Final Verdict: ✅ Ready to merge

The critical finding has been properly addressed. The script now correctly handles submodule initialization failures before attempting framework download, with proper error tracking throughout. Excellent work on the fix!

Open in Web Open in Cursor 

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit d56e4c9. Configure here.

Comment thread scripts/run_tests.sh
FIXTURE_PATH="$REPO_ROOT/tests/fixtures/rapid_spawn_kill.sh"
IOSURFACE_LIMIT_MB="${CMUX_RAPID_SPAWN_KILL_IOSURFACE_LIMIT_MB:-50}"
XCTEST_ITERATIONS="${CMUX_RUN_TESTS_XCTEST_ITERATIONS:-3}"
READY_TIMEOUT_MS="${CMUX_RAPID_SPAWN_KILL_READY_TIMEOUT_MS:-8000}"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Iteration and timeout overrides are silently ignored

Low Severity

XCTEST_ITERATIONS and READY_TIMEOUT_MS are presented as configurable (via CMUX_RUN_TESTS_XCTEST_ITERATIONS and CMUX_RAPID_SPAWN_KILL_READY_TIMEOUT_MS) and passed as env vars to xcodebuild, but RapidSpawnKillFixtureTests.swift's runProcess always overrides them with hardcoded values ("3" and "8000") in the child process environment. Any override set through this script is silently discarded. Only CMUX_RAPID_SPAWN_KILL_IOSURFACE_LIMIT_MB actually propagates because the XCTest reads it from its own ProcessInfo rather than hardcoding it.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit d56e4c9. Configure here.

@EtanHey
EtanHey merged commit 876732b into main Apr 27, 2026
7 of 12 checks passed
@EtanHey
EtanHey deleted the feat/p4e-cmux-fork-orchestrator branch April 27, 2026 18:33
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