Restore macOS DMG layout after re-package#939
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughThe script Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
scripts/release/repackage-dmg.sh (2)
92-105: The final check doesn't actually assert the Finder layout survived.This verifies the
.appbundle and/Applicationssymlink, but it would still pass if the hidden layout assets were missing. Since the goal here is to preserve the background and icon placement, consider also checking for the expected Finder metadata files before declaring success.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/release/repackage-dmg.sh` around lines 92 - 105, The verification currently only checks for the .app bundle and Applications symlink but doesn't assert the Finder layout files; update the final verification to also verify the presence of the hidden Finder metadata by checking for "$VERIFY_MOUNT/.background" (or "$VERIFY_MOUNT/.background/" folder), "$VERIFY_MOUNT/.VolumeIcon.icns", and "$VERIFY_MOUNT/.DS_Store" (using the VERIFY_MOUNT and APP_NAME variables and DMG_PATH context) and exit with an error if any of those expected layout assets are missing so the script ensures background/icon placement and DS_Store layout survived.
77-89: Stage the rebuilt DMG in a temp path until the whole flow succeeds.Line 78 overwrites the only copy of
DMG_PATHbefore notarization, stapling, and final verification have all passed. Writing the rebuilt image to a temporary path and moving it into place only at the end would make this release step much easier to recover from when a later step fails.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/release/repackage-dmg.sh` around lines 77 - 89, The script currently overwrites DMG_PATH when running hdiutil convert and then proceeds to notarize and staple, which risks losing the original if a later step fails; modify the flow so hdiutil convert writes to a temporary path (e.g., DMG_TEMP) instead of DMG_PATH, keep DMG_RW and the original DMG_PATH untouched while running xcrun notarytool submit and xcrun stapler staple against DMG_TEMP, and only on successful notarization and stapling move/rename DMG_TEMP into DMG_PATH (and clean up DMG_RW/DMG_TEMP on failure); update references to DMG_RW, DMG_PATH, hdiutil convert, xcrun notarytool submit, and xcrun stapler staple accordingly so the final replacement is atomic.
🤖 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/repackage-dmg.sh`:
- Around line 68-70: The rm -rf operation uses APP_NAME derived from APP_PATH
without validation; add a defensive check after computing APP_NAME and before rm
-rf/ditto to ensure APP_NAME is non-empty and matches an expected .app bundle
pattern (e.g., ends with ".app" and optionally equals "OpenHuman.app"), and
abort with an error/exit code if the check fails. Refer to the variables
APP_PATH, APP_NAME and MOUNT_DIR and the existing rm -rf "$MOUNT_DIR/$APP_NAME"
/ ditto "$APP_PATH" "$MOUNT_DIR/$APP_NAME" lines when inserting the guard.
---
Nitpick comments:
In `@scripts/release/repackage-dmg.sh`:
- Around line 92-105: The verification currently only checks for the .app bundle
and Applications symlink but doesn't assert the Finder layout files; update the
final verification to also verify the presence of the hidden Finder metadata by
checking for "$VERIFY_MOUNT/.background" (or "$VERIFY_MOUNT/.background/"
folder), "$VERIFY_MOUNT/.VolumeIcon.icns", and "$VERIFY_MOUNT/.DS_Store" (using
the VERIFY_MOUNT and APP_NAME variables and DMG_PATH context) and exit with an
error if any of those expected layout assets are missing so the script ensures
background/icon placement and DS_Store layout survived.
- Around line 77-89: The script currently overwrites DMG_PATH when running
hdiutil convert and then proceeds to notarize and staple, which risks losing the
original if a later step fails; modify the flow so hdiutil convert writes to a
temporary path (e.g., DMG_TEMP) instead of DMG_PATH, keep DMG_RW and the
original DMG_PATH untouched while running xcrun notarytool submit and xcrun
stapler staple against DMG_TEMP, and only on successful notarization and
stapling move/rename DMG_TEMP into DMG_PATH (and clean up DMG_RW/DMG_TEMP on
failure); update references to DMG_RW, DMG_PATH, hdiutil convert, xcrun
notarytool submit, and xcrun stapler staple accordingly so the final replacement
is atomic.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c0a7a2e8-8166-4f97-8cbb-e72e1b8273b3
📒 Files selected for processing (1)
scripts/release/repackage-dmg.sh
| APP_NAME="$(basename "$APP_PATH")" | ||
| rm -rf "$MOUNT_DIR/$APP_NAME" | ||
| ditto "$APP_PATH" "$MOUNT_DIR/$APP_NAME" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 'locate-app|app_path|repackage-dmg\.sh' .github scriptsRepository: tinyhumansai/openhuman
Length of output: 11644
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the actual repackage-dmg.sh file around lines 68-70
echo "=== Checking repackage-dmg.sh around lines 68-70 ==="
sed -n '60,75p' scripts/release/repackage-dmg.sh
echo ""
echo "=== Checking the entire repackage-dmg.sh file ==="
wc -l scripts/release/repackage-dmg.shRepository: tinyhumansai/openhuman
Length of output: 725
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the locate-app step validation logic from the workflow
echo "=== locate-app validation logic from release.yml ==="
sed -n '612,650p' .github/workflows/release.ymlRepository: tinyhumansai/openhuman
Length of output: 1850
Add validation to guard the destructive remove operation.
The script relies on upstream validation from the workflow step to ensure APP_PATH is a .app bundle, but lacks internal safeguards. While the actual risk is mitigated—APP_PATH is always OpenHuman.app in practice and the mount point is a temporary directory—defensive coding should validate the resolved name before deletion.
Proposed hardening
APP_NAME="$(basename "$APP_PATH")"
+[[ "$APP_NAME" == *.app && "$APP_NAME" != "/" ]] || {
+ echo "[dmg] ERROR: unexpected app bundle path: $APP_PATH"
+ exit 1
+}
-rm -rf "$MOUNT_DIR/$APP_NAME"
+rm -rf "${MOUNT_DIR:?}/${APP_NAME:?}"
ditto "$APP_PATH" "$MOUNT_DIR/$APP_NAME"📝 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.
| APP_NAME="$(basename "$APP_PATH")" | |
| rm -rf "$MOUNT_DIR/$APP_NAME" | |
| ditto "$APP_PATH" "$MOUNT_DIR/$APP_NAME" | |
| APP_NAME="$(basename "$APP_PATH")" | |
| [[ "$APP_NAME" == *.app && "$APP_NAME" != "/" ]] || { | |
| echo "[dmg] ERROR: unexpected app bundle path: $APP_PATH" | |
| exit 1 | |
| } | |
| rm -rf "${MOUNT_DIR:?}/${APP_NAME:?}" | |
| ditto "$APP_PATH" "$MOUNT_DIR/$APP_NAME" |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 69-69: Use "${var:?}" to ensure this never expands to / .
(SC2115)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/release/repackage-dmg.sh` around lines 68 - 70, The rm -rf operation
uses APP_NAME derived from APP_PATH without validation; add a defensive check
after computing APP_NAME and before rm -rf/ditto to ensure APP_NAME is non-empty
and matches an expected .app bundle pattern (e.g., ends with ".app" and
optionally equals "OpenHuman.app"), and abort with an error/exit code if the
check fails. Refer to the variables APP_PATH, APP_NAME and MOUNT_DIR and the
existing rm -rf "$MOUNT_DIR/$APP_NAME" / ditto "$APP_PATH"
"$MOUNT_DIR/$APP_NAME" lines when inserting the guard.
1 participant
Updated scripts/release/repackage-dmg.sh to use the original Tauri-generated DMG as a template. This ensures that custom backgrounds, icon positions, and the Applications symlink are preserved when re-packaging the DMG with the notarized .app bundle. Added a verification step and a cleanup trap for robustness.
PR created automatically by Jules for task 1199539227628305615 started by @senamakel
Summary by CodeRabbit