Serialize xcodebuild and fetch prebuilt GhosttyKit to unblock Xcode 26 builds#2981
Merged
austinywang merged 7 commits intomainfrom Apr 19, 2026
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds conditional prebuilt GhosttyKit xcframework retrieval with checksum lookup and archive validation, moves CI download logic into a dedicated script, and serializes Changes
Sequence Diagram(s)sequenceDiagram
actor Dev
participant Reload as reload.sh
participant Lock as /tmp/cmux-xcodebuild.lock
participant Ensure as ensure-ghosttykit.sh
participant Remote as GitHub Releases
participant Validator as validate-xcframework-archive.py
participant Xcode as xcodebuild
Dev->>Reload: run reload.sh
Reload->>Lock: acquire exclusive flock (block if held)
Reload->>Ensure: ensure GhosttyKit cache
alt cache missing
Ensure->>Remote: download GhosttyKit.xcframework.tar.gz
Remote-->>Ensure: tar.gz
Ensure->>Validator: validate archive
Validator-->>Ensure: valid / fail
alt valid
Ensure->>Ensure: extract & move GhosttyKit.xcframework, update stamps
else invalid/fail
Ensure->>Ensure: fallback to local zig build (may invoke xcodebuild)
end
end
Reload->>Xcode: exec xcodebuild (under lock)
Xcode-->>Reload: exit status (preserved via PIPESTATUS)
Reload->>Lock: release flock
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
scripts/reload.sh (1)
331-345: Consider a per-user lock path for robustness on shared systems.The global
/tmp/cmux-xcodebuild.lockworks correctly on single-user dev machines, but has two edge cases worth addressing:
- Cross-user EACCES on shared /tmp: If another user creates the lock first, the file is typically world-readable (0644) and the current user's
open(..., ">>", ...)fails withPermission denied. The pre-check (line 336) silently swallows this viaor exit 0, skipping the "waiting" message, then the blockingperl(line 340) dies withopen lock: Permission denied. SinceSWBBuildServiceis itself per-user, there is no reason for users to share the lock.- TOCTOU on the wait banner (lines 336–338): The non-blocking probe releases the lock before the blocking call, so the "waiting" message may print with no actual wait — cosmetic only.
A per-user lock path fixes (1) and aligns with codebase patterns (e.g.,
build_remote_daemon_release_assets.shuses${TMPDIR:-/tmp}):🔧 Suggested refinement
-XCODEBUILD_LOCK="/tmp/cmux-xcodebuild.lock" +XCODEBUILD_LOCK="${TMPDIR:-/tmp}/cmux-xcodebuild-$(id -u).lock"Functionality is otherwise correct —
exec { $ARGV[0] }@ARGV`` aftershiftreplaces perl with `xcodebuild`, the flock(2) lock survives the `exec` via the inherited file descriptor, and is auto-released when `xcodebuild` exits. `PIPESTATUS[0]` correctly reflects the `xcodebuild` exit code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/reload.sh` around lines 331 - 345, Set XCODEBUILD_LOCK to a per-user tmp path (e.g. XCODEBUILD_LOCK="${TMPDIR:-/tmp}/cmux-xcodebuild-$UID.lock") and update both perl invocations to use that variable; remove the non-blocking probe perl block (the initial open/fcntl check that prints "waiting") to avoid the TOCTOU banner and rely on the blocking perl flock block which will now open the per-user lock without cross-user EACCES issues. Ensure the remaining perl flock block still uses "$XCODEBUILD_LOCK" and preserves the exec { $ARGV[0] } `@ARGV` behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/ensure-ghosttykit.sh`:
- Around line 120-148: The script downloads and blindly extracts the GhosttyKit
tarball (tmp_tar -> tmp_extract -> LOCAL_XCFRAMEWORK) without integrity or
path-safety checks; modify ensure-ghosttykit.sh to first fetch the corresponding
checksum/signature (e.g., from "$url.sha256" or a cosign/minisign artifact) and
verify tmp_tar with shasum -a 256 -c (or verify signature) before any
extraction, and if verification fails exit with a clear error and fallback; only
after successful verification perform a safe extraction using tar
--no-same-owner and pre-validate the archive contents (reject absolute paths or
.. components by listing/tarring first) and only then move the validated
"$tmp_extract/GhosttyKit.xcframework" into LOCAL_XCFRAMEWORK; also add a brief
comment documenting the trust model if no checksum/signature is available.
- Around line 120-128: The mktemp template used for tmp_tar is invalid on macOS
(suffix after Xs) causing mktemp to fail and curl to write to empty path; update
try_fetch_prebuilt_xcframework to create a temp directory with mktemp -d using a
template that ends with Xs (e.g., ".ghosttykit-prebuilt.XXXXXX"), then set
tmp_tar to a file path inside that directory (e.g.,
"$tmp_dl_dir/GhosttyKit.xcframework.tar.gz"); ensure cleanup removes the temp
directory variable (tmp_dl_dir) instead of tmp_tar on failure; also harden curl
by adding --connect-timeout and --max-time flags to avoid hangs when downloading
from $url.
---
Nitpick comments:
In `@scripts/reload.sh`:
- Around line 331-345: Set XCODEBUILD_LOCK to a per-user tmp path (e.g.
XCODEBUILD_LOCK="${TMPDIR:-/tmp}/cmux-xcodebuild-$UID.lock") and update both
perl invocations to use that variable; remove the non-blocking probe perl block
(the initial open/fcntl check that prints "waiting") to avoid the TOCTOU banner
and rely on the blocking perl flock block which will now open the per-user lock
without cross-user EACCES issues. Ensure the remaining perl flock block still
uses "$XCODEBUILD_LOCK" and preserves the exec { $ARGV[0] } `@ARGV` behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 229e6b48-1b1c-4cf3-a9e1-c8166e94d95e
📒 Files selected for processing (2)
scripts/ensure-ghosttykit.shscripts/reload.sh
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/reload.sh">
<violation number="1" location="scripts/reload.sh:344">
P1: The lock is dropped before `xcodebuild` starts because the Perl wrapper uses `exec`, so concurrent builds can still run in parallel.</violation>
</file>
<file name="scripts/ensure-ghosttykit.sh">
<violation number="1" location="scripts/ensure-ghosttykit.sh:122">
P2: Use a BSD-compatible `mktemp` template with trailing `X`s. This pattern leaves `XXXXXX` literal on macOS, making the temp tarball path predictable and causing fetch fallback when that stale file already exists.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
Greptile SummaryThis PR fixes concurrent Confidence Score: 5/5Safe to merge — all remaining findings are P2 hardening suggestions that don't affect correctness on the happy path. The locking mechanism is technically sound (FD_CLOEXEC correctly cleared, execvp transfers the lock fd to xcodebuild). The prebuilt fetch path is well-guarded with pinned SHA256, archive validation, and temp-dir extraction. The two P2 findings (hard link target root-containment check in the validator; missing EXIT trap for tmp_dir in ensure-ghosttykit.sh) are hardening improvements and do not affect normal operation. scripts/validate-xcframework-archive.py (hard link target check) and scripts/ensure-ghosttykit.sh (tmp_dir trap) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[reload.sh] --> B[acquire per-user flock\nTMPDIR/cmux-xcodebuild-UID.lock]
B -->|lock held| C[execvp xcodebuild]
B -->|blocked| D[print waiting msg\nthen block on flock]
D --> C
C --> E[xcodebuild runs\nfd kept open - lock held]
E --> F[xcodebuild exits\nfd closed - lock released]
G[ensure-ghosttykit.sh] --> H{Cache hit?}
H -->|yes| I[Reuse cached xcframework]
H -->|no| J{Local xcframework\nkey matches?}
J -->|yes| K[Seed cache from local]
J -->|no| L[try_fetch_prebuilt_xcframework]
L --> M{ghostty clean and\nchecksum pinned?}
M -->|no| N[zig build ReleaseFast]
M -->|yes| O[curl download\n--connect-timeout 10\n--max-time 300]
O -->|fail| N
O -->|ok| P[SHA256 verify]
P -->|mismatch| N
P -->|ok| Q[validate-xcframework-archive.py\ncheck paths/symlinks/hardlinks]
Q -->|fail| N
Q -->|ok| R[tar --no-same-owner\nto temp dir]
R --> S[mv to LOCAL_XCFRAMEWORK\nwrite stamp files]
S --> K
K --> T[cp -R to CACHE_XCFRAMEWORK]
N --> T
Reviews (2): Last reviewed commit: "Revert workflow changes from GhosttyKit ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
|
Addressed the review findings in Changes:
Validation:
I also ran |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/download-prebuilt-ghosttykit.sh (1)
27-64: Consider extractingvalidate_xcframework_archiveinto a shared helper.The body of
validate_xcframework_archive()here is a verbatim duplicate of the same function inscripts/ensure-ghosttykit.sh(see lines 43-81 of that file). Any future tightening of the archive-safety policy (new member types, additional path checks, etc.) will need to be applied in both places, which is easy to miss. Consider sourcing a common shell fragment (e.g.scripts/lib/ghosttykit-validate.sh) from both scripts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/download-prebuilt-ghosttykit.sh` around lines 27 - 64, The validate_xcframework_archive function is duplicated across scripts (e.g., this file and scripts/ensure-ghosttykit.sh); extract its Python-validation body into a shared helper (e.g., create scripts/lib/ghosttykit-validate.sh that defines validate_xcframework_archive) and update both scripts to source that helper and call the shared validate_xcframework_archive, keeping the function name and behavior identical so future policy changes apply in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/download-prebuilt-ghosttykit.sh`:
- Around line 117-120: The script currently removes "$OUTPUT_DIR" before
extracting the archive, which can irreversibly delete the existing
GhosttyKit.xcframework if extraction fails; change the order so you extract to
"$EXTRACT_DIR" first (tar --no-same-owner -xzf "$ARCHIVE_PATH" -C
"$EXTRACT_DIR"), then remove "$OUTPUT_DIR" immediately before replacing it, and
finally mv "$EXTRACT_DIR/GhosttyKit.xcframework" "$OUTPUT_DIR"; reference the
existing variables/commands OUTPUT_DIR, EXTRACT_DIR, ARCHIVE_PATH, rm -rf, tar
and mv to locate and reorder these steps.
---
Nitpick comments:
In `@scripts/download-prebuilt-ghosttykit.sh`:
- Around line 27-64: The validate_xcframework_archive function is duplicated
across scripts (e.g., this file and scripts/ensure-ghosttykit.sh); extract its
Python-validation body into a shared helper (e.g., create
scripts/lib/ghosttykit-validate.sh that defines validate_xcframework_archive)
and update both scripts to source that helper and call the shared
validate_xcframework_archive, keeping the function name and behavior identical
so future policy changes apply in one place.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b22906b6-0636-49af-87f3-52c6d277f1c2
📒 Files selected for processing (5)
.github/workflows/test-depot.yml.github/workflows/test-e2e.ymlscripts/download-prebuilt-ghosttykit.shscripts/ensure-ghosttykit.shscripts/reload.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/reload.sh
- scripts/ensure-ghosttykit.sh
…-2980-xcodebuild-concurrent-deadlock
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/validate-xcframework-archive.py (1)
28-44: Minor:saw_rootis set for any accepted member, not specifically the root.Because the prior check at line 33 already rejects any entry that isn't
GhosttyKit.xcframeworkor underGhosttyKit.xcframework/, settingsaw_root = Truefor every member is effectively equivalent to "archive was non-empty." The variable name suggests stricter semantics (that theGhosttyKit.xcframeworkdirectory entry itself was seen). Functionally correct today, but consider either renaming (e.g.,saw_any_entry) or tightening the check toname == ROOT or name == ROOT + "/"so the guard matches its name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/validate-xcframework-archive.py` around lines 28 - 44, The flag saw_root currently flips for any accepted archive member, which only verifies the archive is non-empty; change its semantics to truly detect the presence of the ROOT directory entry by only setting saw_root when name == ROOT or name == ROOT + "/", i.e. inside the tar.getmembers() loop after the existing validations check for the ROOT entry explicitly (use the existing normalize(name), is_safe_member(), and ROOT symbols) so the final missing-ROOT guard accurately reflects whether the GhosttyKit.xcframework root directory was present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/validate-xcframework-archive.py`:
- Around line 28-44: The flag saw_root currently flips for any accepted archive
member, which only verifies the archive is non-empty; change its semantics to
truly detect the presence of the ROOT directory entry by only setting saw_root
when name == ROOT or name == ROOT + "/", i.e. inside the tar.getmembers() loop
after the existing validations check for the ROOT entry explicitly (use the
existing normalize(name), is_safe_member(), and ROOT symbols) so the final
missing-ROOT guard accurately reflects whether the GhosttyKit.xcframework root
directory was present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a26d18b8-a4d3-4083-aee1-c7204efde77c
📒 Files selected for processing (4)
scripts/download-prebuilt-ghosttykit.shscripts/ensure-ghosttykit.shscripts/reload.shscripts/validate-xcframework-archive.py
🚧 Files skipped from review as they are similar to previous changes (3)
- scripts/reload.sh
- scripts/ensure-ghosttykit.sh
- scripts/download-prebuilt-ghosttykit.sh
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/validate-xcframework-archive.py`:
- Around line 35-40: The link-validation currently calls
normalize(member.linkname) and is_safe_member(...) but does not ensure links
point inside the xcframework or that the ROOT entry itself isn't a link; update
the validation in the branch where member.islnk() or member.issym() is true to
(1) reject if the current member name equals ROOT or ROOT + "/" (i.e., the ROOT
entry is a link), (2) compute the resolved target path relative to the archive
entry (using normalize(member.linkname) and joining it with the member's
dirname) and then verify the resolved target is under the ROOT subtree (e.g.,
resolved_path == ROOT or startswith(ROOT + "/")), and (3) raise SystemExit with
a clear message referencing member.linkname if the resolved target is outside
ROOT; keep using is_safe_member() first to reject absolute/.. paths but add this
extra ROOT containment check.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a35dd511-d136-48ac-b6f4-b5de8d79ca43
📒 Files selected for processing (1)
scripts/validate-xcframework-archive.py
Comment on lines
+35
to
+40
| if name == ROOT or name == ROOT + "/": | ||
| saw_root = True | ||
| if member.islnk() or member.issym(): | ||
| target = normalize(member.linkname) | ||
| if not target or not is_safe_member(target): | ||
| raise SystemExit(f"unsafe archive link target: {member.linkname}") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat scripts/validate-xcframework-archive.pyRepository: manaflow-ai/cmux
Length of output: 1572
🏁 Script executed:
#!/bin/bash
# Check if there are any test files that show expected behavior
find . -type f -name "*test*validate*" -o -name "*tar*test*" 2>/dev/null | head -20
# Also check git history for context on this file
git log --oneline scripts/validate-xcframework-archive.py 2>/dev/null | head -5
# Search for how this archive is created
git grep -l "xcframework.tar.gz\|validate-xcframework" 2>/dev/nullRepository: manaflow-ai/cmux
Length of output: 341
🏁 Script executed:
# Check the download and ensure scripts to understand archive usage
cat scripts/download-prebuilt-ghosttykit.sh
echo "---"
cat scripts/ensure-ghosttykit.shRepository: manaflow-ai/cmux
Length of output: 11270
🏁 Script executed:
#!/bin/bash
# Verify the key security claims about the link validation logic
# Create a test script to check path safety logic
python3 << 'VERIFY'
from pathlib import PurePosixPath
# Current validation function
def is_safe_member(name: str) -> bool:
path = PurePosixPath(name)
return not path.is_absolute() and ".." not in path.parts
# Test cases for the review's concerns
print("=== Testing current is_safe_member() validation ===\n")
# Test 1: Hardlink target outside ROOT with no ".." or absolute path
print("Test 1: Hardlink to 'some-other-path'")
target = "some-other-path"
safe = is_safe_member(target)
print(f" is_safe_member('{target}'): {safe}")
print(f" Issue: This path doesn't start with ROOT but is_safe_member returns {safe}\n")
# Test 2: Relative path with ".."
print("Test 2: Path with '..'")
target = "../escape"
safe = is_safe_member(target)
print(f" is_safe_member('{target}'): {safe}\n")
# Test 3: Absolute path
print("Test 3: Absolute path")
target = "/etc/passwd"
safe = is_safe_member(target)
print(f" is_safe_member('{target}'): {safe}\n")
# Test 4: Relative path that looks safe but could escape via symlink resolution
print("Test 4: Relative symlink in subdirectory")
symlink_name = "GhosttyKit.xcframework/subdir/link"
symlink_target = "foo"
safe = is_safe_member(symlink_target)
print(f" Member path: {symlink_name}")
print(f" Link target: {symlink_target}")
print(f" is_safe_member('{symlink_target}'): {safe}")
symlink_parent = PurePosixPath(symlink_name).parent
resolved = symlink_parent / symlink_target
print(f" Resolved target when extracted: {resolved}")
print(f" Stays under ROOT? {str(resolved).startswith('GhosttyKit.xcframework')}\n")
# Test 5: ROOT as a symlink
print("Test 5: ROOT entry as symlink")
root_name = "GhosttyKit.xcframework"
root_target = "../real"
safe = is_safe_member(root_target)
print(f" Member path: {root_name}")
print(f" Link target: {root_target}")
print(f" Current code rejects with '..'? {'..' in PurePosixPath(root_target).parts}")
print(f" But if target was 'real'? {is_safe_member('real')}")
print(f" Issue: ROOT as symlink could redirect framework access\n")
print("=== Summary of issues ===")
print("1. Hardlinks can target paths outside ROOT (e.g., 'some-other-path')")
print("2. ROOT itself could be a symlink to a relative target without '..'")
print("3. Symlinks in subdirs could resolve outside ROOT if we only validate target safety, not resolution")
VERIFYRepository: manaflow-ai/cmux
Length of output: 1109
🏁 Script executed:
#!/bin/bash
# Verify the proposed fix logic
python3 << 'VERIFY'
from pathlib import PurePosixPath
ROOT = "GhosttyKit.xcframework"
def normalize(name: str) -> str:
while name.startswith("./"):
name = name[2:]
return name
def is_safe_member(name: str) -> bool:
path = PurePosixPath(name)
return not path.is_absolute() and ".." not in path.parts
# Proposed fix helper
def is_under_root(path: PurePosixPath) -> bool:
value = str(path)
return value == ROOT or value.startswith(ROOT + "/")
print("=== Testing proposed fix logic ===\n")
# Test case 1: ROOT as hardlink to relative target
print("Test 1: ROOT as hardlink")
name = "GhosttyKit.xcframework"
member_linkname = "something"
target = normalize(member_linkname)
if name.rstrip("/") == ROOT:
print(f" Member: {name}")
print(f" Target: {target}")
print(f" Proposed fix REJECTS: 'archive root must be a directory'\n")
# Test case 2: Symlink in subdirectory with absolute resolution
print("Test 2: Symlink in subdir - absolute resolution")
name = "GhosttyKit.xcframework/lib"
member_linkname = "external"
target = normalize(member_linkname)
target_path = PurePosixPath(target)
name_path = PurePosixPath(name)
resolved_target = name_path.parent / target_path
print(f" Member: {name}")
print(f" Target: {target}")
print(f" Name parent: {name_path.parent}")
print(f" Resolved: {resolved_target}")
print(f" is_under_root({resolved_target}): {is_under_root(resolved_target)}")
if not is_under_root(resolved_target):
print(f" Proposed fix REJECTS: 'escapes ROOT'\n")
# Test case 3: Symlink with relative path back out
print("Test 3: Symlink pointing outside")
name = "GhosttyKit.xcframework/subdir/lib"
member_linkname = "../../external"
target = normalize(member_linkname)
if ".." in PurePosixPath(target).parts:
print(f" Member: {name}")
print(f" Target: {target}")
print(f" Contains '..': Yes")
print(f" Already caught by is_safe_member check\n")
# Test case 4: Hardlink to outside member
print("Test 4: Hardlink validation")
name = "GhosttyKit.xcframework/libghost.a"
member_linkname = "other-lib"
target = normalize(member_linkname)
target_path = PurePosixPath(target)
resolved_target = target_path # For hardlinks, no relative resolution
print(f" Member: {name}")
print(f" Hardlink target: {target}")
print(f" Resolved (hardlink, no parent): {resolved_target}")
print(f" is_under_root({resolved_target}): {is_under_root(resolved_target)}")
if not is_under_root(resolved_target):
print(f" Proposed fix REJECTS: 'escapes ROOT'\n")
# Test case 5: Valid symlink within framework
print("Test 5: Valid symlink within framework")
name = "GhosttyKit.xcframework/lib/link"
member_linkname = "libghostty.a"
target = normalize(member_linkname)
target_path = PurePosixPath(target)
name_path = PurePosixPath(name)
resolved_target = name_path.parent / target_path
print(f" Member: {name}")
print(f" Target: {target}")
print(f" Resolved: {resolved_target}")
print(f" is_under_root({resolved_target}): {is_under_root(resolved_target)}")
print(f" Result: ALLOWED\n")
print("=== Fix correctness assessment ===")
print("✓ Prevents ROOT from being a symlink/hardlink")
print("✓ Ensures symlink targets resolve within ROOT (relative resolution)")
print("✓ Ensures hardlink targets are within ROOT")
VERIFYRepository: manaflow-ai/cmux
Length of output: 1288
🏁 Script executed:
#!/bin/bash
# Verify edge cases and implementation correctness of the proposed fix
python3 << 'VERIFY'
from pathlib import PurePosixPath
ROOT = "GhosttyKit.xcframework"
def is_under_root(path: PurePosixPath) -> bool:
value = str(path)
return value == ROOT or value.startswith(ROOT + "/")
print("=== Testing edge cases in proposed fix ===\n")
# Edge case 1: name.rstrip("/") logic
print("Edge case 1: ROOT with trailing slash")
names = ["GhosttyKit.xcframework", "GhosttyKit.xcframework/"]
for name in names:
result = name.rstrip("/") == ROOT
print(f" name.rstrip('/') == ROOT for '{name}': {result}")
print()
# Edge case 2: Path with PurePosixPath normalization
print("Edge case 2: PurePosixPath path normalization")
test_paths = [
("GhosttyKit.xcframework", True),
("GhosttyKit.xcframework/", True),
("GhosttyKit.xcframework/subdir", True),
("GhosttyKit.xcframework/subdir/file", True),
("other-dir", False),
("other-dir/GhosttyKit.xcframework", False),
]
for path_str, expected in test_paths:
result = is_under_root(PurePosixPath(path_str))
status = "✓" if result == expected else "✗"
print(f" {status} is_under_root('{path_str}'): {result} (expected {expected})")
print()
# Edge case 3: Symlink resolution when name ends with /
print("Edge case 3: Symlink resolution with trailing slash in name")
name = "GhosttyKit.xcframework/lib/"
target = "external"
target_path = PurePosixPath(target)
name_path = PurePosixPath(name)
resolved = name_path.parent / target_path
print(f" name: '{name}'")
print(f" name_path.parent: {name_path.parent}")
print(f" resolved: {resolved}")
print(f" Correct? {str(resolved) == 'GhosttyKit.xcframework/external'}\n")
# Edge case 4: Hardlink with relative target
print("Edge case 4: Hardlink to relative path")
target = "lib/libghostty.a"
target_path = PurePosixPath(target)
under = is_under_root(target_path)
print(f" Hardlink target: '{target}'")
print(f" is_under_root: {under}")
print(f" Correctly REJECTED: {not under}\n")
# Edge case 5: Check if all member names are already validated to start with ROOT
print("Edge case 5: Pre-validation of member names")
print(" From code: every member name is checked with:")
print(" if name != ROOT and not name.startswith(ROOT + '/'):")
print(" raise SystemExit(...)")
print(" So at link validation, name ALWAYS satisfies is_under_root(name)")
print(" Therefore, for symlinks, resolved_target will too (if it stays local)")
print()
print("=== Implementation completeness ===")
print("✓ is_under_root() correctly identifies ROOT and descendants")
print("✓ name.rstrip('/') handles trailing slashes properly")
print("✓ Symlink resolution uses parent directory (correct for relative symlinks)")
print("✓ Hardlink resolution uses target directly (correct for hardlinks)")
VERIFYRepository: manaflow-ai/cmux
Length of output: 1671
Ensure symlink and hardlink targets remain within the xcframework tree.
Line 37 validates link targets with is_safe_member(), which rejects absolute paths and .. components, but does not verify that targets stay under GhosttyKit.xcframework. This allows:
- Hardlinks to point to paths outside the framework (e.g.,
some-other-pathpassesis_safe_member()but is not under ROOT) - The ROOT entry itself to be a symlink or hardlink, potentially redirecting access
Add a check to reject ROOT as a link and verify that resolved link targets remain under GhosttyKit.xcframework before extraction.
🛡️ Proposed hardening
def is_safe_member(name: str) -> bool:
path = PurePosixPath(name)
return not path.is_absolute() and ".." not in path.parts
+def is_under_root(path: PurePosixPath) -> bool:
+ value = str(path)
+ return value == ROOT or value.startswith(ROOT + "/")
+
+
def main() -> None:
@@
if name == ROOT or name == ROOT + "/":
saw_root = True
if member.islnk() or member.issym():
target = normalize(member.linkname)
if not target or not is_safe_member(target):
raise SystemExit(f"unsafe archive link target: {member.linkname}")
+ if name.rstrip("/") == ROOT:
+ raise SystemExit(f"archive root must be a directory: {member.name}")
+
+ target_path = PurePosixPath(target)
+ if member.issym():
+ resolved_target = PurePosixPath(name).parent / target_path
+ else:
+ resolved_target = target_path
+
+ if not is_under_root(resolved_target):
+ raise SystemExit(f"archive link target escapes {ROOT}: {member.linkname}")
elif not (member.isfile() or member.isdir()):
raise SystemExit(f"unsupported archive member: {member.name}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/validate-xcframework-archive.py` around lines 35 - 40, The
link-validation currently calls normalize(member.linkname) and
is_safe_member(...) but does not ensure links point inside the xcframework or
that the ROOT entry itself isn't a link; update the validation in the branch
where member.islnk() or member.issym() is true to (1) reject if the current
member name equals ROOT or ROOT + "/" (i.e., the ROOT entry is a link), (2)
compute the resolved target path relative to the archive entry (using
normalize(member.linkname) and joining it with the member's dirname) and then
verify the resolved target is under the ROOT subtree (e.g., resolved_path ==
ROOT or startswith(ROOT + "/")), and (3) raise SystemExit with a clear message
referencing member.linkname if the resolved target is outside ROOT; keep using
is_safe_member() first to reject absolute/.. paths but add this extra ROOT
containment check.
There was a problem hiding this comment.
3 issues found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/test-e2e.yml">
<violation number="1">
P1: The inlined GhosttyKit download/extract path skips checksum and archive validation, so CI can extract an unverified release artifact.</violation>
</file>
<file name=".github/workflows/test-depot.yml">
<violation number="1">
P1: The new inlined GhosttyKit download/extract bypasses pinned checksum and archive validation, weakening CI supply-chain protections.</violation>
</file>
<file name="scripts/validate-xcframework-archive.py">
<violation number="1" location="scripts/validate-xcframework-archive.py:35">
P2: The validator incorrectly requires an explicit root directory entry, so valid archives with only `GhosttyKit.xcframework/...` members are rejected.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| raise SystemExit(f"unsafe archive entry: {member.name}") | ||
| if name != ROOT and not name.startswith(ROOT + "/"): | ||
| raise SystemExit(f"unexpected archive entry: {member.name}") | ||
| if name == ROOT or name == ROOT + "/": |
There was a problem hiding this comment.
P2: The validator incorrectly requires an explicit root directory entry, so valid archives with only GhosttyKit.xcframework/... members are rejected.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/validate-xcframework-archive.py, line 35:
<comment>The validator incorrectly requires an explicit root directory entry, so valid archives with only `GhosttyKit.xcframework/...` members are rejected.</comment>
<file context>
@@ -0,0 +1,49 @@
+ raise SystemExit(f"unsafe archive entry: {member.name}")
+ if name != ROOT and not name.startswith(ROOT + "/"):
+ raise SystemExit(f"unexpected archive entry: {member.name}")
+ if name == ROOT or name == ROOT + "/":
+ saw_root = True
+ if member.islnk() or member.issym():
</file context>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6398d65. Configure here.
| tmp_dir="$(mktemp -d "$CACHE_ROOT/.ghosttykit-prebuilt.XXXXXX")" | ||
| tmp_tar="$tmp_dir/GhosttyKit.xcframework.tar.gz" | ||
| tmp_extract="$tmp_dir/extract" | ||
| mkdir -p "$tmp_extract" |
There was a problem hiding this comment.
set -e disabled in conditional context risks bad paths
Medium Severity
Because try_fetch_prebuilt_xcframework is invoked inside an elif condition (line 216), bash disables set -e for the entire function body. If mktemp -d fails at line 156, tmp_dir silently becomes an empty string, causing tmp_tar to resolve to /GhosttyKit.xcframework.tar.gz and tmp_extract to /extract. Subsequent operations like mkdir -p and curl -o then target the root filesystem. The mktemp failure and cascading errors are all silently swallowed. An explicit guard after mktemp (e.g. checking tmp_dir is non-empty and a directory) is needed.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 6398d65. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #2980 — concurrent
reload.shinvocations deadlock on Xcode 26'sSWBBuildService.Changes in this PR:
scripts/reload.sh— serialize the top-levelxcodebuildbehind a per-user lock so parallelreload.shruns queue instead of racing the shared Xcode 26 build service. The lock wait line and wrapper errors remain visible in the filtered build output.scripts/ensure-ghosttykit.sh— before falling back tozig build(which internally spawns anotherxcodebuild), try to fetch a prebuiltGhosttyKit.xcframeworkfor a cleanghosttysubmodule checkout. Any fetch, checksum, validation, or extraction failure cleanly falls back to the existing local ReleaseFast build path.scripts/download-prebuilt-ghosttykit.shandscripts/validate-xcframework-archive.py— share the checksum verification, archive validation, and safe extraction logic used by the prebuilt path.No workflow
.ymlfiles are changed in this PR.Test plan
./scripts/reload.sh --tag a &and./scripts/reload.sh --tag b &simultaneously on Xcode 26 — second run prints the waiting message, first run completes, second run then proceeds.~/.cache/cmux/ghosttykitandghostty/macos/GhosttyKit.xcframework, then run./scripts/reload.sh --tag test— verify==> Fetching prebuilt GhosttyKit.xcframeworkpath runs and succeeds.CMUX_GHOSTTYKIT_NO_PREBUILT=1, clean caches, run again — verify falls back tozig build.ghostty/, clean caches, run — verify skips fetch (dirty key) and falls back tozig build../scripts/reload.sh --tag warmstill reuses$CACHE_XCFRAMEWORKwith no fetch and no build.🤖 Generated with Claude Code
Note
Medium Risk
Changes the build/reload pipeline and introduces remote artifact download/extraction; while guarded by pinned SHA256 and archive validation, failures could still affect developer build reliability on macOS/Xcode 26.
Overview
Unblocks Xcode 26 dev builds by serializing
xcodebuildinvocations inscripts/reload.shusing a per-user lock, so parallelreload.shruns queue instead of crashing the sharedSWBBuildService.Speeds and stabilizes GhosttyKit setup by adding an optional prebuilt
GhosttyKit.xcframeworkfetch path inscripts/ensure-ghosttykit.sh(clean submodule only, opt-out viaCMUX_GHOSTTYKIT_NO_PREBUILT=1), and hardensscripts/download-prebuilt-ghosttykit.shwith temp-dir extraction, curl timeouts,--no-same-owner, and a newscripts/validate-xcframework-archive.pyto prevent unsafe/unexpected tar contents before installing.Reviewed by Cursor Bugbot for commit 6398d65. Bugbot is set up for automated code reviews on this repo. Configure here.