Skip to content

Implement YAML-driven sjust-based macOS defaults with user overrides#114

Draft
Copilot wants to merge 8 commits intomasterfrom
copilot/fix-99
Draft

Implement YAML-driven sjust-based macOS defaults with user overrides#114
Copilot wants to merge 8 commits intomasterfrom
copilot/fix-99

Conversation

Copy link
Contributor

Copilot AI commented Aug 8, 2025

  • Implement comprehensive macOS system defaults configuration optimized for developer productivity
  • UNIFIED IMPLEMENTATION: Single sjust-based approach eliminating code duplication
    • Remove separate Ansible task file (defaults.yml) to eliminate duplication
    • Implement unified sjust task that applies 40+ developer-focused defaults directly using defaults command
    • Update Ansible playbook to execute sjust task instead of separate implementation
    • Both Ansible (--tags macos-defaults) and standalone usage (sjust macos-defaults) execute identical logic
    • FIXED: Ensure sjust is installed before macos-defaults task execution
  • SparkJust Commands for macOS defaults management:
    • Add sjust macos-defaults command to apply all developer defaults
    • Add sjust macos-defaults-check command to show current defaults status
    • Add sjust macos-defaults-reset command to reset selected defaults (with safety prompts)
    • NEW: Add sjust macos-defaults-init-overrides command to create user customization files
  • NEW: Add reduce motion accessibility setting (com.apple.universalaccess.plist reduceMotion 1)
    • Improves focus and performance for developers
    • Integrated into unified sjust implementation
    • Documented in README.md feature list
  • NEW: YAML-based configuration system
    • Move all 40+ settings from hardcoded bash to structured YAML (config/macos/defaults.yml)
    • Add user override capability via ~/.local/spark/macos-defaults/overrides.yml
    • Install yq as dependency for YAML processing
    • Automatic merging of user overrides with default configuration
    • Enhanced readability and discoverability of all settings
  • SIMPLIFIED: Ansible dependency management
    • Remove duplicate tagging - sjust installation only tagged with sjust
    • macOS defaults task only runs when sjust is installed (using file existence check)
    • Clean separation of concerns between installation and execution
  • Add documentation for YAML-based approach and user override examples

The implementation provides a single, unified YAML-driven approach where both Ansible and standalone usage execute the same sjust task, eliminating code duplication and simplifying maintenance. Users can now easily customize settings via YAML overrides without touching the core configuration. All 40+ developer defaults are now discoverable and documented in structured YAML format.

ENHANCED USER EXPERIENCE: The new YAML-based system allows users to override any default setting through a simple configuration file (~/.local/spark/macos-defaults/overrides.yml), making customization accessible without modifying core system files.


💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

@paolomainardi
Copy link
Member

@copilot try again to implement this PR

Co-authored-by: paolomainardi <8747+paolomainardi@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Aug 8, 2025

@copilot try again to implement this PR

I've successfully implemented the macOS defaults provisioning as requested in issue #99. The implementation includes:

New Ansible task file (ansible/macos/macos/defaults.yml) with 40+ developer-focused macOS defaults
Full integration into the main playbook with proper tagging (macos-defaults)
Comprehensive coverage: UI/UX, Finder, Dock, keyboard/trackpad, Safari developer settings, Terminal, and system tools
Selective execution support with --tags macos-defaults or --skip-tags macos-defaults
Updated documentation in README.md with usage examples

The implementation follows Jeff Geerling's approach using the community.general.osx_defaults module and is optimized for developer productivity. All syntax checks pass successfully.

Commit: 5f742ff

Copilot AI changed the title [WIP] Automatic provisioning of MacOS defaults Implement automatic provisioning of macOS defaults for developers Aug 8, 2025
@paolomainardi paolomainardi requested a review from Copilot August 14, 2025 16:39
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements comprehensive macOS system defaults configuration optimized for developer productivity, automatically applying 40+ system-level settings to enhance the development experience. The solution integrates seamlessly with the existing Ansible provisioning system using proper tagging for selective execution.

