[codex] Improve setup errors for missing Xcode and GhosttyKit cache locks#6443
[codex] Improve setup errors for missing Xcode and GhosttyKit cache locks#6443archit-goyal wants to merge 7 commits into
Conversation
|
@archit-goyal is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
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 setup scripts receive diagnostic improvements: ChangesSetup Script Diagnostics
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (19 passed)
✨ Finishing Touches🧪 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| while ! mkdir "$LOCK_DIR" 2>"$LOCK_MKDIR_ERR"; do | ||
| if [[ ! -d "$LOCK_DIR" ]]; then | ||
| echo "error: could not create GhosttyKit cache lock at $LOCK_DIR." >&2 | ||
| if [[ -s "$LOCK_MKDIR_ERR" ]]; then | ||
| sed 's/^/ /' "$LOCK_MKDIR_ERR" >&2 | ||
| fi | ||
| echo "Check that CMUX_GHOSTTYKIT_CACHE_DIR, or its parent cache directory, is writable." >&2 | ||
| rm -f "$LOCK_MKDIR_ERR" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
mkdir failing because another process currently holds the lock and mkdir failing because the parent directory is not writable both set a non-zero exit code. Between those two events, a concurrent holder could release the lock (remove $LOCK_DIR). When that happens, the [[ ! -d "$LOCK_DIR" ]] guard fires, the script exits 1 with "could not create GhosttyKit cache lock", and a perfectly valid concurrent setup run is killed with a misleading permission-error message. This is a regression from the previous behavior, which simply waited. The window is tiny but nonzero; on shared CI caches with parallel runners it is more likely.
| @@ -124,6 +134,7 @@ while ! mkdir "$LOCK_DIR" 2>/dev/null; do | |||
| echo "==> Waiting for GhosttyKit cache lock for $GHOSTTY_KEY..." | |||
| sleep 1 | |||
| done | |||
| rm -f "$LOCK_MKDIR_ERR" | |||
| trap 'rmdir "$LOCK_DIR" >/dev/null 2>&1 || true' EXIT | |||
There was a problem hiding this comment.
$LOCK_MKDIR_ERR not included in the EXIT trap
$LOCK_MKDIR_ERR is created on line 118, but the trap … EXIT is not set until line 138 (after the lock is acquired). If the script receives SIGINT or SIGTERM during the lock-wait loop, neither the lock dir nor the temp file has a trap handler at that point, so $LOCK_MKDIR_ERR is orphaned in $TMPDIR. The two explicit rm -f calls handle the normal success path and the permission-error exit, but not signal-driven exits. Moving the trap earlier (before the loop) and including rm -f "$LOCK_MKDIR_ERR" in it would close this gap.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a8a09d665
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ! xcodebuild -version >/dev/null 2>&1; then | ||
| echo "Error: full Xcode is required, but xcodebuild is not available from an Xcode developer directory." | ||
| echo "Install Xcode 15+ from the App Store, then run:" | ||
| echo " sudo xcode-select -s /Applications/Xcode.app/Contents/Developer" |
There was a problem hiding this comment.
On machines with a full but unsupported Xcode selected, this check still succeeds because it only verifies that xcodebuild -version exits successfully, and the added guidance explicitly says Xcode 15+ is enough. The repo's source of truth is .xcode-version:1 (26.0), and scripts/select-nightly-xcodes.sh:69 already treats absence of a macOS 26+ SDK as an error, so setup can now report success for contributors who will hit a later build failure. Please compare the selected Xcode/SDK against the pinned version instead of accepting any full Xcode.
Useful? React with 👍 / 👎.
| while ! mkdir "$LOCK_DIR" 2>/dev/null; do | ||
| LOCK_MKDIR_ERR="$(mktemp "${TMPDIR:-/tmp}/cmux-ghosttykit-lock.XXXXXX")" | ||
| while ! mkdir "$LOCK_DIR" 2>"$LOCK_MKDIR_ERR"; do | ||
| if [[ ! -d "$LOCK_DIR" ]]; then |
There was a problem hiding this comment.
When another ensure-ghosttykit.sh process releases the cache lock after this process's mkdir fails with EEXIST but before the -d check runs, this branch treats normal lock contention as a cache-writability error and exits. The lock loop is meant to serialize concurrent setup/reload invocations, so this TOCTOU can make one of two parallel builds fail spuriously; retry the mkdir path before surfacing a non-lock error.
Useful? React with 👍 / 👎.
![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
🤖 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/setup.sh`:
- Around line 20-24: The xcodebuild -version check in the if statement does not
differentiate between Xcode not being installed and other failure modes such as
license or setup issues. Modify the check to first verify that xcodebuild exists
or is accessible, then separately check if it executes successfully. Provide
distinct error messages for each failure scenario: one for missing or
inaccessible xcodebuild (directing users to install Xcode), and another for
execution failures (directing users to address license/setup issues). This
allows users to receive appropriate guidance based on their actual problem.
🪄 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: 365178cf-3ee0-4037-a2f9-5cb9c01c6aeb
📒 Files selected for processing (2)
scripts/ensure-ghosttykit.shscripts/setup.sh
| echo "error: could not create the GhosttyKit cache lock." >&2 | ||
| echo "The underlying mkdir command failed while creating the lock directory." >&2 | ||
| echo "Check that the GhosttyKit cache directory is writable." >&2 | ||
| exit 1 |
There was a problem hiding this comment.
mkdir error output captured but never printed
$LOCK_MKDIR_ERR is redirected into a temp file on every mkdir call (lines 127 and 131), but when the permission-error path fires the file's contents are never written to stderr before exit 1. The EXIT trap then deletes it. The user sees only a static "Check that the GhosttyKit cache directory is writable" hint and never the actual mkdir diagnostic (e.g. mkdir: /path/.lock: Permission denied, or a "read-only filesystem" error). The whole point of introducing $LOCK_MKDIR_ERR was to surface this detail, so its absence here is a functional gap.
| echo "error: could not create the GhosttyKit cache lock." >&2 | |
| echo "The underlying mkdir command failed while creating the lock directory." >&2 | |
| echo "Check that the GhosttyKit cache directory is writable." >&2 | |
| exit 1 | |
| echo "error: could not create the GhosttyKit cache lock." >&2 | |
| echo "The underlying mkdir command failed while creating the lock directory." >&2 | |
| if [[ -s "$LOCK_MKDIR_ERR" ]]; then | |
| cat "$LOCK_MKDIR_ERR" >&2 | |
| fi | |
| echo "Check that $CMUX_GHOSTTYKIT_CACHE_DIR (or its parent directory) is writable." >&2 | |
| exit 1 |
|
All code-review/security checks are green on the latest head (Greptile 5/5, CodeRabbit success, Socket success). The only remaining failures are the Vercel preview deployment checks, which require Manaflow team authorization for an external contributor deployment. |
1 participant
Summary
scripts/setup.shwhen full Xcode is not selectedWhy
During a fresh-clone setup pass,
./scripts/setup.sheventually depends on full Xcode tooling, but a Command Line Tools-only machine did not get a clear prerequisite failure until later. The GhosttyKit helper also treated any lockmkdirfailure as lock contention, which can make permission errors look like an indefinite wait on a cache lock that does not exist.Verification
bash -n scripts/setup.shbash -n scripts/ensure-ghosttykit.shgit diff --check./scripts/setup.shon a CLT-only machine now prints a clear Xcode prerequisite errormkdirpermission error immediatelyNeed help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fail fast when full Xcode isn’t selected and harden GhosttyKit cache lock handling. Setup blocks CLT-only configs, validates the active developer dir, and surfaces sanitized, actionable lock errors instead of waiting on a bad lock.
scripts/setup.sh: Xcode 15+ preflight — check forxcodebuild, ensurexcode-selectpoints to an Xcode app, verifyxcodebuild -versionruns; print install/select/license steps; standardized errors.scripts/ensure-ghosttykit.sh: Sanitizemkdirerrors and user paths; detect contention vs permission/path failures and fail fast with a clear message; register lock ownership and only clean up after acquire; remove stale locks viarmdirand exit with guidance if removal fails (norm -rf).Written for commit 7c491c9. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
mkdirerror output, adding an extra retry when appropriate, and ensuring lock cleanup only runs after a lock is successfully acquired.Chores
xcodebuild, verifies the active developer directory, and confirmsxcodebuild -versionworks, with clearer remediation and license-acceptance guidance on failure.