Repository navigation
[codex] Improve setup errors for missing Xcode and GhosttyKit cache locks - #6443
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 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. |
|
Gentle follow-up here: the latest head still has the code-review and security checks green, and the only red checks I can see are the Vercel preview deployments requiring Manaflow team authorization. No rush from my side, but if this still looks reasonable, would appreciate either review/merge or Vercel authorization when convenient. Happy to update the branch or address anything else if you prefer that first. |
…errors # Conflicts: # scripts/setup.sh
|
Following up on the refreshed head |
|
Hi, just a gentle follow-up on this small setup-error fix. The branch is refreshed from upstream and the focused validation remains green. Happy to address any maintainer feedback when convenient. Thanks! |
|
Thank you for this, @archit-goyal! Most of the setup.sh Xcode check is now covered by #12352's Metal check, which also catches Command Line Tools-only machines. The |
Main now preflights the Metal toolchain before any setup mutation, and it already stops a Command Line Tools-only machine. Run one xcodebuild -version check first so a missing Xcode or an unaccepted license gets a direct message, and drop the developer-directory path glob, which could reject valid Xcode installs. Co-authored-by: archit-goyal <go4archit@gmail.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
All contributors have signed the CLA ✍️ ✅ |
|
Thank you for this, @archit-goyal! I merged main in and pushed a small follow-up: main now checks for the Metal toolchain early, so I moved the Xcode check into one
|
|
I have read the CLA Document v2.2 and I hereby sign the CLA |
|
recheck |
teamleaderleo
added
area: build-and-ci
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
Review: The two setup-script changes are narrowly scoped and correct after the current-main merge. Fixed: retried safely when a stale lock disappears during concurrent cleanup, while retaining hard failure for a genuine removal error. Left: Bash syntax and the focused lock-race harness pass; Xcode and hosted CI remain for CI. |
|
Merge receipt for |
eaec3f0 fix: resolve registry guard base ref at runtime (manaflow-ai#15990) 5fbbc0c [codex] Improve setup errors for missing Xcode and GhosttyKit cache locks (manaflow-ai#6443) e53826c fix: ignore SwiftPM warning text in package lane (manaflow-ai#15975) # Conflicts: # .github/workflows/ci-guards.yml
2 participants
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
Fails fast when full Xcode isn’t available and hardens GhosttyKit cache lock handling. Setup can now catch missing Xcode or an unaccepted license before mutating anything, and the lock helper surfaces sanitized, actionable errors instead of treating every
mkdirfailure as lock contention.scripts/setup.sh: a singlexcodebuild -versionpreflight runs before setup mutations; failures print the active developer directory plus install/select/license steps.scripts/ensure-ghosttykit.sh: captures and sanitizesmkdirerrors; retries only on real contention, otherwise fails fast; registers lock ownership before cleanup and removes stale locks withrmdironly, retrying when a concurrent waiter wins the race and exiting with guidance when removal genuinely fails.Written for commit 30e471d. 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.