Key Changes

  • New Ansible task file with developer-focused macOS defaults for UI/UX, Finder, Dock, keyboard, Safari, and development tools
  • Integration into the main provisioning playbook with macos-defaults tag support
  • Documentation updates showing usage examples and selective deployment options

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
ansible/macos/macos/defaults.yml New comprehensive macOS defaults configuration with 40+ developer-optimized settings
ansible/macos/macos/base.yml Integration of defaults task into main provisioning workflow
README.md Documentation updates explaining the new defaults feature and usage examples

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

value: true
tags: macos-defaults

- name: Enable Safari Developer menu
Copy link

Copilot AI Aug 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate functionality: This task (line 300-306) appears to duplicate the functionality of the previous task 'Enable Safari's developer menu' (line 292-298). Both tasks enable Safari's developer menu using different keys. Consider consolidating or clarifying the distinction between these settings.

Suggested change
- name: Enable Safari Developer menu
- name: Enable Safari's Develop menu

Copilot uses AI. Check for mistakes.

- name: Restart Activity Monitor to apply changes
command: killall "Activity Monitor"
ignore_errors: true
Copy link

Copilot AI Aug 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The restart tasks use command module with killall which may not be the most robust approach. Consider using ansible.builtin.shell with error handling or checking if the process exists before attempting to kill it. The current approach may generate unnecessary error messages in logs.

Suggested change
ignore_errors: true
ansible.builtin.shell: |
if pgrep Finder > /dev/null; then
killall Finder
fi
tags: macos-defaults
- name: Restart Dock to apply changes
ansible.builtin.shell: |
if pgrep Dock > /dev/null; then
killall Dock
fi
tags: macos-defaults
- name: Restart Safari to apply changes
ansible.builtin.shell: |
if pgrep Safari > /dev/null; then
killall Safari
fi
tags: macos-defaults
- name: Restart Activity Monitor to apply changes
ansible.builtin.shell: |
if pgrep "Activity Monitor" > /dev/null; then
killall "Activity Monitor"
fi

Copilot uses AI. Check for mistakes.
paolomainardi and others added 2 commits September 11, 2025 12:02
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@paolomainardi
Copy link
Member

@copilot, I would like to migrate the Ansible implementation to a simple task that Ansible can reuse, making it easier for users to run without the burden of executing the Ansible provisioner.

Another thing I want to add is this com.apple.universalaccess.plist reduceMotion 1

Co-authored-by: paolomainardi <8747+paolomainardi@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Sep 11, 2025

@copilot, I would like to migrate the Ansible implementation to a simple task that Ansible can reuse, making it easier for users to run without the burden of executing the Ansible provisioner.

Anot...

I've successfully migrated the Ansible implementation to standalone sjust tasks that make it much easier for users to apply macOS defaults without running the full provisioner:

New SparkJust commands:

  • sjust macos-defaults - Apply all 40+ developer-optimized macOS defaults
  • sjust macos-defaults-check - Preview what would be changed (dry-run mode)
  • sjust macos-defaults-reset - Reset selected defaults with safety prompts

Added the requested setting:

  • com.apple.universalaccess.plist reduceMotion 1 for better focus and performance

The standalone tasks provide user-friendly output, progress indicators, and safety features while maintaining full compatibility with the existing Ansible integration. Users can now easily apply just the macOS defaults without the complexity of running the full provisioner.

Commit: 8725bc8

@paolomainardi
Copy link
Member

@copilot, I would like to migrate the Ansible implementation to a simple task that Ansible can reuse, making it easier for users to run without the burden of executing the Ansible provisioner.
Anot...

I've successfully migrated the Ansible implementation to standalone sjust tasks that make it much easier for users to apply macOS defaults without running the full provisioner:

New SparkJust commands:

  • sjust macos-defaults - Apply all 40+ developer-optimized macOS defaults
  • sjust macos-defaults-check - Preview what would be changed (dry-run mode)
  • sjust macos-defaults-reset - Reset selected defaults with safety prompts

Added the requested setting:

  • com.apple.universalaccess.plist reduceMotion 1 for better focus and performance

