Repository navigation
Publish separate universal nightly track - #1067
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
📝 Walkthrough🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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}
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="tests/test_nightly_universal_build.sh">
<violation number="1" location="tests/test_nightly_universal_build.sh:8">
P1: This entire test file verifies source-code text patterns (awk/grep on the YAML workflow file) rather than observable runtime behavior, which the project's test quality policy explicitly prohibits.
If the goal is to lock in universal-build and publish-guard behavior, consider a test that actually exercises the workflow outputs — e.g., a small script that invokes the build with the expected flags and verifies the resulting binary is universal via `lipo -archs`, or a unit test for the publish-decision logic.</violation>
</file>
<file name=".github/workflows/nightly.yml">
<violation number="1" location=".github/workflows/nightly.yml:42">
P2: The fallback to `'main'` when `context.ref` has an unrecognized prefix is an unsafe default since `isMainRef = true` gates real publishing. If an unexpected ref format reaches this code, it would silently publish to the production nightly release. Consider defaulting to a non-main value so the workflow fails safe (build-only, upload artifacts) rather than fail dangerous (publish).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| @@ -0,0 +1,67 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
P1: This entire test file verifies source-code text patterns (awk/grep on the YAML workflow file) rather than observable runtime behavior, which the project's test quality policy explicitly prohibits.
If the goal is to lock in universal-build and publish-guard behavior, consider a test that actually exercises the workflow outputs — e.g., a small script that invokes the build with the expected flags and verifies the resulting binary is universal via lipo -archs, or a unit test for the publish-decision logic.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_nightly_universal_build.sh, line 8:
<comment>This entire test file verifies source-code text patterns (awk/grep on the YAML workflow file) rather than observable runtime behavior, which the project's test quality policy explicitly prohibits.
If the goal is to lock in universal-build and publish-guard behavior, consider a test that actually exercises the workflow outputs — e.g., a small script that invokes the build with the expected flags and verifies the resulting binary is universal via `lipo -archs`, or a unit test for the publish-decision logic.</comment>
<file context>
@@ -0,0 +1,67 @@
+ROOT_DIR="$(cd "$(dirname "$0")/.." && pwd)"
+WORKFLOW_FILE="$ROOT_DIR/.github/workflows/nightly.yml"
+
+if ! awk '
+ /^ - name: Build app \(Release\)/ { in_build=1; next }
+ in_build && /^ - name:/ { in_build=0 }
</file context>
| const requestedRef = context.ref.startsWith('refs/heads/') | ||
| ? context.ref.replace('refs/heads/', '') | ||
| : 'main'; |
There was a problem hiding this comment.
P2: The fallback to 'main' when context.ref has an unrecognized prefix is an unsafe default since isMainRef = true gates real publishing. If an unexpected ref format reaches this code, it would silently publish to the production nightly release. Consider defaulting to a non-main value so the workflow fails safe (build-only, upload artifacts) rather than fail dangerous (publish).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/nightly.yml, line 42:
<comment>The fallback to `'main'` when `context.ref` has an unrecognized prefix is an unsafe default since `isMainRef = true` gates real publishing. If an unexpected ref format reaches this code, it would silently publish to the production nightly release. Consider defaulting to a non-main value so the workflow fails safe (build-only, upload artifacts) rather than fail dangerous (publish).</comment>
<file context>
@@ -38,46 +39,58 @@ jobs:
- let nightlySha = null;
- try {
- const ref = await github.rest.git.getRef({
+ const requestedRef = context.ref.startsWith('refs/heads/')
+ ? context.ref.replace('refs/heads/', '')
+ : 'main';
</file context>
| const requestedRef = context.ref.startsWith('refs/heads/') | |
| ? context.ref.replace('refs/heads/', '') | |
| : 'main'; | |
| const requestedRef = context.ref.startsWith('refs/heads/') | |
| ? context.ref.replace('refs/heads/', '') | |
| : context.ref; |
Greptile SummaryThis PR updates the nightly CI workflow to build universal ( Key changes:
Concern: The Confidence Score: 2/5
Last reviewed commit: 1ed4aa1 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const requestedRef = context.ref.startsWith('refs/heads/') | ||
| ? context.ref.replace('refs/heads/', '') | ||
| : 'main'; | ||
| const isMainRef = requestedRef === 'main'; |
There was a problem hiding this comment.
Fallback to 'main' allows unintended publish for tag-dispatched workflows
When context.ref does not start with refs/heads/ (e.g., a workflow dispatch via the GitHub API on a tag reference like refs/tags/some-tag), the code falls back to requestedRef = 'main'. This sets isMainRef = true, which causes:
headShato be fetched from the currentmainbranch tip (not from the tag's commit)should_publishto be set to'true'- The nightly tag to be moved and release assets to be published—all unintentionally
A safer approach would be to fail explicitly or to keep the raw ref, allowing isMainRef to be false for non-branch refs:
| const requestedRef = context.ref.startsWith('refs/heads/') | |
| ? context.ref.replace('refs/heads/', '') | |
| : 'main'; | |
| const isMainRef = requestedRef === 'main'; | |
| const requestedRef = context.ref.startsWith('refs/heads/') | |
| ? context.ref.replace('refs/heads/', '') | |
| : null; | |
| const isMainRef = requestedRef === 'main'; |
Then add a guard before processing:
| const requestedRef = context.ref.startsWith('refs/heads/') | |
| ? context.ref.replace('refs/heads/', '') | |
| : 'main'; | |
| const isMainRef = requestedRef === 'main'; | |
| if (context.ref.startsWith('refs/heads/') === false && context.eventName === 'workflow_dispatch') { | |
| throw new Error('workflow_dispatch must be triggered on a branch ref, not a tag'); | |
| } |
This prevents accidental or automated tag-based API dispatch from triggering an unintended nightly publish.
lawrencecchen
changed the title
![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: e23eb285cd
ℹ️ 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 ! awk ' | ||
| /^ - name: Build Apple Silicon app \(Release\)/ { in_arm=1; next } | ||
| /^ - name: Build universal app \(Release\)/ { in_universal=1; next } | ||
| in_arm && /^ - name:/ { in_arm=0 } | ||
| in_universal && /^ - name:/ { in_universal=0 } |
There was a problem hiding this comment.
This test only inspects .github/workflows/nightly.yml text via awk/grep patterns, so it can pass even when the workflow behavior is broken (or fail on harmless refactors), which gives brittle, non-runtime coverage for a release pipeline change. The repository policy in /workspace/cmux/AGENTS.md explicitly forbids this pattern (“Do not add tests that only verify source code text...”), so this should be rewritten to validate observable behavior (e.g., by exercising the workflow logic through an executable seam) rather than matching YAML strings.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
1 issue found across 2 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="tests/test_nightly_universal_build.sh">
<violation number="1" location="tests/test_nightly_universal_build.sh:64">
P2: The branch-upload regression check omits `appcast.xml`, so it can falsely pass when only the universal appcast is uploaded.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| in_upload && /cmux-nightly-macos\*\.dmg/ { saw_arm_artifacts=1 } | ||
| in_upload && /cmux-nightly-universal-macos\*\.dmg/ { saw_universal_artifacts=1 } | ||
| in_upload && /appcast-universal\.xml/ { saw_universal_appcast=1 } | ||
| END { exit !(saw_if && saw_upload && saw_arm_artifacts && saw_universal_artifacts && saw_universal_appcast) } |
There was a problem hiding this comment.
P2: The branch-upload regression check omits appcast.xml, so it can falsely pass when only the universal appcast is uploaded.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_nightly_universal_build.sh, line 64:
<comment>The branch-upload regression check omits `appcast.xml`, so it can falsely pass when only the universal appcast is uploaded.</comment>
<file context>
@@ -38,9 +58,12 @@ if ! awk '
+ in_upload && /cmux-nightly-macos\*\.dmg/ { saw_arm_artifacts=1 }
+ in_upload && /cmux-nightly-universal-macos\*\.dmg/ { saw_universal_artifacts=1 }
+ in_upload && /appcast-universal\.xml/ { saw_universal_appcast=1 }
+ END { exit !(saw_if && saw_upload && saw_arm_artifacts && saw_universal_artifacts && saw_universal_appcast) }
' "$WORKFLOW_FILE"; then
- echo "FAIL: non-main nightly runs must upload artifacts instead of publishing the official nightly release"
</file context>
![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
🤖 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/nightly.yml:
- Around line 207-224: The prepare_variant() step currently always stamps
production NIGHTLY values into Info.plist (CFBundleIdentifier and SUFeedURL)
which causes branch artifacts to appear as the real nightly; change
prepare_variant() to detect branch builds and either (A) remove Sparkle keys
(SUPublicEDKey, SUFeedURL) for non-main branches, or (B) append a short
branch/tag suffix to the CFBundleIdentifier and SUFeedURL (use the provided
tag/branch name) so bundle IDs and feed URLs are branch-scoped; apply the same
change to the equivalent block referenced for lines 227-234; reference
CFBundleIdentifier, SUFeedURL, SUPublicEDKey, and the prepare_variant() mv
rename when implementing the branch/tag logic.
- Around line 42-55: The code currently coerces any non-refs/heads/* ref to
'main'; instead, preserve tag refs and only special-case branch refs: compute
requestedRef by stripping the appropriate prefix (refs/heads/ or refs/tags/)
from context.ref rather than defaulting to 'main', determine isMainRef only when
the original ref started with refs/heads/ and the stripped name equals 'main',
and only call github.rest.repos.getBranch to override headSha when isMainRef is
true; update the logic around requestedRef, isMainRef, headSha, context.ref, and
github.rest.repos.getBranch accordingly so tag-dispatched runs keep their
requested ref and sha.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 89940e70-2a6f-4105-8abe-f9e9fb7724d8
📒 Files selected for processing (2)
.github/workflows/nightly.ymltests/test_nightly_universal_build.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_nightly_universal_build.sh
| const requestedRef = context.ref.startsWith('refs/heads/') | ||
| ? context.ref.replace('refs/heads/', '') | ||
| : 'main'; | ||
| const isMainRef = requestedRef === 'main'; | ||
|
|
||
| let headSha = context.sha; | ||
| if (isMainRef) { | ||
| const branch = await github.rest.repos.getBranch({ | ||
| owner, | ||
| repo, | ||
| ref: 'tags/nightly', | ||
| branch: 'main', | ||
| }); | ||
| if (ref.data.object.type === 'commit') { | ||
| nightlySha = ref.data.object.sha; | ||
| } else if (ref.data.object.type === 'tag') { | ||
| const tagObject = await github.rest.git.getTag({ | ||
| headSha = branch.data.commit.sha; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/nightly.yml | head -100Repository: manaflow-ai/cmux
Length of output: 4279
🌐 Web query:
GitHub Actions workflow_dispatch github.ref github.ref_type
💡 Result:
For a workflow run triggered by workflow_dispatch:
-
github.refis the fully-qualified git ref of the branch or tag that the run is targeting, e.g.:- branch:
refs/heads/main - tag:
refs/tags/v1.2.3
This is “the fully-formed ref of the branch or tag that triggered the workflow run.” [1]
- branch:
-
github.ref_typetells you what kind of ref it is:branchortag. [1]
How workflow_dispatch decides the ref:
- From the GitHub UI (“Run workflow” button): you can choose the branch to run on, and that choice becomes
github.ref/github.ref_type. [3] - From the REST API (
Create a workflow dispatch event): you must send aref, and GitHub documents it as “the git reference for the workflow” and says it “can be a branch or tag name.” Thatrefbecomesgithub.ref/github.ref_typefor the run. [2]
Example usage
- run: |
echo "ref = ${{ github.ref }}"
echo "ref_type = ${{ github.ref_type }}"Sources
- GitHub Docs – Contexts (
github.ref,github.ref_type). [1] - GitHub Docs – REST API: “Create a workflow dispatch event” (
refcan be branch or tag). [2] - GitHub Changelog –
workflow_dispatchUI lets you choose which branch to run on. [3]
Don't coerce non-branch dispatches to main.
workflow_dispatch runs keep the branch or tag that received the dispatch, and GitHub exposes the triggering ref type via the refs/heads/ vs. refs/tags/ prefix in github.ref. With the current fallback, any non-refs/heads/* dispatch is treated as main, so a tag-dispatched run would rebuild the tip of main and set should_publish=true instead of staying on the requested ref.
🛠️ Suggested fix
- const requestedRef = context.ref.startsWith('refs/heads/')
- ? context.ref.replace('refs/heads/', '')
- : 'main';
- const isMainRef = requestedRef === 'main';
+ const requestedRef = context.ref.replace(/^refs\/(?:heads|tags)\//, '');
+ const isBranchRef = context.ref.startsWith('refs/heads/');
+ const isMainRef = isBranchRef && requestedRef === 'main';Also applies to: 80-84
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/nightly.yml around lines 42 - 55, The code currently
coerces any non-refs/heads/* ref to 'main'; instead, preserve tag refs and only
special-case branch refs: compute requestedRef by stripping the appropriate
prefix (refs/heads/ or refs/tags/) from context.ref rather than defaulting to
'main', determine isMainRef only when the original ref started with refs/heads/
and the stripped name equals 'main', and only call github.rest.repos.getBranch
to override headSha when isMainRef is true; update the logic around
requestedRef, isMainRef, headSha, context.ref, and github.rest.repos.getBranch
accordingly so tag-dispatched runs keep their requested ref and sha.
| prepare_variant() { | ||
| local app_dir="$1" | ||
| local bundle_id="$2" | ||
| local feed_url="$3" | ||
| local app_plist="$app_dir/cmux.app/Contents/Info.plist" | ||
|
|
||
| /usr/libexec/PlistBuddy -c "Set :CFBundleName cmux NIGHTLY" "$app_plist" | ||
| /usr/libexec/PlistBuddy -c "Set :CFBundleDisplayName cmux NIGHTLY" "$app_plist" | ||
| /usr/libexec/PlistBuddy -c "Set :CFBundleIdentifier ${bundle_id}" "$app_plist" | ||
| /usr/libexec/PlistBuddy -c "Delete :SUPublicEDKey" "$app_plist" >/dev/null 2>&1 || true | ||
| /usr/libexec/PlistBuddy -c "Delete :SUFeedURL" "$app_plist" >/dev/null 2>&1 || true | ||
| /usr/libexec/PlistBuddy -c "Add :SUPublicEDKey string ${SPARKLE_PUBLIC_KEY}" "$app_plist" | ||
| /usr/libexec/PlistBuddy -c "Add :SUFeedURL string ${feed_url}" "$app_plist" | ||
| /usr/libexec/PlistBuddy -c "Set :CFBundleShortVersionString ${BASE_MARKETING}-nightly.${NIGHTLY_DATE}" "$app_plist" | ||
| /usr/libexec/PlistBuddy -c "Set :CFBundleVersion ${NIGHTLY_BUILD}" "$app_plist" | ||
| /usr/libexec/PlistBuddy -c "Delete :CMUXCommit" "$app_plist" >/dev/null 2>&1 || true | ||
| /usr/libexec/PlistBuddy -c "Add :CMUXCommit string ${SHORT_SHA}" "$app_plist" | ||
| mv "$app_dir/cmux.app" "$app_dir/cmux NIGHTLY.app" |
There was a problem hiding this comment.
Branch-only artifacts still masquerade as the real NIGHTLY app.
These calls always stamp the production nightly bundle IDs and official feed URLs before the later artifact/publish split happens. So a non-main artifact still carries the real NIGHTLY identity on disk, which makes QA installs collide with the published nightly track instead of staying isolated. Branch runs should either remove Sparkle metadata or use a branch-scoped bundle ID/feed suffix first.
Based on learnings: For parallel/isolated builds, use --tag with a short descriptive name to create isolated apps with separate bundle IDs, sockets, and derived data paths.
Also applies to: 227-234
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/nightly.yml around lines 207 - 224, The prepare_variant()
step currently always stamps production NIGHTLY values into Info.plist
(CFBundleIdentifier and SUFeedURL) which causes branch artifacts to appear as
the real nightly; change prepare_variant() to detect branch builds and either
(A) remove Sparkle keys (SUPublicEDKey, SUFeedURL) for non-main branches, or (B)
append a short branch/tag suffix to the CFBundleIdentifier and SUFeedURL (use
the provided tag/branch name) so bundle IDs and feed URLs are branch-scoped;
apply the same change to the equivalent block referenced for lines 227-234;
reference CFBundleIdentifier, SUFeedURL, SUPublicEDKey, and the
prepare_variant() mv rename when implementing the branch/tag logic.
There was a problem hiding this comment.
2 issues found across 7 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="tests/test_ci_universal_release_settings.sh">
<violation number="1" location="tests/test_ci_universal_release_settings.sh:12">
P2: This regression test relies on grep/awk checks of source text instead of verifying executable behavior, which violates the repository’s test quality policy.</violation>
</file>
<file name="scripts/setup.sh">
<violation number="1" location="scripts/setup.sh:61">
P2: The cache key/stamp does not include xcframework target, so existing same-SHA artifacts can bypass this new universal build setting and keep serving stale architecture output.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| "$ROOT_DIR/scripts/setup.sh" \ | ||
| "$ROOT_DIR/scripts/build-sign-upload.sh" | ||
| do | ||
| if ! grep -Fq -- '-Dxcframework-target=universal' "$file"; then |
There was a problem hiding this comment.
P2: This regression test relies on grep/awk checks of source text instead of verifying executable behavior, which violates the repository’s test quality policy.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_ci_universal_release_settings.sh, line 12:
<comment>This regression test relies on grep/awk checks of source text instead of verifying executable behavior, which violates the repository’s test quality policy.</comment>
<file context>
@@ -0,0 +1,29 @@
+ "$ROOT_DIR/scripts/setup.sh" \
+ "$ROOT_DIR/scripts/build-sign-upload.sh"
+do
+ if ! grep -Fq -- '-Dxcframework-target=universal' "$file"; then
+ echo "FAIL: $file must build GhosttyKit with -Dxcframework-target=universal"
+ exit 1
</file context>
| ( | ||
| cd ghostty | ||
| zig build -Demit-xcframework=true -Doptimize=ReleaseFast | ||
| zig build -Demit-xcframework=true -Dxcframework-target=universal -Doptimize=ReleaseFast |
There was a problem hiding this comment.
P2: The cache key/stamp does not include xcframework target, so existing same-SHA artifacts can bypass this new universal build setting and keep serving stale architecture output.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/setup.sh, line 61:
<comment>The cache key/stamp does not include xcframework target, so existing same-SHA artifacts can bypass this new universal build setting and keep serving stale architecture output.</comment>
<file context>
@@ -58,7 +58,7 @@ else
(
cd ghostty
- zig build -Demit-xcframework=true -Doptimize=ReleaseFast
+ zig build -Demit-xcframework=true -Dxcframework-target=universal -Doptimize=ReleaseFast
)
# Stamp the build output with the SHA it was built from
</file context>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76f53aaf90
ℹ️ 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".
| const requestedRef = context.ref.startsWith('refs/heads/') | ||
| ? context.ref.replace('refs/heads/', '') | ||
| : 'main'; |
There was a problem hiding this comment.
The decide script coerces any ref that is not refs/heads/* to 'main', which makes isMainRef true and enables publish/tag mutation steps for non-branch dispatches; in those cases the workflow can unexpectedly publish nightly artifacts even though the requested ref was not main. Use the actual context.ref (including tag refs) and only set should_publish for an exact refs/heads/main match.
Useful? React with 👍 / 👎.
| "$ROOT_DIR/scripts/setup.sh" \ | ||
| "$ROOT_DIR/scripts/build-sign-upload.sh" | ||
| do | ||
| if ! grep -Fq -- '-Dxcframework-target=universal' "$file"; then |
There was a problem hiding this comment.
This test validates behavior by grepping/awking repository files instead of exercising runtime build behavior, so it is brittle to refactors and can still pass when the actual pipeline/build output is wrong. The repository policy in /workspace/cmux/AGENTS.md explicitly forbids source-text-only tests, so this should be rewritten to assert observable behavior through an executable path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/nightly.yml (1)
187-190: Consider adding descriptive error messages to architecture assertions.The bare
[[ ... ]]assertions will fail silently without context. Adding explicit error messages would aid debugging in CI.♻️ Suggested improvement
- [[ "$ARM_APP_ARCHS" == "arm64" ]] - [[ "$ARM_CLI_ARCHS" == "arm64" ]] - [[ "$APP_ARCHS" == *arm64* && "$APP_ARCHS" == *x86_64* ]] - [[ "$CLI_ARCHS" == *arm64* && "$CLI_ARCHS" == *x86_64* ]] + [[ "$ARM_APP_ARCHS" == "arm64" ]] || { echo "ARM app expected arm64, got: $ARM_APP_ARCHS" >&2; exit 1; } + [[ "$ARM_CLI_ARCHS" == "arm64" ]] || { echo "ARM CLI expected arm64, got: $ARM_CLI_ARCHS" >&2; exit 1; } + [[ "$APP_ARCHS" == *arm64* && "$APP_ARCHS" == *x86_64* ]] || { echo "Universal app expected arm64+x86_64, got: $APP_ARCHS" >&2; exit 1; } + [[ "$CLI_ARCHS" == *arm64* && "$CLI_ARCHS" == *x86_64* ]] || { echo "Universal CLI expected arm64+x86_64, got: $CLI_ARCHS" >&2; exit 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/nightly.yml around lines 187 - 190, The architecture assertions using bare test expressions like [[ "$ARM_APP_ARCHS" == "arm64" ]] and [[ "$APP_ARCHS" == *arm64* && "$APP_ARCHS" == *x86_64* ]] should produce descriptive errors on failure; update each test to log a clear message and fail explicitly (for example replace the bare [[ ... ]] with a conditional that on false echoes a contextual error including the variable and expected value and then exits non‑zero, or call a small helper/assert function that does this) so CI shows which arch check failed and why.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/nightly.yml:
- Around line 187-190: The architecture assertions using bare test expressions
like [[ "$ARM_APP_ARCHS" == "arm64" ]] and [[ "$APP_ARCHS" == *arm64* &&
"$APP_ARCHS" == *x86_64* ]] should produce descriptive errors on failure; update
each test to log a clear message and fail explicitly (for example replace the
bare [[ ... ]] with a conditional that on false echoes a contextual error
including the variable and expected value and then exits non‑zero, or call a
small helper/assert function that does this) so CI shows which arch check failed
and why.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4438177c-f750-4b47-9859-6acf7fce86ae
📒 Files selected for processing (2)
.github/workflows/nightly.ymltests/test_nightly_universal_build.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_nightly_universal_build.sh
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4cce891ae3
ℹ️ 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".
| fi | ||
| fi | ||
| cd ghostty && zig build -Demit-xcframework=true -Demit-macos-app=false -Doptimize=ReleaseFast | ||
| cd ghostty && zig build -Demit-xcframework=true -Demit-macos-app=false -Dxcframework-target=universal -Doptimize=ReleaseFast |
There was a problem hiding this comment.
Changing the GhosttyKit build to -Dxcframework-target=universal without changing the artifact identity means xcframework-${sha} can still resolve to an older single-arch release for the same SHA, because the workflow skips rebuilding when that tag already exists and downstream downloads are keyed by SHA. In that case nightly/release universal builds continue consuming a stale non-universal xcframework and can fail when x86_64 slices are required; include the target in the release key (or force a rebuild/migration) so the artifact identity matches its contents.
Useful? React with 👍 / 👎.
| ( | ||
| cd ghostty | ||
| zig build -Demit-xcframework=true -Doptimize=ReleaseFast | ||
| zig build -Demit-xcframework=true -Dxcframework-target=universal -Doptimize=ReleaseFast |
There was a problem hiding this comment.
This switches local GhosttyKit builds to universal, but setup.sh cache reuse is still keyed only by GHOSTTY_SHA, so a previously cached/native xcframework for the same SHA will be reused and never rebuilt. Developers who already cached the old artifact can silently keep using a non-universal framework, which breaks or masks universal release behavior locally; include the build target in the cache key/stamp so old native caches are not reused.
Useful? React with 👍 / 👎.
…acos-build Publish separate universal nightly track
This branch was successfully deployed
1 participant
Summary
com.cmuxterm.app.nightlytrack withappcast.xmlcom.cmuxterm.app.nightly.universalandappcast-universal.xmlTesting
bash tests/test_nightly_universal_build.shIssues
Summary by CodeRabbit