Repository navigation
Make stable release build universal - #1133
Closed
lawrencecchen wants to merge 5 commits into
Closed
lawrencecchen wants to merge 5 commits into
lawrencecchen wants to merge 5 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a release-mode decision and separate verification flow to CI; builds macOS universal (arm64 + x86_64) artifacts, verifies architectures for GhosttyKit/app/CLI, delegates DMG creation to a new script, emits release-verification metadata, and conditionally uploads publish vs verification artifacts. Changes
Sequence DiagramsequenceDiagram
participant Trigger
participant GHA as GitHub Actions
participant Builder as build-sign-upload.sh
participant Verifier as arch/verifier (lipo)
participant DMG as create_release_dmg.sh
participant Storage as Artifact Storage
Trigger->>GHA: push (tag or commit)
GHA->>GHA: Determine release mode (publish_release, release_tag, artifact_name)
GHA->>Builder: Run macOS build (ARCHS=arm64 x86_64)
Builder->>Verifier: Probe binaries for architectures
Verifier-->>GHA: Return architectures + write release-verification.txt
Builder->>DMG: Create DMG (modern/legacy, may call npx or brew)
DMG-->>Builder: DMG path (or error)
alt publish_release == true
GHA->>Storage: Upload release assets (dmg, appcast, zips)
else
GHA->>Storage: Upload verification artifacts (cmux-macos.dmg, appcast.xml, release-verification.txt)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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}
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/build-sign-upload.sh">
<violation number="1" location="scripts/build-sign-upload.sh:82">
P2: The CLI binary path is used unconditionally here, but the codesign step later guards with `if [ -f "$CLI_PATH" ]`, implying it may be absent. Either guard this block similarly, or remove the guard from the codesign step if the CLI is now always expected in release builds.</violation>
<violation number="2" location="scripts/build-sign-upload.sh:87">
P2: These bare `[[ ]]` assertions exit silently under `set -e` with no error message. Add an `|| { echo ...; exit 1; }` clause so operators can immediately see which binary is missing an architecture.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| echo "Build succeeded" | ||
|
|
||
| APP_BINARY="$APP_PATH/Contents/MacOS/cmux" | ||
| CLI_BINARY="$APP_PATH/Contents/Resources/bin/cmux" |
There was a problem hiding this comment.
P2: The CLI binary path is used unconditionally here, but the codesign step later guards with if [ -f "$CLI_PATH" ], implying it may be absent. Either guard this block similarly, or remove the guard from the codesign step if the CLI is now always expected in release builds.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/build-sign-upload.sh, line 82:
<comment>The CLI binary path is used unconditionally here, but the codesign step later guards with `if [ -f "$CLI_PATH" ]`, implying it may be absent. Either guard this block similarly, or remove the guard from the codesign step if the CLI is now always expected in release builds.</comment>
<file context>
@@ -71,9 +71,22 @@ fi
echo "Build succeeded"
+APP_BINARY="$APP_PATH/Contents/MacOS/cmux"
+CLI_BINARY="$APP_PATH/Contents/Resources/bin/cmux"
+APP_ARCHS="$(lipo -archs "$APP_BINARY")"
+CLI_ARCHS="$(lipo -archs "$CLI_BINARY")"
</file context>
Comment on lines
+87
to
+88
| [[ "$APP_ARCHS" == *arm64* && "$APP_ARCHS" == *x86_64* ]] | ||
| [[ "$CLI_ARCHS" == *arm64* && "$CLI_ARCHS" == *x86_64* ]] |
There was a problem hiding this comment.
P2: These bare [[ ]] assertions exit silently under set -e with no error message. Add an || { echo ...; exit 1; } clause so operators can immediately see which binary is missing an architecture.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/build-sign-upload.sh, line 87:
<comment>These bare `[[ ]]` assertions exit silently under `set -e` with no error message. Add an `|| { echo ...; exit 1; }` clause so operators can immediately see which binary is missing an architecture.</comment>
<file context>
@@ -71,9 +71,22 @@ fi
+CLI_ARCHS="$(lipo -archs "$CLI_BINARY")"
+echo "App binary architectures: $APP_ARCHS"
+echo "CLI binary architectures: $CLI_ARCHS"
+[[ "$APP_ARCHS" == *arm64* && "$APP_ARCHS" == *x86_64* ]]
+[[ "$CLI_ARCHS" == *arm64* && "$CLI_ARCHS" == *x86_64* ]]
+
</file context>
Suggested change
| [[ "$APP_ARCHS" == *arm64* && "$APP_ARCHS" == *x86_64* ]] | |
| [[ "$CLI_ARCHS" == *arm64* && "$CLI_ARCHS" == *x86_64* ]] | |
| [[ "$APP_ARCHS" == *arm64* && "$APP_ARCHS" == *x86_64* ]] || { echo "ERROR: App binary is not universal (got: $APP_ARCHS)" >&2; exit 1; } | |
| [[ "$CLI_ARCHS" == *arm64* && "$CLI_ARCHS" == *x86_64* ]] || { echo "ERROR: CLI binary is not universal (got: $CLI_ARCHS)" >&2; exit 1; } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/build-sign-upload.sh`:
- Around line 83-84: The script later calls lipo to compute APP_ARCHS and
CLI_ARCHS but the preflight tool check does not include lipo, so add lipo to the
dependency/tools list used by the preflight check (the same place where other
required tools are validated—e.g., the tools array or
check_tools/check_dependencies function) so the script verifies lipo is present
before the build proceeds; this ensures the lipo binary is validated alongside
the other tools and will fail fast if missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c60bc82-b44f-46b2-989a-c4f032e2b203
📒 Files selected for processing (2)
.github/workflows/release.ymlscripts/build-sign-upload.sh
Comment on lines
+83
to
+84
| APP_ARCHS="$(lipo -archs "$APP_BINARY")" | ||
| CLI_ARCHS="$(lipo -archs "$CLI_BINARY")" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the relevant sections of the build script
cat -n scripts/build-sign-upload.sh | head -100Repository: manaflow-ai/cmux
Length of output: 3844
🏁 Script executed:
# Also check if lipo is mentioned elsewhere in the file
rg -n "lipo" scripts/build-sign-upload.shRepository: manaflow-ai/cmux
Length of output: 147
🏁 Script executed:
# Find the tool gate check section
rg -n "for tool in" scripts/build-sign-upload.sh -A 5Repository: manaflow-ai/cmux
Length of output: 294
Add lipo to the pre-flight dependency check.
Lines 83-84 use lipo -archs as a hard requirement, but the tool gate check (line 56) doesn't include it. This means a missing lipo will only be caught after the build finishes, not at preflight.
🔧 Suggested fix
-for tool in zig xcodebuild create-dmg xcrun codesign ditto gh; do
+for tool in zig xcodebuild create-dmg xcrun codesign ditto gh lipo; do🤖 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 83 - 84, The script later calls
lipo to compute APP_ARCHS and CLI_ARCHS but the preflight tool check does not
include lipo, so add lipo to the dependency/tools list used by the preflight
check (the same place where other required tools are validated—e.g., the tools
array or check_tools/check_dependencies function) so the script verifies lipo is
present before the build proceeds; this ensures the lipo binary is validated
alongside the other tools and will fail fast if missing.
Contributor
Greptile SummaryThis PR forces the stable macOS release build to produce a universal binary by passing Key changes:
Issues found:
Confidence Score: 4/5
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+87
to
+88
| [[ "$APP_ARCHS" == *arm64* && "$APP_ARCHS" == *x86_64* ]] | ||
| [[ "$CLI_ARCHS" == *arm64* && "$CLI_ARCHS" == *x86_64* ]] |
Contributor
There was a problem hiding this comment.
Silent arch assertion failures lack diagnostic output
When either binary is missing an architecture, these [[ ... ]] assertions fail with exit code 1 (due to set -euo pipefail on line 2), but produce no error message. Developers only see a generic "exited with code 1", making triage difficult. This same issue exists in .github/workflows/release.yml lines 172–173.
Adding explicit error messages makes failures immediately diagnosable:
Suggested change
| [[ "$APP_ARCHS" == *arm64* && "$APP_ARCHS" == *x86_64* ]] | |
| [[ "$CLI_ARCHS" == *arm64* && "$CLI_ARCHS" == *x86_64* ]] | |
| if ! [[ "$APP_ARCHS" == *arm64* && "$APP_ARCHS" == *x86_64* ]]; then | |
| echo "ERROR: app binary is not universal (got: $APP_ARCHS)" >&2; exit 1 | |
| fi | |
| if ! [[ "$CLI_ARCHS" == *arm64* && "$CLI_ARCHS" == *x86_64* ]]; then | |
| echo "ERROR: CLI binary is not universal (got: $CLI_ARCHS)" >&2; exit 1 | |
| fi |
There was a problem hiding this comment.
2 issues 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="scripts/build-sign-upload.sh">
<violation number="1" location="scripts/build-sign-upload.sh:72">
P2: `test -f` will silently abort (via `set -e`) with no error message if the binary is missing. Add a diagnostic so the operator knows *why* the build failed.</violation>
<violation number="2" location="scripts/build-sign-upload.sh:75">
P2: The architecture assertion silently exits if GhosttyKit isn't universal. Add an error message so the failure is self-explanatory.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| test -f "$GHOSTTYKIT_BINARY" | ||
| GHOSTTYKIT_ARCHS="$(lipo -archs "$GHOSTTYKIT_BINARY")" | ||
| echo "GhosttyKit architectures: $GHOSTTYKIT_ARCHS" | ||
| [[ "$GHOSTTYKIT_ARCHS" == *arm64* && "$GHOSTTYKIT_ARCHS" == *x86_64* ]] |
There was a problem hiding this comment.
P2: The architecture assertion silently exits if GhosttyKit isn't universal. Add an error message so the failure is self-explanatory.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/build-sign-upload.sh, line 75:
<comment>The architecture assertion silently exits if GhosttyKit isn't universal. Add an error message so the failure is self-explanatory.</comment>
<file context>
@@ -68,6 +68,12 @@ else
+test -f "$GHOSTTYKIT_BINARY"
+GHOSTTYKIT_ARCHS="$(lipo -archs "$GHOSTTYKIT_BINARY")"
+echo "GhosttyKit architectures: $GHOSTTYKIT_ARCHS"
+[[ "$GHOSTTYKIT_ARCHS" == *arm64* && "$GHOSTTYKIT_ARCHS" == *x86_64* ]]
+
# --- Build app (Release, unsigned) ---
</file context>
Suggested change
| [[ "$GHOSTTYKIT_ARCHS" == *arm64* && "$GHOSTTYKIT_ARCHS" == *x86_64* ]] | |
| [[ "$GHOSTTYKIT_ARCHS" == *arm64* && "$GHOSTTYKIT_ARCHS" == *x86_64* ]] || { echo "ERROR: GhosttyKit is not universal (got: $GHOSTTYKIT_ARCHS)" >&2; exit 1; } |
| fi | ||
|
|
||
| GHOSTTYKIT_BINARY="GhosttyKit.xcframework/macos-arm64_x86_64/libghostty.a" | ||
| test -f "$GHOSTTYKIT_BINARY" |
There was a problem hiding this comment.
P2: test -f will silently abort (via set -e) with no error message if the binary is missing. Add a diagnostic so the operator knows why the build failed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/build-sign-upload.sh, line 72:
<comment>`test -f` will silently abort (via `set -e`) with no error message if the binary is missing. Add a diagnostic so the operator knows *why* the build failed.</comment>
<file context>
@@ -68,6 +68,12 @@ else
fi
+GHOSTTYKIT_BINARY="GhosttyKit.xcframework/macos-arm64_x86_64/libghostty.a"
+test -f "$GHOSTTYKIT_BINARY"
+GHOSTTYKIT_ARCHS="$(lipo -archs "$GHOSTTYKIT_BINARY")"
+echo "GhosttyKit architectures: $GHOSTTYKIT_ARCHS"
</file context>
Suggested change
| test -f "$GHOSTTYKIT_BINARY" | |
| test -f "$GHOSTTYKIT_BINARY" || { echo "ERROR: universal GhosttyKit binary not found at $GHOSTTYKIT_BINARY" >&2; exit 1; } |
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)
.github/workflows/release.yml (1)
316-336:⚠️ Potential issue | 🟠 MajorDry-run appcasts point at non-existent GitHub release URLs.
The script at line 19 constructs
DOWNLOAD_URL_PREFIXashttps://github.com/manaflow-ai/cmux/releases/download/$TAG/, interpolating the provided tag directly. The workflow passesrelease_tag(which isverify-${SHORT_SHA}in verify mode) as that tag parameter. Since verify-mode runs do not create a GitHub Release at that synthetic tag, the uploaded appcast.xml contains enclosure URLs that will fail to resolve, making the verification appcast unusable for end-to-end Sparkle update testing.Either override
DOWNLOAD_URL_PREFIXfor verify-mode runs (e.g., point to the artifact download URL or use a mock URL), or skip generating/uploading the appcast when not publishing a release.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 316 - 336, The appcast generation uses DOWNLOAD_URL_PREFIX (set in ./scripts/sparkle_generate_appcast.sh) with the provided release_tag (steps.release_mode.outputs.release_tag), which in verify mode is a synthetic tag that has no GitHub Release; update the workflow to either (A) set/override DOWNLOAD_URL_PREFIX to a valid URL for verify-mode runs (for example the actions artifact download URL or a mock base URL) before invoking sparkles_generate_appcast.sh when steps.release_mode.outputs.publish_release != 'true', or (B) skip generating and uploading the appcast/appcast.xml in verify mode entirely by gating the sparkle_generate_appcast.sh invocation and the "Upload verification artifacts" step on steps.release_mode.outputs.publish_release == 'true'; modify the workflow conditions around the sparkles_generate_appcast.sh call and the upload steps (references: DOWNLOAD_URL_PREFIX in the script, release_tag/steps.release_mode.outputs.release_tag, sparkles_generate_appcast.sh invocation, and the "Upload verification artifacts" upload step) accordingly.
🤖 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 @.github/workflows/release.yml:
- Around line 316-336: The appcast generation uses DOWNLOAD_URL_PREFIX (set in
./scripts/sparkle_generate_appcast.sh) with the provided release_tag
(steps.release_mode.outputs.release_tag), which in verify mode is a synthetic
tag that has no GitHub Release; update the workflow to either (A) set/override
DOWNLOAD_URL_PREFIX to a valid URL for verify-mode runs (for example the actions
artifact download URL or a mock base URL) before invoking
sparkles_generate_appcast.sh when steps.release_mode.outputs.publish_release !=
'true', or (B) skip generating and uploading the appcast/appcast.xml in verify
mode entirely by gating the sparkle_generate_appcast.sh invocation and the
"Upload verification artifacts" step on
steps.release_mode.outputs.publish_release == 'true'; modify the workflow
conditions around the sparkles_generate_appcast.sh call and the upload steps
(references: DOWNLOAD_URL_PREFIX in the script,
release_tag/steps.release_mode.outputs.release_tag, sparkles_generate_appcast.sh
invocation, and the "Upload verification artifacts" upload step) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 09c25462-cb00-4180-8894-ed4f56c02408
📒 Files selected for processing (2)
.github/workflows/release.ymlscripts/build-sign-upload.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/build-sign-upload.sh
There was a problem hiding this comment.
🧹 Nitpick comments (3)
scripts/create_release_dmg.sh (1)
32-53: Version detection fallback is reasonable but relies on help text stability.The detection logic first attempts to parse
--versionoutput, then falls back to--helptext. The heuristic for legacy detection (<output_name.dmg> <source_folder>) is fragile if help text changes across versions.Consider documenting this heuristic in a comment for future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/create_release_dmg.sh` around lines 32 - 53, Add a clarifying comment above the detect_create_dmg_mode function explaining the fallback heuristic: we first try to parse the major version from the tool's --version output and, if that fails, we detect "legacy" by checking for the specific help text fragment "<output_name.dmg> <source_folder>"; note that this is a brittle string match tied to current help text formatting and should be updated if the tool's --help output changes or extended to a more robust detection method in the future. Include references to the variables used (version_output, help_output, major) and the intended outputs ("legacy" vs "modern") so maintainers know what to preserve when modifying the logic..github/workflows/release.yml (1)
26-38: Consider grouping redirects for cleaner shell output (optional).Static analysis flagged SC2129 suggesting grouped redirects. While functional as-is, grouping can improve readability:
♻️ Optional refactor
if [[ "${GITHUB_EVENT_NAME}" == "push" && "${GITHUB_REF:-}" == refs/tags/* ]]; then - echo "publish_release=true" >> "$GITHUB_OUTPUT" - echo "release_tag=${GITHUB_REF_NAME}" >> "$GITHUB_OUTPUT" - echo "artifact_name=release-${GITHUB_REF_NAME}" >> "$GITHUB_OUTPUT" + { + echo "publish_release=true" + echo "release_tag=${GITHUB_REF_NAME}" + echo "artifact_name=release-${GITHUB_REF_NAME}" + } >> "$GITHUB_OUTPUT" else REF_SLUG="$(printf '%s' "${GITHUB_REF_NAME}" | tr '/[:space:]' '-' | tr -cd '[:alnum:]-_.')" SHORT_SHA="${GITHUB_SHA::7}" - echo "publish_release=false" >> "$GITHUB_OUTPUT" - echo "release_tag=verify-${SHORT_SHA}" >> "$GITHUB_OUTPUT" - echo "artifact_name=release-verification-${REF_SLUG}-${SHORT_SHA}" >> "$GITHUB_OUTPUT" + { + echo "publish_release=false" + echo "release_tag=verify-${SHORT_SHA}" + echo "artifact_name=release-verification-${REF_SLUG}-${SHORT_SHA}" + } >> "$GITHUB_OUTPUT" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 26 - 38, The shell script writes multiple lines to the same file via repeated echo ... >> "$GITHUB_OUTPUT" which triggers SC2129; group the redirects inside the run block by writing all outputs to GITHUB_OUTPUT in a single here-doc or grouped redirection so that publishing and verification branches set publish_release, release_tag, and artifact_name together without repeated >> ops; update the conditional branches that currently echo "publish_release=...", "release_tag=...", and "artifact_name=..." to use a single grouped write to "$GITHUB_OUTPUT" (referencing GITHUB_OUTPUT, publish_release, release_tag, artifact_name, and the existing branch logic using GITHUB_EVENT_NAME/GITHUB_REF_NAME/GITHUB_SHA) so static analysis is satisfied and shell output is cleaner.tests/test_create_release_dmg.sh (1)
99-171: Test cases cover the critical paths.The four test cases validate:
- Modern binary uses
--overwriteand--identity- Legacy with npx falls back to modern via npx
- Legacy without npx uses legacy flags (
--app-drop-link,--codesign)- Require modern without npx fails appropriately
Consider adding a test case for the
--no-code-signflag when no signing identity is provided, to ensure the modern path handles unsigned builds correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_create_release_dmg.sh` around lines 99 - 171, Add a new test function (similar to case_modern_binary) that exercises the modern code path when no signing identity is available and the script should pass the --no-code-sign flag; call run_script with FAKE_CREATE_DMG_VERSION="8.0.0" and no SIGNING-ID/identity environment so the code chooses unsigned behavior, then assert the DMG was produced, grep the create-dmg log for "--no-code-sign" and assert npx was not invoked (npx log is empty); reference the existing case_modern_binary and run_script usage to copy setup/teardown and logging variable names.
🤖 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/release.yml:
- Around line 26-38: The shell script writes multiple lines to the same file via
repeated echo ... >> "$GITHUB_OUTPUT" which triggers SC2129; group the redirects
inside the run block by writing all outputs to GITHUB_OUTPUT in a single
here-doc or grouped redirection so that publishing and verification branches set
publish_release, release_tag, and artifact_name together without repeated >>
ops; update the conditional branches that currently echo "publish_release=...",
"release_tag=...", and "artifact_name=..." to use a single grouped write to
"$GITHUB_OUTPUT" (referencing GITHUB_OUTPUT, publish_release, release_tag,
artifact_name, and the existing branch logic using
GITHUB_EVENT_NAME/GITHUB_REF_NAME/GITHUB_SHA) so static analysis is satisfied
and shell output is cleaner.
In `@scripts/create_release_dmg.sh`:
- Around line 32-53: Add a clarifying comment above the detect_create_dmg_mode
function explaining the fallback heuristic: we first try to parse the major
version from the tool's --version output and, if that fails, we detect "legacy"
by checking for the specific help text fragment "<output_name.dmg>
<source_folder>"; note that this is a brittle string match tied to current help
text formatting and should be updated if the tool's --help output changes or
extended to a more robust detection method in the future. Include references to
the variables used (version_output, help_output, major) and the intended outputs
("legacy" vs "modern") so maintainers know what to preserve when modifying the
logic.
In `@tests/test_create_release_dmg.sh`:
- Around line 99-171: Add a new test function (similar to case_modern_binary)
that exercises the modern code path when no signing identity is available and
the script should pass the --no-code-sign flag; call run_script with
FAKE_CREATE_DMG_VERSION="8.0.0" and no SIGNING-ID/identity environment so the
code chooses unsigned behavior, then assert the DMG was produced, grep the
create-dmg log for "--no-code-sign" and assert npx was not invoked (npx log is
empty); reference the existing case_modern_binary and run_script usage to copy
setup/teardown and logging variable names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 45d339fd-adf2-4acc-a23c-e22549e4fa01
📒 Files selected for processing (5)
.github/workflows/nightly.yml.github/workflows/release.ymlscripts/build-sign-upload.shscripts/create_release_dmg.shtests/test_create_release_dmg.sh
There was a problem hiding this comment.
2 issues found across 5 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_create_release_dmg.sh">
<violation number="1" location="tests/test_create_release_dmg.sh:12">
P2: Shadowing the system `TMPDIR` env var causes all child processes (including the script-under-test and its `mktemp` calls) to use the test's working directory as the temp root. Rename to a test-local variable (e.g., `TEST_TMPDIR`) and substitute all references to avoid polluting the child process environment.</violation>
</file>
<file name="scripts/build-sign-upload.sh">
<violation number="1" location="scripts/build-sign-upload.sh:59">
P2: Pre-flight check doesn't match the actual `CMUX_CREATE_DMG_REQUIRE_MODERN=1` requirement. A legacy `create-dmg` (without `npx`) passes this check but `create_release_dmg.sh` will reject it later, after the expensive build/sign/notarize steps have already completed. Consider also checking for a modern `create-dmg` or `npx` here.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| exit 1 | ||
| fi | ||
|
|
||
| TMPDIR="$(mktemp -d)" |
There was a problem hiding this comment.
P2: Shadowing the system TMPDIR env var causes all child processes (including the script-under-test and its mktemp calls) to use the test's working directory as the temp root. Rename to a test-local variable (e.g., TEST_TMPDIR) and substitute all references to avoid polluting the child process environment.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_create_release_dmg.sh, line 12:
<comment>Shadowing the system `TMPDIR` env var causes all child processes (including the script-under-test and its `mktemp` calls) to use the test's working directory as the temp root. Rename to a test-local variable (e.g., `TEST_TMPDIR`) and substitute all references to avoid polluting the child process environment.</comment>
<file context>
@@ -0,0 +1,194 @@
+ exit 1
+fi
+
+TMPDIR="$(mktemp -d)"
+trap 'rm -rf "$TMPDIR"' EXIT
+
</file context>
| for tool in zig xcodebuild xcrun codesign ditto gh; do | ||
| command -v "$tool" >/dev/null || { echo "MISSING: $tool" >&2; exit 1; } | ||
| done | ||
| if ! command -v create-dmg >/dev/null 2>&1 && ! command -v npx >/dev/null 2>&1; then |
There was a problem hiding this comment.
P2: Pre-flight check doesn't match the actual CMUX_CREATE_DMG_REQUIRE_MODERN=1 requirement. A legacy create-dmg (without npx) passes this check but create_release_dmg.sh will reject it later, after the expensive build/sign/notarize steps have already completed. Consider also checking for a modern create-dmg or npx here.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/build-sign-upload.sh, line 59:
<comment>Pre-flight check doesn't match the actual `CMUX_CREATE_DMG_REQUIRE_MODERN=1` requirement. A legacy `create-dmg` (without `npx`) passes this check but `create_release_dmg.sh` will reject it later, after the expensive build/sign/notarize steps have already completed. Consider also checking for a modern `create-dmg` or `npx` here.</comment>
<file context>
@@ -53,9 +53,13 @@ APP_PATH="build/Build/Products/Release/cmux.app"
+for tool in zig xcodebuild xcrun codesign ditto gh; do
command -v "$tool" >/dev/null || { echo "MISSING: $tool" >&2; exit 1; }
done
+if ! command -v create-dmg >/dev/null 2>&1 && ! command -v npx >/dev/null 2>&1; then
+ echo "MISSING: create-dmg or npx" >&2
+ exit 1
</file context>
There was a problem hiding this comment.
1 issue 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="tests/test_ci_create_dmg_pinned.sh">
<violation number="1" location="tests/test_ci_create_dmg_pinned.sh:15">
P2: This test enforces workflow command strings via grep instead of validating DMG packaging behavior/artifacts, which makes it brittle and violates the repo’s test-quality policy.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| for workflow in "${WORKFLOWS[@]}"; do | ||
| if ! grep -Eq 'npm install --global .*create-dmg@' "$workflow"; then | ||
| echo "FAIL: $workflow must install create-dmg with an explicit version" | ||
| if ! grep -Eq 'brew list create-dmg >/dev/null 2>&1 \|\| brew install create-dmg' "$workflow"; then |
There was a problem hiding this comment.
P2: This test enforces workflow command strings via grep instead of validating DMG packaging behavior/artifacts, which makes it brittle and violates the repo’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_create_dmg_pinned.sh, line 15:
<comment>This test enforces workflow command strings via grep instead of validating DMG packaging behavior/artifacts, which makes it brittle and violates the repo’s test-quality policy.</comment>
<file context>
@@ -11,15 +12,15 @@ WORKFLOWS=(
for workflow in "${WORKFLOWS[@]}"; do
- if ! grep -Eq 'npm install --global .*create-dmg@' "$workflow"; then
- echo "FAIL: $workflow must install create-dmg with an explicit version"
+ if ! grep -Eq 'brew list create-dmg >/dev/null 2>&1 \|\| brew install create-dmg' "$workflow"; then
+ echo "FAIL: $workflow must provision the Homebrew create-dmg formula"
exit 1
</file context>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
scripts/build-sign-upload.sh (1)
56-58:⚠️ Potential issue | 🟡 MinorAdd
lipoto the pre-flight dependency check.The script uses
lipo -archson lines 73, 89, and 90 to verify architectures, butlipois not included in the tool gate check. A missinglipowould only be caught after the build finishes.🔧 Suggested fix
-for tool in zig xcodebuild xcrun codesign ditto gh; do +for tool in zig xcodebuild xcrun codesign ditto gh lipo; do,
🤖 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 56 - 58, The pre-flight dependency check loop that iterates over tools ("for tool in zig xcodebuild xcrun codesign ditto gh; do") is missing lipo, which the script later calls with lipo -archs; update that tool list to include lipo so command -v verifies its presence before proceeding (i.e., add "lipo" to the space-separated list used in the for loop).
🧹 Nitpick comments (3)
scripts/create_release_dmg.sh (1)
32-53: Version detection fallback is reasonable but could log the heuristic being used.The
detect_create_dmg_modefunction has good fallback logic: first tries--version, then falls back to--helpoutput inspection. Consider adding a debug log when falling back to heuristic detection to aid troubleshooting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/create_release_dmg.sh` around lines 32 - 53, The detect_create_dmg_mode function should emit a debug message when it falls back from --version to inspecting --help so callers can see the heuristic used; update the function (around the branch after computing help_output) to log (e.g., via echo or the repository's logger) that it's using the help-output heuristic for the given bin_name and include the inspected snippet or the final decision ("legacy" vs "modern") alongside bin_name and help_output variable; keep the existing return values and only add the minimal debug logging before echoing the detected mode..github/workflows/release.yml (1)
23-35: Consider grouping echo statements per shellcheck SC2129.Static analysis suggests using brace grouping for multiple redirects to the same file.
🔧 Optional: Group redirects
- if [[ "${GITHUB_EVENT_NAME}" == "push" && "${GITHUB_REF:-}" == refs/tags/* ]]; then - echo "publish_release=true" >> "$GITHUB_OUTPUT" - echo "release_tag=${GITHUB_REF_NAME}" >> "$GITHUB_OUTPUT" - echo "artifact_name=release-${GITHUB_REF_NAME}" >> "$GITHUB_OUTPUT" - else + if [[ "${GITHUB_EVENT_NAME}" == "push" && "${GITHUB_REF:-}" == refs/tags/* ]]; then + { + echo "publish_release=true" + echo "release_tag=${GITHUB_REF_NAME}" + echo "artifact_name=release-${GITHUB_REF_NAME}" + } >> "$GITHUB_OUTPUT" + else REF_SLUG="$(printf '%s' "${GITHUB_REF_NAME}" | tr '/[:space:]' '-' | tr -cd '[:alnum:]-_.')" SHORT_SHA="${GITHUB_SHA::7}" - echo "publish_release=false" >> "$GITHUB_OUTPUT" - echo "release_tag=verify-${SHORT_SHA}" >> "$GITHUB_OUTPUT" - echo "artifact_name=release-verification-${REF_SLUG}-${SHORT_SHA}" >> "$GITHUB_OUTPUT" + { + echo "publish_release=false" + echo "release_tag=verify-${SHORT_SHA}" + echo "artifact_name=release-verification-${REF_SLUG}-${SHORT_SHA}" + } >> "$GITHUB_OUTPUT" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 23 - 35, The multiple echo lines that append to the same output file (writing publish_release, release_tag, artifact_name to "$GITHUB_OUTPUT") should be grouped to avoid SC2129; replace the separate echo ... >> "$GITHUB_OUTPUT" calls inside both branches (the block that checks GITHUB_EVENT_NAME and the else branch that sets REF_SLUG/SHORT_SHA) with a single grouped redirect (use { ... } >> "$GITHUB_OUTPUT") so all three variables (publish_release, release_tag, artifact_name) are written in one redirect for each branch, keeping the same variable names (publish_release, release_tag, artifact_name) and computed values (GITHUB_REF_NAME, REF_SLUG, SHORT_SHA).tests/test_ci_create_dmg_pinned.sh (1)
15-17: Grep pattern is fragile and may miss valid variations.The exact pattern match
brew list create-dmg >/dev/null 2>&1 || brew install create-dmgrequires workflows to use this precise syntax. Common variations like2>&1 >/dev/null,&>/dev/null, or different spacing would cause false failures.Consider a more flexible pattern:
🔧 Suggested fix
- if ! grep -Eq 'brew list create-dmg >/dev/null 2>&1 \|\| brew install create-dmg' "$workflow"; then + if ! grep -Eq 'brew (list|install).*create-dmg' "$workflow"; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_ci_create_dmg_pinned.sh` around lines 15 - 17, The current grep in tests/test_ci_create_dmg_pinned.sh is too strict (it expects the exact redirection order and spacing) and causes false negatives; change the check to be more flexible by ensuring the workflow contains a command that lists or installs create-dmg rather than matching the exact string: either search for both "brew list create-dmg" and "brew install create-dmg" separately (e.g., two greps) or use a single regex that allows any whitespace and common redirection variants and matches the presence of "brew (list|install) create-dmg"; update the test around the variable workflow to use that more permissive pattern so valid variations like "&>/dev/null" or swapped redirections pass.
🤖 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/create_release_dmg.sh`:
- Around line 168-173: The final call to create_dmg_modern "$path_bin" is
unreachable dead code because path_bin is either empty (exits) or already
handled earlier; remove this redundant invocation and ensure create_dmg_modern
is only called where the modern path detection branch (the code that checks
path_bin and the modern condition around lines that reference path_bin and
create_dmg_modern) invokes it once—delete the trailing call to create_dmg_modern
and rely on the existing branch that already calls create_dmg_modern when
appropriate.
---
Duplicate comments:
In `@scripts/build-sign-upload.sh`:
- Around line 56-58: The pre-flight dependency check loop that iterates over
tools ("for tool in zig xcodebuild xcrun codesign ditto gh; do") is missing
lipo, which the script later calls with lipo -archs; update that tool list to
include lipo so command -v verifies its presence before proceeding (i.e., add
"lipo" to the space-separated list used in the for loop).
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 23-35: The multiple echo lines that append to the same output file
(writing publish_release, release_tag, artifact_name to "$GITHUB_OUTPUT") should
be grouped to avoid SC2129; replace the separate echo ... >> "$GITHUB_OUTPUT"
calls inside both branches (the block that checks GITHUB_EVENT_NAME and the else
branch that sets REF_SLUG/SHORT_SHA) with a single grouped redirect (use { ... }
>> "$GITHUB_OUTPUT") so all three variables (publish_release, release_tag,
artifact_name) are written in one redirect for each branch, keeping the same
variable names (publish_release, release_tag, artifact_name) and computed values
(GITHUB_REF_NAME, REF_SLUG, SHORT_SHA).
In `@scripts/create_release_dmg.sh`:
- Around line 32-53: The detect_create_dmg_mode function should emit a debug
message when it falls back from --version to inspecting --help so callers can
see the heuristic used; update the function (around the branch after computing
help_output) to log (e.g., via echo or the repository's logger) that it's using
the help-output heuristic for the given bin_name and include the inspected
snippet or the final decision ("legacy" vs "modern") alongside bin_name and
help_output variable; keep the existing return values and only add the minimal
debug logging before echoing the detected mode.
In `@tests/test_ci_create_dmg_pinned.sh`:
- Around line 15-17: The current grep in tests/test_ci_create_dmg_pinned.sh is
too strict (it expects the exact redirection order and spacing) and causes false
negatives; change the check to be more flexible by ensuring the workflow
contains a command that lists or installs create-dmg rather than matching the
exact string: either search for both "brew list create-dmg" and "brew install
create-dmg" separately (e.g., two greps) or use a single regex that allows any
whitespace and common redirection variants and matches the presence of "brew
(list|install) create-dmg"; update the test around the variable workflow to use
that more permissive pattern so valid variations like "&>/dev/null" or swapped
redirections pass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 16dbd926-3ec4-496d-bbaf-4de619a14abd
📒 Files selected for processing (6)
.github/workflows/nightly.yml.github/workflows/release.ymlscripts/build-sign-upload.shscripts/create_release_dmg.shtests/test_ci_create_dmg_pinned.shtests/test_create_release_dmg.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/nightly.yml
- tests/test_create_release_dmg.sh
Comment on lines
+168
to
+173
| if [ -z "$path_bin" ]; then | ||
| echo "create-dmg is required but not found in PATH" >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| create_dmg_modern "$path_bin" |
There was a problem hiding this comment.
Line 173 appears to be unreachable dead code.
At this point in the control flow:
- If
path_binis empty, line 169 exits - If
path_binis non-empty, it must have been checked at line 158 as modern (since legacy was already checked at line 141), so line 159-160 would have handled it
The final create_dmg_modern "$path_bin" on line 173 cannot be reached.
🔧 Suggested fix
if [ -z "$path_bin" ]; then
echo "create-dmg is required but not found in PATH" >&2
exit 1
fi
-
-create_dmg_modern "$path_bin"📝 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.
Suggested change
| if [ -z "$path_bin" ]; then | |
| echo "create-dmg is required but not found in PATH" >&2 | |
| exit 1 | |
| fi | |
| create_dmg_modern "$path_bin" | |
| if [ -z "$path_bin" ]; then | |
| echo "create-dmg is required but not found in PATH" >&2 | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/create_release_dmg.sh` around lines 168 - 173, The final call to
create_dmg_modern "$path_bin" is unreachable dead code because path_bin is
either empty (exits) or already handled earlier; remove this redundant
invocation and ensure create_dmg_modern is only called where the modern path
detection branch (the code that checks path_bin and the modern condition around
lines that reference path_bin and create_dmg_modern) invokes it once—delete the
trailing call to create_dmg_modern and rely on the existing branch that already
calls create_dmg_modern when appropriate.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
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
ARCHS="arm64 x86_64"lipoand keep a dry-run workflow path that uploads verification artifacts instead of publishing a releasescripts/build-sign-upload.shhelper aligned with the release workflowTesting
bash -n scripts/build-sign-upload.shruby -e 'require "yaml"; YAML.load_file(".github/workflows/release.yml")'git diff --checkgh workflow run "Release macOS app" --repo manaflow-ai/cmux --ref task-universal-stable-releaseIssues
Summary by cubic
Make the stable macOS release universal for Apple Silicon and Intel, and keep a dry-run verification path. Enforce styled, signed DMGs via a new wrapper that prefers the Homebrew
create-dmg(legacy layout) and only falls back to moderncreate-dmgwhen styling isn't required, fixing the Applications drop target across environments.ARCHS="arm64 x86_64",ONLY_ACTIVE_ARCH=NO, and-destination 'generic/platform=macOS'.lipo, and writerelease-verification.txt.cmux-macos.dmg,appcast.xml,release-verification.txt) with dynamicrelease_tagand artifact name.scripts/build-sign-upload.shwith CI: same build flags and checks (including GhosttyKit verification).scripts/create_release_dmg.sh: require styled DMGs for signed release/nightly using the Homebrewcreate-dmg; fall back to moderncreate-dmg(PATH ornpx) when styling isn’t required; workflows provision Homebrew and avoid npm; tests ensure correct tool selection and layout.Written for commit 9243aa9. Summary will update on new commits.
Summary by CodeRabbit
Chores
New Features
Tests