The standalone tasks provide user-friendly output, progress indicators, and safety features while maintaining full compatibility with the existing Ansible integration. Users can now easily apply just the macOS defaults without the complexity of running the full provisioner.

Commit: 8725bc8

Help improve Copilot by leaving feedback using the 👍 or 👎 buttons

@copilot i still see the ansible implementation, this isn't correct. I want to have the implementation just as an sjust task that ansible will execute.

Co-authored-by: paolomainardi <8747+paolomainardi@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Sep 11, 2025

@copilot, I would like to migrate the Ansible implementation to a simple task that Ansible can reuse, making it easier for users to run without the burden of executing the Ansible provisioner.
...

I've successfully migrated the implementation to a unified sjust approach as requested. The changes include:

Removed the separate Ansible implementation:

  • Deleted ansible/macos/macos/defaults.yml file
  • Updated Ansible playbook to call the sjust task directly

Unified sjust implementation:

  • Rewrote sjust macos-defaults to apply defaults directly using the defaults command
  • Updated sjust macos-defaults-check to show current defaults status
  • Both Ansible and standalone usage now execute the same implementation

Benefits:

  • Eliminates code duplication between Ansible and sjust
  • Easier maintenance with single source of truth
  • Users can apply defaults with sjust macos-defaults or via Ansible with --tags macos-defaults
  • Both approaches use identical logic

The reduce motion accessibility setting (com.apple.universalaccess.plist reduceMotion 1) is also included in the unified implementation.

Commit: 6f820e8

Copilot AI changed the title Implement automatic provisioning of macOS defaults for developers Implement unified sjust-based macOS defaults for developers Sep 11, 2025
- Add both 'sjust' and 'macos-defaults' tags to sjust installation block
- Add both tags to macOS defaults application task
- Now running --tags "macos-defaults" includes sjust installation
- Ensures sjust is always available before executing macos-defaults

Co-authored-by: paolomainardi <8747+paolomainardi@users.noreply.github.com>
Copy link
Member

@paolomainardi paolomainardi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

echo "${yellow}You may need to restart applications or reboot for all changes to take effect.${normal}"
else
echo "Reset cancelled."
fi No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to try making this list of settings easier by putting them in a YAML file, so they are easily discoverable and readable, and then process the YAML file to apply the settings.

We should have a way to let the user to override this default in a file ~/.local/spark/macos-defaults/overrides.yml that will override the original settings.

We should use a language that has a native YAML processor. I do not want to write a script that is too complex and requires dependencies. Otherwise we can install https://github.com/mikefarah/yq with brew and use it from a shell script.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented YAML-based configuration system as requested. All 40+ settings are now defined in config/macos/defaults.yml for better readability and discoverability. Added user override support via ~/.local/spark/macos-defaults/overrides.yml that automatically merges with default settings. Using yq (installed via Homebrew) for YAML processing from shell scripts. Added sjust macos-defaults-init-overrides command to help users get started with customization.

Commit: 61c1468

Co-authored-by: paolomainardi <8747+paolomainardi@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Sep 11, 2025

@copilot

Completed the refactoring to implement both requested changes:

  1. Simplified Ansible dependency: Removed duplicate tagging and added file existence check so macOS defaults only runs when sjust is installed
  2. YAML-based configuration: Moved all settings to structured YAML with user override capability via ~/.local/spark/macos-defaults/overrides.yml

The system now provides better discoverability, easier customization, and cleaner dependency management.

Commit: 61c1468

Copilot AI changed the title Implement unified sjust-based macOS defaults for developers Implement YAML-driven sjust-based macOS defaults with user overrides Sep 11, 2025
Copilot AI requested a review from paolomainardi September 11, 2025 11:18
@paolomainardi
Copy link
Member

@claude review this MR and let me know what do you think about this implementation and what improvements we can make

@claude
Copy link

claude bot commented Dec 13, 2025

Claude finished @paolomainardi's task in 1m 53s —— View job


Code Review Complete ✅

