Repository navigation
Fix cmux --version root walk - #1255
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds CI steps (ci.yml, nightly.yml, release.yml) to run a CLI memory-guard regression that locates/validates the built cmux binary and runs tests/test_cli_version_memory_guard.py. Refactors CLI/cmux.swift to canonicalize URLs and add parentSearchURL(for:) to stop unsafe directory traversal. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes a macOS-specific infinite loop in Changes:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["resolvedExecutableURL()"] --> B["current = executableURL\n.deletingLastPathComponent()\n.standardizedFileURL"]
B --> C{Check current dir\nfor Info.plist /\nproject marker}
C -- found --> D[Append to candidates / return info]
C -- not found --> E["parentSearchURL(for: current)"]
E --> F{"standardized.path\n== '/' or empty?"}
F -- yes --> G[Return nil → break loop]
F -- no --> H["parent = standardized\n.deletingLastPathComponent()\n.standardizedFileURL"]
H --> I{"parent.path\n== current.path?"}
I -- yes --> G
I -- no --> J[Return parent]
J --> C
style G fill:#f96,color:#000
style D fill:#6c6,color:#000
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| CLI_BIN="$(find "$HOME/Library/Developer/Xcode/DerivedData" -path "*/Build/Products/Debug/cmux" -print -quit)" | ||
| if [ -z "${CLI_BIN:-}" ] || [ ! -x "$CLI_BIN" ]; then | ||
| echo "cmux CLI binary not found in DerivedData" >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
find order is non-deterministic; stale binary risk on self-hosted runners
CLI_BIN="$(find "$HOME/Library/Developer/Xcode/DerivedData" -path "*/Build/Products/Debug/cmux" -print -quit)"find … -print -quit stops at the first file found in directory-traversal order, which is filesystem-defined. On ephemeral GitHub-hosted runners this is fine, but on self-hosted or cached macOS runners a previous run's DerivedData directory could persist and cause the test to pick up an older binary rather than the one just built. Consider scoping the search to the project-specific DerivedData folder (e.g., DerivedData/GhosttyTabs-*) or sorting by modification time (e.g., via find … -print0 | xargs -0 ls -t | head -1) to always select the most-recently-built binary.
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3152b4ac-5917-4ec1-8d82-5145d59804d5
📒 Files selected for processing (4)
.github/workflows/ci.yml.github/workflows/nightly.yml.github/workflows/release.ymlCLI/cmux.swift
| - name: Run CLI version memory guard regression | ||
| run: | | ||
| set -euo pipefail | ||
|
|
||
| CLI_BIN="$(find "$HOME/Library/Developer/Xcode/DerivedData" -path "*/Build/Products/Debug/cmux" -print -quit)" | ||
| if [ -z "${CLI_BIN:-}" ] || [ ! -x "$CLI_BIN" ]; then | ||
| echo "cmux CLI binary not found in DerivedData" >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| CMUX_CLI_BIN="$CLI_BIN" python3 tests/test_cli_version_memory_guard.py |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate cmux-unit scheme =="
SCHEME="$(fd 'cmux-unit\.xcscheme$' . | head -n 1)"
if [ -z "${SCHEME:-}" ]; then
echo "cmux-unit.xcscheme not found" >&2
exit 1
fi
echo "$SCHEME"
echo
echo "== Build/Test references inside cmux-unit scheme =="
sed -n '/<BuildAction>/,/<\/BuildAction>/p;/<TestAction/,/<\/TestAction>/p' "$SCHEME" \
| rg -n 'BlueprintName=|BuildableName='
echo
echo "== Current CI lookup =="
rg -n 'cmux-unit|Build/Products/Debug/cmux' .github/workflows/ci.yml
echo
echo "Expected results:"
echo "1) The scheme output should show a BuildableReference for the cmux product/target, or another explicit path proving the CLI is built."
echo "2) The DerivedData search should be scopeable to GhosttyTabs-* instead of every entry under DerivedData."Repository: manaflow-ai/cmux
Length of output: 1365
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== All Xcode targets in GhosttyTabs.xcodeproj =="
grep -r 'target "' GhosttyTabs.xcodeproj/project.pbxproj | sed 's/.*target "//' | sed 's/".*//' | sort -u
echo
echo "== Check what test_cli_version_memory_guard.py expects =="
if [ -f tests/test_cli_version_memory_guard.py ]; then
head -n 50 tests/test_cli_version_memory_guard.py | grep -E 'CMUX_CLI_BIN|argv|cmux' || echo "No direct reference found in first 50 lines"
fi
echo
echo "== Search for CLI or command-line target references =="
rg -i 'cli|command.?line|executable' GhosttyTabs.xcodeproj --type xml | head -n 20
echo
echo "== Check if cmux is defined as a separate product (not app) =="
rg 'BuildableReference.*cmux[^.]' GhosttyTabs.xcodeproj --type xml --context 3Repository: manaflow-ai/cmux
Length of output: 108
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Directory structure of GhosttyTabs.xcodeproj =="
find GhosttyTabs.xcodeproj -type f -name "*.pbxproj" -o -name "*.xcscheme" | head -20
echo
echo "== List all scheme files =="
ls -la GhosttyTabs.xcodeproj/xcshareddata/xcschemes/ 2>/dev/null || echo "No schemes directory found"
echo
echo "== Content of test_cli_version_memory_guard.py =="
if [ -f tests/test_cli_version_memory_guard.py ]; then
cat tests/test_cli_version_memory_guard.py
else
echo "File not found at tests/test_cli_version_memory_guard.py"
echo "Searching for it:"
find . -name "*test_cli*" -type f 2>/dev/null
fi
echo
echo "== Search for cmux references in all schemes =="
rg -l 'cmux' GhosttyTabs.xcodeproj/xcshareddata/xcschemes/ 2>/dev/null | xargs -I {} sh -c 'echo "File: {}"; rg "BlueprintName|BuildableName" "{}" | head -10'
echo
echo "== Check if cmux (standalone binary) is referenced anywhere in build output =="
rg 'cmux.*product|executable.*cmux' GhosttyTabs.xcodeproj --type xml 2>/dev/null | head -10Repository: manaflow-ai/cmux
Length of output: 10928
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Full ci.yml content around the regression test (lines 150-170) =="
sed -n '150,170p' .github/workflows/ci.yml
echo
echo "== Check if there are other build steps before this regression test =="
sed -n '90,165p' .github/workflows/ci.yml | grep -E "^\s+-\s+name:|run:" | head -20
echo
echo "== Search for any CLI build or compilation steps =="
rg 'swift build|xcodebuild.*-product|CLI|executable' .github/workflows/ci.yml | head -20
echo
echo "== Check if cmux binary is produced by building the main app =="
rg -A 5 'Build/Products/Debug/cmux' .github/workflows/ci.ymlRepository: manaflow-ai/cmux
Length of output: 2194
The CI workflow will fail because cmux-unit scheme does not produce a standalone CLI binary.
The cmux-unit scheme only builds cmux.app (app bundle) and cmuxTests.xctest (test bundle). The regression test expects a standalone cmux executable at Build/Products/Debug/cmux, which this scheme does not create. The find command at line 158 will not find any matching binary, and the script will exit with code 1 at line 162 before the test even runs.
Either add a separate build step to produce the CLI binary, modify the scheme to build it, or adjust how the binary is resolved.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_cli_version_memory_guard.py (1)
76-89:⚠️ Potential issue | 🟠 MajorThis fixture never exercises the
/termination path.Because
build_fixture()always creates a validcmux.app/Contents/Info.plist,--versioncan resolve bundle metadata before ancestor traversal needs the new root guard. That leaves the actual"/" -> "/.."regression from the PR objective untested, so it could slip back in while this test still passes. Please add a second fixture/run that omits bundle metadata or otherwise forces the lookup to climb all the way to the root boundary under the 40k-entry directory.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cli_version_memory_guard.py` around lines 76 - 89, The test currently always creates a valid Info.plist under contents_path so the --version path resolves bundle metadata early; add a second fixture/run that omits the Info.plist (or creates a .app without a Contents/Info.plist) so the code must traverse ancestors up to the root guard (the "/" -> "/..") under the 40k-entry directory. Concretely, after the existing setup that writes Info.plist, add another case that either (a) creates a sibling .app entry (using resources_path and JUNK_APP_COUNT) but does not create contents_path/Info.plist, or (b) deletes or renames the created Info.plist before invoking the CLI, then invoke the same --version invocation to ensure ancestor traversal hits the filesystem root boundary; reference resources_path, contents_path, JUNK_APP_COUNT and the test's existing invocation to locate where to add the extra run.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/test_cli_version_memory_guard.py`:
- Around line 76-89: The test currently always creates a valid Info.plist under
contents_path so the --version path resolves bundle metadata early; add a second
fixture/run that omits the Info.plist (or creates a .app without a
Contents/Info.plist) so the code must traverse ancestors up to the root guard
(the "/" -> "/..") under the 40k-entry directory. Concretely, after the existing
setup that writes Info.plist, add another case that either (a) creates a sibling
.app entry (using resources_path and JUNK_APP_COUNT) but does not create
contents_path/Info.plist, or (b) deletes or renames the created Info.plist
before invoking the CLI, then invoke the same --version invocation to ensure
ancestor traversal hits the filesystem root boundary; reference resources_path,
contents_path, JUNK_APP_COUNT and the test's existing invocation to locate where
to add the extra run.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aca85900-f2ea-494d-be53-220e402e7f84
📒 Files selected for processing (3)
.github/workflows/ci.ymlCLI/cmux.swifttests/test_cli_version_memory_guard.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci.yml
…n-hang-investigation Fix cmux --version root walk
This branch was successfully deployed
2 participants
Summary
/into/..when searching for bundle metadatatests/test_cli_version_memory_guard.pyin PR CI and in nightly/release build workflowsTesting
./scripts/download-prebuilt-ghosttykit.sh./scripts/reload.sh --tag version-hang-fix/Users/tiffanysun/Library/Developer/Xcode/DerivedData/cmux-version-hang-fix/Build/Products/Debug/cmux DEV version-hang-fix.app/Contents/Resources/bin/cmux --versionIssues
cmux --versionhangs from the packaged app on macOS SequoiaSummary by cubic
Fixes a hang in
cmux --versionby stopping bundle metadata lookup at the filesystem root and standardizing paths during traversal. Adds a regression test to CI, nightly, and release; PR CI runs against the newest built CLI.standardizedFileURLandparentSearchURLto canonicalize paths and stop at "/" (prevents walking into "/..").tests/test_cli_version_memory_guard.pyin PR CI (auto-picks newest CLI from Xcode DerivedData), nightly, and release; fail fast if the CLI binary is missing.Written for commit 85f9ad6. Summary will update on new commits.
Summary by CodeRabbit
Testing
Bug Fixes