Real-world Example: Homebrew Delete with Confirmation (#13)#14
Real-world Example: Homebrew Delete with Confirmation (#13)#14dan-snelson wants to merge 1 commit into
Conversation
* Real-world Example: Homebrew Delete with Confirmation Signed-off-by: Dan K. Snelson <24623109+dan-snelson@users.noreply.github.com> * Align skill guidance with user-run demos and Jamf/root-run workflows (#14) * Review of PR #13 Codex-syncronized examples with current 'skills/' Signed-off-by: Dan K. Snelson <24623109+dan-snelson@users.noreply.github.com> * Review of PR #13 Codex-syncronized examples and updated 'skills/' Signed-off-by: Dan K. Snelson <24623109+dan-snelson@users.noreply.github.com> --------- Signed-off-by: Dan K. Snelson <24623109+dan-snelson@users.noreply.github.com> --------- Signed-off-by: Dan K. Snelson <24623109+dan-snelson@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR cleans up skill-pack documentation after a merge conflict and aligns the swiftDialog builder guidance with real-world user-run vs Jamf/root-run workflows, while adding a concrete “Homebrew uninstall with confirmation” example in the playground.
Changes:
- Remove merge-conflict markers and consolidate guidance around command-file workflows (user-run
mktempvs Jamf/root-run/var/tmpownership handoff). - Update Codex/Claude skill references and templates to consistently document the Jamf/root-run command-file handoff pattern.
- Add a new playground “Homebrew delete with confirmation” prompt and a corresponding Zsh implementation.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| skills/README.md | Removes merge-conflict residue and retains the consolidated workflow guidance. |
| skills/codex-swiftdialog-builder/SKILL.md | Aligns command-file guidance with user-run vs Jamf/root-run handoff. |
| skills/codex-swiftdialog-builder/references/authoring-patterns.md | Updates background/command-file conventions and examples to include /var/tmp handoff. |
| skills/codex-swiftdialog-builder/references/advanced-patterns.md | Removes conflicting alternative text and standardizes the recommended workflow steps. |
| skills/codex-swiftdialog-builder/README.md | Documents the split between user-run mktemp and Jamf/root-run /var/tmp handoff patterns. |
| skills/codex-swiftdialog-builder/assets/templates/progress-dialog.zsh | Ensures the template reflects the Jamf/root-run /var/tmp ownership handoff workflow. |
| skills/claude-swiftdialog-builder/templates/progress-dialog.zsh | Mirrors the Jamf/root-run template updates for the Claude skill pack. |
| skills/claude-swiftdialog-builder/references/authoring-patterns.md | Standardizes command-file guidance and examples with /var/tmp handoff. |
| skills/claude-swiftdialog-builder/references/advanced-patterns.md | Removes conflict fallout and aligns workflow guidance with the updated conventions. |
| skills/claude-swiftdialog-builder/README.md | Updates documented conventions to include /var/tmp ownership handoff pattern. |
| skills/claude-swiftdialog-builder/CLAUDE.md | Aligns background dialog + command-file guidance with the updated convention. |
| playground/README.md | Updates the referenced generated-script conventions (incl. Jamf/root-run /var/tmp handoff). |
| playground/homebrew-delete-confirmation.zsh | Adds a full Jamf/root-run Homebrew uninstall script with confirmation + progress UI and hash verification. |
| playground/homebrew-delete-confirmation.md | Adds the corresponding internal prompt/spec used to generate the Homebrew example script. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # --- Step 4: Remove /opt/homebrew if it remains --- | ||
| send_command "progress: 4" | ||
| send_command "progresstext: Step 4/5: Removing /opt/homebrew..." | ||
| set_step_status "Delete /opt/homebrew directory" "wait" "Removing..." | ||
|
|
||
| if [[ -e "$HOMEBREW_DIRECTORY" ]]; then | ||
| if ! /bin/rm -rfv "$HOMEBREW_DIRECTORY" >>"$LOG_FILE" 2>&1; then | ||
| finish_with_error \ | ||
| "Delete /opt/homebrew directory" \ |
There was a problem hiding this comment.
Step 4 hard-codes deletion of /opt/homebrew via HOMEBREW_DIRECTORY, even though detect_homebrew_binary() can select an Intel install under /usr/local. This can either fail to clean up the actual Homebrew prefix or (worse) delete an unrelated /opt/homebrew directory. Derive the target directory from the detected brew (e.g., brew --prefix) and use that consistently in the warning/progress messaging and deletion logic.
| # Internal: Homebrew Uninstall Confirmation (Jamf/root-run) | ||
|
|
||
| DIALOG="/usr/local/bin/dialog" | ||
| UNINSTALL_URL="https://raw.githubusercontent.com/Homebrew/install/HEAD/uninstall.sh" |
There was a problem hiding this comment.
UNINSTALL_URL points at .../HEAD/uninstall.sh but EXPECTED_SHA256 is pinned to a single script version. Any upstream change to HEAD will cause this workflow to fail until the hash is updated. To keep this reproducible (and the hash meaningful), pin the download URL to the specific commit/tag that matches the expected hash, or otherwise introduce a process for updating both together.
| UNINSTALL_URL="https://raw.githubusercontent.com/Homebrew/install/HEAD/uninstall.sh" | |
| # Pin the Homebrew uninstall script to the exact immutable ref that matches EXPECTED_SHA256. | |
| UNINSTALL_REF="REPLACE_WITH_HOMEbrew_INSTALL_COMMIT_OR_TAG_MATCHING_EXPECTED_SHA256" | |
| UNINSTALL_URL="https://raw.githubusercontent.com/Homebrew/install/${UNINSTALL_REF}/uninstall.sh" |
| prepare_log_file() { | ||
| local target_file="$1" | ||
|
|
||
| if [[ -z "$target_file" || ! -e "$target_file" ]]; then | ||
| return 1 | ||
| fi | ||
|
|
||
| if ! /bin/chmod 644 "$target_file" 2>/dev/null; then | ||
| return 1 | ||
| fi | ||
|
|
||
| return 0 |
There was a problem hiding this comment.
The log file is created in /var/tmp and then chmod'd to 644, making it world-readable. The uninstall output can include system paths/usernames and other environment details; consider restricting this to 600 and (if the GUI user needs to read it) handing ownership/permissions to the logged-in user similarly to the command-file handoff pattern.
| /bin/rm -f "/var/tmp/dialog.log" | ||
|
|
There was a problem hiding this comment.
reveal_log_file() unconditionally deletes /var/tmp/dialog.log, which is not created by this script and could belong to another workflow. This looks like leftover debug cleanup; remove it or only delete files this script created.
| /bin/rm -f "/var/tmp/dialog.log" |
Real-world Example: Homebrew Delete with Confirmation
Align skill guidance with user-run demos and Jamf/root-run workflows (Real-world Example: Homebrew Delete with Confirmation (#13) #14)
Review of PR Align skill guidance with user-run demos and Jamf/root-run workflows #13