I've reviewed the YAML-driven sjust-based macOS defaults implementation. Overall, this is a well-architected solution that successfully achieves the goals of:

  1. Eliminating code duplication between Ansible and standalone usage
  2. Making settings discoverable and maintainable through YAML
  3. Providing user customization through overrides

Strengths 💪

Architecture & Design

  • Excellent separation of concerns: The unified sjust approach eliminates duplication between Ansible and standalone execution paths
  • YAML-driven configuration: All 40+ settings are now structured, documented, and easily discoverable in config/macos/defaults.yml
  • User override system: The merge strategy using yq eval-all '. as $item ireduce ({}; . * $item)' is elegant and functional
  • Clear categorization: Settings are logically grouped (ui_ux, keyboard, accessibility, finder, dock, safari, etc.)

Implementation Quality

  • Proper error handling: All bash scripts follow project standards with set -euo pipefail
  • Variable quoting: Consistent use of ${variable} syntax with proper bracing
  • Dependency checking: yq availability is verified before execution
  • Temporary file cleanup: Merged configs are properly cleaned up
  • User-friendly output: Color-coded feedback using the project's libformatting library

User Experience

  • Four well-designed commands:
    • macos-defaults - Apply settings
    • macos-defaults-check - Preview status
    • macos-defaults-reset - Reset with safety prompts
    • macos-defaults-init-overrides - Initialize user customization
  • Safety features: Confirmation prompts for destructive operations
  • Documentation: Comprehensive README updates with examples

Issues & Improvements 🔧

Critical Issues

1. Ansible Dependency Check Is Flawed ⚠️

Location: ansible/macos/macos/base.yml:397

when: "'/usr/local/bin/sjust' is exists"

Problem: This uses incorrect Jinja2 syntax. The is exists test doesn't work with string literals.

Fix: Use proper Ansible file stat check:

- name: Check if sjust is installed
  ansible.builtin.stat:
    path: /usr/local/bin/sjust
  register: sjust_binary

- name: Apply macOS system defaults for developers
  shell: /usr/local/bin/sjust macos-defaults
  become: no
  tags: macos-defaults
  when: sjust_binary.stat.exists

Alternative simpler fix:

when: "lookup('file', '/usr/local/bin/sjust', errors='ignore') != ''"

2. Shell Variable Expansion Security Risk ⚠️

Location: sjust/recipes/03-macos-defaults.just:79

expanded_value=$(eval echo "${value}")

Problem: Using eval on YAML-sourced data is dangerous. If a user (or attacker) puts $(rm -rf /) in their overrides YAML, it will execute.

Safer alternatives:

# Option 1: Use envsubst (if available)
expanded_value=$(echo "${value}" | envsubst)

# Option 2: Only expand specific known variables
expanded_value="${value//\$\{HOME\}/${HOME}}"

# Option 3: Use printf with parameter expansion
expanded_value=$(printf '%s' "${value}" | sed "s|\${HOME}|${HOME}|g")

3. Array Handling Missing Quotes 🐛

Location: sjust/recipes/03-macos-defaults.just:85

defaults write "${domain}" "${key}" -array ${array_values}

Problem: ${array_values} is unquoted, which will break with values containing spaces.

Fix:

# Better approach: process array items individually
while IFS= read -r array_item; do
    if [[ -n "${array_item}" ]]; then
        array_args+=("-array-add" "${array_item}")
    fi
done < <(echo "${item}" | yq eval '.value[]' -)

