Repository navigation
Fix bundled Ghostty theme picker helper packaging - #1416
lawrencecchen wants to merge 4 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughAdds building, verification, and conditional codesigning of a Ghostty CLI helper: a Zig-based helper build script, integration into Xcode build phases, and propagation of helper verification/signing into CI/nightly/release workflows and the signing/upload script. Changes
Sequence Diagram(s)sequenceDiagram
participant Xcode as Xcode Build
participant BuildHelper as build-ghostty-cli-helper.sh
participant Zig as Zig Compiler
participant Lipo as lipo
participant SignScript as build-sign-upload.sh
participant Codesign as Code Signer
Xcode->>BuildHelper: invoke with arch flags / --output
alt universal
BuildHelper->>Zig: zig build cli-helper (arm64)
Zig-->>BuildHelper: arm64 binary
BuildHelper->>Zig: zig build cli-helper (x86_64)
Zig-->>BuildHelper: x86_64 binary
BuildHelper->>Lipo: merge slices -> universal helper
Lipo-->>BuildHelper: universal binary
else single-target
BuildHelper->>Zig: zig build cli-helper (target)
Zig-->>BuildHelper: single binary
end
BuildHelper-->>Xcode: place helper in Resources/bin/ghostty
Xcode->>SignScript: start signing phase (app path)
SignScript->>SignScript: verify helper executable at HELPER_PATH
SignScript->>Codesign: codesign main CLI binary
Codesign-->>SignScript: CLI signed
alt helper exists
SignScript->>Codesign: codesign Ghostty helper
Codesign-->>SignScript: helper signed
end
SignScript-->>Xcode: signing complete
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 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 |
![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: fc94a07f96
ℹ️ 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 ! command -v zig >/dev/null 2>&1; then | ||
| echo "error: zig is required to build the Ghostty CLI helper" >&2 | ||
| exit 1 |
There was a problem hiding this comment.
This new helper builder exits immediately if zig is missing, and the app build phase now always invokes it, which introduces a hard toolchain dependency that did not exist for prebuilt-artifact flows. In particular, the release/nightly workflows currently only download prebuilt Ghostty artifacts before running xcodebuild, so environments without preinstalled Zig will now fail during packaging instead of producing a release build. Add a fallback prebuilt helper path or explicitly install Zig in those pipelines.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 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="GhosttyTabs.xcodeproj/project.pbxproj">
<violation number="1" location="GhosttyTabs.xcodeproj/project.pbxproj:333">
P2: Single-arch x86_64 builds can package the wrong Ghostty helper architecture because the non-universal branch always builds for host arch.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/build-sign-upload.sh (1)
96-102: Minor:HELPER_PATHis defined twice.
HELPER_PATHis already defined at line 77 with the same value. The redefinition here is harmless but redundant.♻️ Suggested cleanup
# --- Codesign --- echo "Codesigning..." CLI_PATH="$APP_PATH/Contents/Resources/bin/cmux" -HELPER_PATH="$APP_PATH/Contents/Resources/bin/ghostty" if [ -f "$CLI_PATH" ]; then /usr/bin/codesign --force --options runtime --timestamp --sign "$SIGN_HASH" --entitlements "$ENTITLEMENTS" "$CLI_PATH" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/build-sign-upload.sh` around lines 96 - 102, The HELPER_PATH variable is redundantly redefined in the codesign block; remove the duplicate definition (the second HELPER_PATH="$APP_PATH/Contents/Resources/bin/ghostty") and keep the existing earlier declaration, then leave the existing conditional that codesigns "$HELPER_PATH" intact so the codesign lines referencing HELPER_PATH and CLI_PATH run without changing behavior.
🤖 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/build-sign-upload.sh`:
- Around line 96-102: The HELPER_PATH variable is redundantly redefined in the
codesign block; remove the duplicate definition (the second
HELPER_PATH="$APP_PATH/Contents/Resources/bin/ghostty") and keep the existing
earlier declaration, then leave the existing conditional that codesigns
"$HELPER_PATH" intact so the codesign lines referencing HELPER_PATH and CLI_PATH
run without changing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72631dd5-8bc7-4d73-a033-420283ed6582
📒 Files selected for processing (5)
.github/workflows/nightly.yml.github/workflows/release.ymlGhosttyTabs.xcodeproj/project.pbxprojscripts/build-ghostty-cli-helper.shscripts/build-sign-upload.sh
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e48a701499
ℹ️ 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 ! command -v zig >/dev/null 2>&1; then | ||
| echo "error: zig is required to build the Ghostty CLI helper" >&2 | ||
| exit 1 |
There was a problem hiding this comment.
This helper now hard-fails when zig is missing, and the updated Xcode build phase invokes it on every cmux app build, so prebuilt-artifact flows can still break outside release/nightly. Fresh evidence: I checked .github/workflows/ci.yml (tests-depot UI build), .github/workflows/test-e2e.yml (UI test build), and .github/workflows/ci-macos-compat.yml (smoke-test build), and these jobs run xcodebuild -scheme cmux without installing Zig first. On any runner image (or local machine) without a preinstalled Zig toolchain, those builds now fail before packaging; either add a non-Zig fallback for the helper or install Zig in every workflow that builds the app.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/release.yml (1)
205-211: Use executable invariant for helper signing guard.At signing time, prefer
-x+ fail-fast rather than a soft-fcheck so helper signing cannot be silently skipped.Suggested change
HELPER_PATH="$APP_PATH/Contents/Resources/bin/ghostty" @@ - if [ -f "$HELPER_PATH" ]; then - /usr/bin/codesign --force --options runtime --timestamp --sign "$APPLE_SIGNING_IDENTITY" --entitlements "$ENTITLEMENTS" "$HELPER_PATH" - fi + [ -x "$HELPER_PATH" ] || { echo "Ghostty helper not executable at $HELPER_PATH" >&2; exit 1; } + /usr/bin/codesign --force --options runtime --timestamp --sign "$APPLE_SIGNING_IDENTITY" --entitlements "$ENTITLEMENTS" "$HELPER_PATH"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 205 - 211, The current guard for signing the helper uses a soft [-f "$HELPER_PATH"] check which can silently skip signing; change the guard to use the executable test [-x "$HELPER_PATH"] and fail fast if the helper is missing so signing cannot be skipped: replace the if-condition that references HELPER_PATH to check -x instead of -f and ensure the branch exits with a non-zero status (or enable fail-fast like set -e) when the helper is expected but not executable, keeping the existing /usr/bin/codesign invocation for "$HELPER_PATH"; similarly verify the CLI_PATH check follows the same pattern if applicable.scripts/build-ghostty-cli-helper.sh (1)
30-37: Add explicit value validation for--targetand--output.Missing option values can currently fail via
shiftwith a less actionable error; explicit guards make CLI failures clearer.Suggested change
--target) - TARGET_TRIPLE="${2:-}" + if [[ $# -lt 2 || -z "${2:-}" ]]; then + echo "Missing value for --target" >&2 + usage >&2 + exit 1 + fi + TARGET_TRIPLE="$2" shift 2 ;; --output) - OUTPUT_PATH="${2:-}" + if [[ $# -lt 2 || -z "${2:-}" ]]; then + echo "Missing value for --output" >&2 + usage >&2 + exit 1 + fi + OUTPUT_PATH="$2" shift 2 ;;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/build-ghostty-cli-helper.sh` around lines 30 - 37, The --target and --output branches set TARGET_TRIPLE and OUTPUT_PATH but don't validate that a value was supplied; after parsing each option in the case branches for "--target" and "--output" (where TARGET_TRIPLE and OUTPUT_PATH are assigned), add an explicit check that the assigned variable is non-empty and print a clear error (e.g., "Missing value for --target" / "Missing value for --output") to stderr and exit 1 if empty, to avoid silent/ambiguous failures from shift; ensure the checks reference TARGET_TRIPLE and OUTPUT_PATH so they run immediately after assignment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yml:
- Around line 148-154: The current step "Verify bundled Ghostty theme picker
helper" only checks HELPER_BINARY exists; modify it to also verify the binary
contains both target architectures (e.g., arm64 and x86_64) before passing.
After the existence check for HELPER_BINARY, run an architecture inspection
(using lipo -info or file) on HELPER_BINARY and fail the step if the output does
not indicate both required arches (arm64 and x86_64), emitting a clear error
that the helper is not universal.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 205-211: The current guard for signing the helper uses a soft [-f
"$HELPER_PATH"] check which can silently skip signing; change the guard to use
the executable test [-x "$HELPER_PATH"] and fail fast if the helper is missing
so signing cannot be skipped: replace the if-condition that references
HELPER_PATH to check -x instead of -f and ensure the branch exits with a
non-zero status (or enable fail-fast like set -e) when the helper is expected
but not executable, keeping the existing /usr/bin/codesign invocation for
"$HELPER_PATH"; similarly verify the CLI_PATH check follows the same pattern if
applicable.
In `@scripts/build-ghostty-cli-helper.sh`:
- Around line 30-37: The --target and --output branches set TARGET_TRIPLE and
OUTPUT_PATH but don't validate that a value was supplied; after parsing each
option in the case branches for "--target" and "--output" (where TARGET_TRIPLE
and OUTPUT_PATH are assigned), add an explicit check that the assigned variable
is non-empty and print a clear error (e.g., "Missing value for --target" /
"Missing value for --output") to stderr and exit 1 if empty, to avoid
silent/ambiguous failures from shift; ensure the checks reference TARGET_TRIPLE
and OUTPUT_PATH so they run immediately after assignment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e388755e-393c-4fa4-9ec9-8634937e99a0
📒 Files selected for processing (5)
.github/workflows/nightly.yml.github/workflows/release.ymlGhosttyTabs.xcodeproj/project.pbxprojscripts/build-ghostty-cli-helper.shscripts/build-sign-upload.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/nightly.yml
- scripts/build-sign-upload.sh
| - name: Verify bundled Ghostty theme picker helper | ||
| if: steps.guard_release_assets.outputs.skip_all != 'true' | ||
| run: | | ||
| set -euo pipefail | ||
| HELPER_BINARY="build/Build/Products/Release/cmux.app/Contents/Resources/bin/ghostty" | ||
| [ -x "$HELPER_BINARY" ] || { echo "Ghostty theme picker helper not found at $HELPER_BINARY" >&2; exit 1; } | ||
|
|
There was a problem hiding this comment.
Verify helper architecture set, not just existence.
This check can pass with a single-arch helper, which risks shipping a non-universal release artifact.
Suggested hardening
- name: Verify bundled Ghostty theme picker helper
if: steps.guard_release_assets.outputs.skip_all != 'true'
run: |
set -euo pipefail
HELPER_BINARY="build/Build/Products/Release/cmux.app/Contents/Resources/bin/ghostty"
[ -x "$HELPER_BINARY" ] || { echo "Ghostty theme picker helper not found at $HELPER_BINARY" >&2; exit 1; }
+ HELPER_ARCHS="$(/usr/bin/lipo -archs "$HELPER_BINARY")"
+ case " $HELPER_ARCHS " in
+ *" arm64 "*) ;;
+ *) echo "Ghostty helper missing arm64 architecture: $HELPER_ARCHS" >&2; exit 1 ;;
+ esac
+ case " $HELPER_ARCHS " in
+ *" x86_64 "*) ;;
+ *) echo "Ghostty helper missing x86_64 architecture: $HELPER_ARCHS" >&2; exit 1 ;;
+ esac📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Verify bundled Ghostty theme picker helper | |
| if: steps.guard_release_assets.outputs.skip_all != 'true' | |
| run: | | |
| set -euo pipefail | |
| HELPER_BINARY="build/Build/Products/Release/cmux.app/Contents/Resources/bin/ghostty" | |
| [ -x "$HELPER_BINARY" ] || { echo "Ghostty theme picker helper not found at $HELPER_BINARY" >&2; exit 1; } | |
| - name: Verify bundled Ghostty theme picker helper | |
| if: steps.guard_release_assets.outputs.skip_all != 'true' | |
| run: | | |
| set -euo pipefail | |
| HELPER_BINARY="build/Build/Products/Release/cmux.app/Contents/Resources/bin/ghostty" | |
| [ -x "$HELPER_BINARY" ] || { echo "Ghostty theme picker helper not found at $HELPER_BINARY" >&2; exit 1; } | |
| HELPER_ARCHS="$(/usr/bin/lipo -archs "$HELPER_BINARY")" | |
| case " $HELPER_ARCHS " in | |
| *" arm64 "*) ;; | |
| *) echo "Ghostty helper missing arm64 architecture: $HELPER_ARCHS" >&2; exit 1 ;; | |
| esac | |
| case " $HELPER_ARCHS " in | |
| *" x86_64 "*) ;; | |
| *) echo "Ghostty helper missing x86_64 architecture: $HELPER_ARCHS" >&2; exit 1 ;; | |
| esac |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release.yml around lines 148 - 154, The current step
"Verify bundled Ghostty theme picker helper" only checks HELPER_BINARY exists;
modify it to also verify the binary contains both target architectures (e.g.,
arm64 and x86_64) before passing. After the existence check for HELPER_BINARY,
run an architecture inspection (using lipo -info or file) on HELPER_BINARY and
fail the step if the output does not indicate both required arches (arm64 and
x86_64), emitting a clear error that the helper is not universal.
…e-picker-helper # Conflicts: # .github/workflows/ci-macos-compat.yml # .github/workflows/ci.yml # .github/workflows/nightly.yml # .github/workflows/release.yml # .github/workflows/test-depot.yml # .github/workflows/test-e2e.yml # scripts/build-ghostty-cli-helper.sh
|
cmux-reconcile: close-candidate Proposed action: Close this empty PR without merging; preserve the branch. Evidence checked September 18, 2026: GitHub reports 0 changed files, 0 additions, and 0 deletions. I independently fetched the PR diff and it is empty. Head: There is no remaining patch in this PR against its target branch. This does not establish that the original feature shipped to Recheck the head/diff before acting in case new work arrives. Search |
|
All contributors have signed the CLA ✍️ ✅ |
|
Closing as already on main: merging this branch into main at 2b6ca4c produces main's own tree, so there's nothing left to land. The branch is kept; reopen if something here is still missing. Part of the backlog cleanup in manaflow-ai/cmuxterm-hq#563. |
This branch was successfully deployed
2 participants
Summary
Verification
Summary by cubic
Bundles the Ghostty theme picker helper into app builds (including universal) and adds CI guards so we don’t ship without it. Syncs workflows with the latest changes and validates, signs, and packages the helper alongside the
cmuxCLI.scripts/build-ghostty-cli-helper.shto build the helper (single-target or universal) and output tocmux.app/Contents/Resources/bin/ghostty.ARCHS), place it underResources/bin, and fail the build if missing.zigwhen missing and enforce helper bundling.lipo, and codesign it alongside thecmuxCLI.Written for commit cab0e29. Summary will update on new commits.
Summary by CodeRabbit