new: publish to homebrew#1135
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a macOS release pipeline: introduces 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/release/release-macos-dmg.sh (1)
6-6: Unused variableDEFAULT_ENV_FILE.This variable is assigned but never referenced. It appears to be leftover from development.
🧹 Remove unused variable
REPO_ROOT="$(cd "$SCRIPT_DIR/../.." && pwd)" -DEFAULT_ENV_FILE="" if [[ -n "${SERVERBOX_RELEASE_ENV_FILE:-}" ]]; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/release/release-macos-dmg.sh` at line 6, The script defines an unused variable DEFAULT_ENV_FILE="" which should be removed; delete the DEFAULT_ENV_FILE assignment from release-macos-dmg.sh and verify there are no remaining references to DEFAULT_ENV_FILE elsewhere in the script (remove them if any) so the script contains only variables that are actually used.
🤖 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/release/release-macos-dmg.sh`:
- Line 6: The script defines an unused variable DEFAULT_ENV_FILE="" which should
be removed; delete the DEFAULT_ENV_FILE assignment from release-macos-dmg.sh and
verify there are no remaining references to DEFAULT_ENV_FILE elsewhere in the
script (remove them if any) so the script contains only variables that are
actually used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bbd43315-aeb4-4baa-af7f-bf1c9bfcfe46
📒 Files selected for processing (7)
.env.release.example.gitignoreMakefileREADME.mdREADME_zh.mdscripts/release/package-dmg-from-xcarchive.shscripts/release/release-macos-dmg.sh
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
scripts/release/release-macos-dmg.sh (1)
80-83:⚠️ Potential issue | 🔴 CriticalBlock recursive deletion of
/tmpand/var/foldersroots.At Line [81], exact
/tmpand/var/foldersare still accepted, and at Line [195] that value can flow intosafe_remove -rf. A misconfiguredDMG_STAGING_PATH=/tmpcan wipe the full temp root.💡 Proposed fix
validate_allowed_path() { local label="$1" local path="$2" local normalized normalized="$(normalize_abs_path "$path")" case "$normalized" in - "$REPO_ROOT/build" | "$REPO_ROOT/build/"* | /tmp | /tmp/* | /var/folders | /var/folders/*) + "$REPO_ROOT/build" | "$REPO_ROOT/build/"* | /tmp/* | /var/folders/*) printf '%s\n' "$normalized" ;; + /tmp | /var/folders) + echo "$label must be a subdirectory under /tmp or /var/folders: $normalized" >&2 + exit 1 + ;; *) echo "$label must be under $REPO_ROOT/build, /tmp, or /var/folders: $normalized" >&2 exit 1 ;; esac }Also applies to: 193-195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/release/release-macos-dmg.sh` around lines 80 - 83, The case branch that currently allows "$REPO_ROOT/build", "$REPO_ROOT/build/"* , /tmp and /var/folders lets an exact /tmp or /var/folders value flow into safe_remove -rf; change the validation so normalized (and any value derived from DMG_STAGING_PATH) explicitly rejects the root temp directories by checking normalized == "/tmp" or normalized == "/var/folders" (and their trailing-slash variants) and returning an error instead of printing and continuing; ensure safe_remove and any callers (e.g., the code path that uses DMG_STAGING_PATH) never receive these exact roots by adding this explicit equality guard before invoking safe_remove.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@scripts/release/release-macos-dmg.sh`:
- Around line 80-83: The case branch that currently allows "$REPO_ROOT/build",
"$REPO_ROOT/build/"* , /tmp and /var/folders lets an exact /tmp or /var/folders
value flow into safe_remove -rf; change the validation so normalized (and any
value derived from DMG_STAGING_PATH) explicitly rejects the root temp
directories by checking normalized == "/tmp" or normalized == "/var/folders"
(and their trailing-slash variants) and returning an error instead of printing
and continuing; ensure safe_remove and any callers (e.g., the code path that
uses DMG_STAGING_PATH) never receive these exact roots by adding this explicit
equality guard before invoking safe_remove.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: db4cc982-df18-4df1-b7ab-d12c3104c6c4
📒 Files selected for processing (1)
scripts/release/release-macos-dmg.sh
Contributor
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 `@scripts/release/release-macos-dmg.sh`:
- Around line 74-117: The validation functions (validate_allowed_path and
validate_path_within_base) currently compare raw normalized strings and can be
bypassed by symlinks; update them to canonicalize (resolve symlinks) for both
the candidate path and the base/allowed directories before performing the prefix
checks, e.g. call a realpath/readlink -f equivalent on "$path" and on
"$REPO_ROOT/build" (and any base passed into validate_path_within_base) or
explicitly detect and reject any path containing symlink components returned by
normalize_abs_path; ensure the prefix/case comparisons use these resolved values
(referencing normalize_abs_path, validate_allowed_path,
validate_path_within_base, and REPO_ROOT) and fail with an error if
canonicalization fails or any symlink is detected.
- Around line 198-215: The script doesn't create parent directories for
EXPORT_OPTIONS_PATH and OVERRIDE_XCCONFIG_PATH before writing to them, causing
"No such file or directory" when those are nested under BUILD_ROOT; add mkdir -p
"$(dirname "$EXPORT_OPTIONS_PATH")" and mkdir -p "$(dirname
"$OVERRIDE_XCCONFIG_PATH")" (or a single mkdir -p for both dirs) prior to using
/usr/libexec/PlistBuddy -c 'Clear dict' "$EXPORT_OPTIONS_PATH" and before the
cat >"$OVERRIDE_XCCONFIG_PATH" heredoc so the parent directories exist when
writing those files.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a8cbc129-7bd0-4eb3-9000-ebe0aec507fa
📒 Files selected for processing (1)
scripts/release/release-macos-dmg.sh
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/release/release-macos-dmg.sh (2)
335-339: Consider allowing custom release notes.The release is created with empty notes. An optional
RELEASE_NOTESenvironment variable would allow customization without modifying the script.💡 Optional: Support custom release notes
At variable defaults section (~line 228), add:
RELEASE_NOTES="${RELEASE_NOTES:-}"Then update the create command:
gh release create "$RELEASE_TAG" \ --repo "$APP_REPO_SLUG" \ --title "$RELEASE_TITLE" \ - --notes "" + --notes "$RELEASE_NOTES"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/release/release-macos-dmg.sh` around lines 335 - 339, Add support for optional custom release notes by introducing a RELEASE_NOTES environment variable (e.g., set RELEASE_NOTES="${RELEASE_NOTES:-}") and then pass it into the gh release create invocation instead of the empty string; update the gh release create call that uses RELEASE_TAG, APP_REPO_SLUG and RELEASE_TITLE to use --notes "$RELEASE_NOTES" so callers can supply custom notes via RELEASE_NOTES.
322-327: Consider adding a timeout for notarization.The
notarytool submit --waitcommand waits indefinitely for Apple's servers. In rare cases, notarization can take an extended time or hang. Consider wrapping with a timeout for CI reliability.💡 Optional: Add timeout wrapper
-xcrun notarytool submit "$DMG_PATH" \ - --keychain-profile "$APPLE_NOTARY_KEYCHAIN_PROFILE" \ - --wait +NOTARIZATION_TIMEOUT="${NOTARIZATION_TIMEOUT:-3600}" +if ! timeout "$NOTARIZATION_TIMEOUT" xcrun notarytool submit "$DMG_PATH" \ + --keychain-profile "$APPLE_NOTARY_KEYCHAIN_PROFILE" \ + --wait; then + echo "notarization timed out or failed after ${NOTARIZATION_TIMEOUT}s" >&2 + exit 1 +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/release/release-macos-dmg.sh` around lines 322 - 327, Wrap the xcrun notarytool submit "$DMG_PATH" --keychain-profile "$APPLE_NOTARY_KEYCHAIN_PROFILE" --wait call in a timeout so CI won't block indefinitely: replace the bare call with a timeout wrapper (e.g. timeout or gtimeout) that uses a configurable env var like NOTARIZE_TIMEOUT, check exit code for timeout vs success, and fail the script with a clear message if the timeout is reached; keep the subsequent xcrun stapler staple/validate steps conditional on a successful notarization return.
🤖 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/release/release-macos-dmg.sh`:
- Around line 335-339: Add support for optional custom release notes by
introducing a RELEASE_NOTES environment variable (e.g., set
RELEASE_NOTES="${RELEASE_NOTES:-}") and then pass it into the gh release create
invocation instead of the empty string; update the gh release create call that
uses RELEASE_TAG, APP_REPO_SLUG and RELEASE_TITLE to use --notes
"$RELEASE_NOTES" so callers can supply custom notes via RELEASE_NOTES.
- Around line 322-327: Wrap the xcrun notarytool submit "$DMG_PATH"
--keychain-profile "$APPLE_NOTARY_KEYCHAIN_PROFILE" --wait call in a timeout so
CI won't block indefinitely: replace the bare call with a timeout wrapper (e.g.
timeout or gtimeout) that uses a configurable env var like NOTARIZE_TIMEOUT,
check exit code for timeout vs success, and fail the script with a clear message
if the timeout is reached; keep the subsequent xcrun stapler staple/validate
steps conditional on a successful notarization return.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fe384001-7f38-43f5-b199-d55b4f1af0ba
📒 Files selected for processing (1)
scripts/release/release-macos-dmg.sh
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
1 participant
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.
Fixes #896
Summary by CodeRabbit
Documentation
New Features
Chores