if [[ ${#array_args[@]} -gt 0 ]]; then
    defaults write "${domain}" "${key}" "${array_args[@]}"
fi

Medium Priority Issues

4. Inconsistent Error Messages

Some error messages say "Error: yq is required" while success messages use emojis ("✅"). Consider standardizing:

  • Either use emojis throughout for consistency
  • Or use text-only for better terminal compatibility

5. Missing Shellcheck in Documentation

The project requires all shell scripts to pass shellcheck (per .github/copilot-instructions.md), but this isn't mentioned in the PR. Consider running:

shellcheck sjust/recipes/03-macos-defaults.just

6. Restart Applications Logic Could Be More Robust

Location: sjust/recipes/03-macos-defaults.just:122-126

for app in ${restart_apps}; do
    app_clean=$(echo "${app}" | tr -d '"')
    killall "${app_clean}" 2>/dev/null || true
done

Issue: If yq returns quoted strings like "Activity Monitor", the for loop will split on spaces.

Better approach:

while IFS= read -r app; do
    if [[ -n "${app}" && "${app}" != "null" ]]; then
        # Remove any surrounding quotes
        app_clean=$(echo "${app}" | sed 's/^"//;s/"$//')
        if pgrep -x "${app_clean}" >/dev/null 2>&1; then
            echo "  Restarting ${app_clean}..."
            killall "${app_clean}" 2>/dev/null || true
        fi
    fi
done < <(yq eval '.restart_applications[]' "${config_to_use}")

Minor Issues

7. YAML Type Consistency

In config/macos/defaults.yml, the accessibility.reduceMotion uses type: int with value: 1, but this is semantically a boolean. Consider documenting why int is used here (macOS requires it) or use a comment:

accessibility:
  - domain: com.apple.universalaccess
    key: reduceMotion
    type: int  # macOS requires int (0/1) rather than bool
    value: 1
    description: "Reduce motion for better focus and performance"

8. Missing Validation for User Overrides

If a user creates an invalid YAML override file, the merge will fail silently or with cryptic yq errors. Consider adding validation:

# After checking if overrides exist
if [[ -f "${user_overrides}" ]]; then
    if ! yq eval '.' "${user_overrides}" >/dev/null 2>&1; then
        echo "${red}Error: Invalid YAML syntax in ${user_overrides}${normal}"
        echo "Please fix the syntax and try again."
        exit 1
    fi
    echo "${green}Merging user overrides...${normal}"
    # ... rest of merge logic
fi

9. Documentation: Add Troubleshooting Section

The README is excellent but could benefit from a troubleshooting section:

  • What to do if yq is not installed
  • How to check which settings were applied
  • How to manually reset a specific setting
  • Common override examples

Suggested Enhancements 🚀

Enhancement 1: Add Dry-Run Mode to Apply Command

Allow users to see what would change before applying:

sjust macos-defaults --dry-run

This could reuse the check logic but with different formatting.

Enhancement 2: Selective Category Application

Allow users to apply only specific categories:

sjust macos-defaults --category finder,dock

Enhancement 3: Export Current Settings

Add a command to export current macOS settings to a YAML file:

sjust macos-defaults-export > ~/my-current-settings.yml

This would help users create their override files.

Enhancement 4: Diff Command

Show what settings differ between config and current state:

sjust macos-defaults-diff

Enhancement 5: Add Pre-Apply Backup

Before applying defaults, create a backup of current settings:

# In macos-defaults command, before applying
backup_dir="${HOME}/.local/spark/macos-defaults/backups"
mkdir -p "${backup_dir}"
backup_file="${backup_dir}/backup-$(date +%Y%m%d-%H%M%S).yml"
# ... export current settings to backup_file

Testing Recommendations 🧪

  1. Test with malicious override file to verify security
  2. Test with spaces in values (e.g., screenshot location)
  3. Test with missing yq to verify error handling
  4. Test with invalid YAML in overrides
  5. Test Ansible integration with --tags macos-defaults
  6. Verify all 40+ settings actually apply correctly on a fresh macOS install

Summary

This is a solid implementation that significantly improves maintainability and user experience. The critical issues (Ansible condition syntax, eval security risk, array handling) should be fixed before merging, but the overall architecture is excellent.

Recommendation: Fix the critical issues, consider the medium-priority improvements, and this will be ready to merge.

Priority fixes:

  1. ✅ Fix Ansible when condition syntax
  2. ✅ Replace eval with safer variable expansion
  3. ✅ Fix array handling with proper quoting
  4. ⚡ Add YAML validation for user overrides

Great work on this refactoring! The YAML-driven approach is much more maintainable than the previous hardcoded bash implementation.


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants