feat: Rosetta audit helper + shell linter optimisations (t301)#1184
feat: Rosetta audit helper + shell linter optimisations (t301)#1184marcusquinn merged 1 commit intomainfrom
Conversation
Summary of ChangesHello @marcusquinn, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the development environment setup and maintenance by introducing a utility to optimize Homebrew package management on Apple Silicon, ensuring native performance for installed tools. Concurrently, it significantly boosts the efficiency of shell script linting and formatting processes, streamlining code quality checks for shell scripts. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds shfmt as an advisory pre-pass to the shell linter pipeline, introduces a new rosetta-audit-helper.sh CLI for detecting and migrating x86 Homebrew packages on Apple Silicon, and extends setup.sh to detect/install shell linting tools and to offer Rosetta audit integration. Changes
Sequence DiagramsequenceDiagram
autonumber
participant User
participant RosettaAudit as Rosetta<br/>Audit Script
participant BrewARM as ARM<br/>Homebrew
participant BrewX86 as x86<br/>Homebrew
participant Analyzer as Package<br/>Analyzer
participant Migrator as Migration<br/>Executor
User->>RosettaAudit: scan
RosettaAudit->>RosettaAudit: detect Apple Silicon & dual-brew
RosettaAudit->>BrewARM: list installed packages
RosettaAudit->>BrewX86: list installed packages
BrewARM-->>RosettaAudit: ARM list
BrewX86-->>RosettaAudit: x86 list
RosettaAudit->>Analyzer: categorize packages
Analyzer-->>RosettaAudit: migratable / non-migratable
RosettaAudit-->>User: summary & recommendations
User->>RosettaAudit: migrate (--dry-run)?
RosettaAudit->>Migrator: run Phase 1 (remove duplicates)
Migrator->>BrewX86: uninstall duplicates
Migrator->>BrewARM: ensure ARM installs (Phase 2)
Migrator->>BrewX86: remove x86-only after ARM installs
Migrator-->>RosettaAudit: per-package status
RosettaAudit-->>User: migration report
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Thu Feb 12 00:30:07 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @.agents/scripts/rosetta-audit-helper.sh:
- Around line 156-161: cmd_scan creates temporary files (migratable_file,
non_migratable_file) and stores them in _SCAN_CLEANUP_FILES but doesn’t register
them with the existing cleanup mechanism, so they can leak on unexpected exit;
modify cmd_scan to call _save_cleanup_scope and push_cleanup (the same pattern
used by check_positional_parameters) immediately after creating migratable_file
and non_migratable_file so the files are removed on exit, and then delete the
manual rm -f $_SCAN_CLEANUP_FILES calls (leave _SCAN_CLEANUP_FILES assignment
intact so other code can reference it).
- Around line 339-368: The cmd_migrate function currently always returns 0;
update its exit behavior so callers can detect failures by returning a non-zero
status when failures occurred: after the block that checks "$failed" and prints
the warning (the branch that prints "Migration completed with $failed
failures"), change the final return to return a non-zero value (e.g., return
$failed or return 1) while keeping dry-run and success paths returning 0; locate
the cmd_migrate function and the "$failed" variable usage to implement this
change so setup.sh and other callers can react to partial failures.
- Around line 44-54: The functions is_apple_silicon and has_dual_brew rely on
the fragile pattern "[[ ]] && [[ ]]; return $?" which can cause unexpected exits
under set -e; change each to an explicit boolean return path so the function
body never lets a failing test trigger errexit — e.g., evaluate the compound
condition and then return 0 on success and return 1 on failure (or use a guarded
"if ...; then return 0; else return 1; fi" around the existing tests) so callers
like "is_apple_silicon && do_something" won't cause the script to abort
unexpectedly.
In @.agents/subagent-index.toon:
- Around line 97-98: Update the TOON header script count from scripts[74] to
scripts[76] on the header line referenced (change the scripts[...] value near
line 90); ensure the header reflects the actual inventory that now includes
"linters-local.sh" and "rosetta-audit-helper.sh" so the total script count
matches 76.
In `@setup.sh`:
- Around line 2069-2077: The current check can misdetect universal (fat)
binaries because sc_arch uses grep ... | head -1 and may pick x86_64 even when
arm64 is present; update the logic around sc_path/sc_arch in the shellcheck
detection: instead of treating presence of x86_64 as the Rosetta case, test
whether the shellcheck binary lacks arm64 support (e.g., inspect the output of
file "$sc_path" and check for the presence of "arm64" with grep -q or similar)
and only warn (via print_warning/print_info) when uname -m is arm64 and the file
output does NOT contain arm64; otherwise report success with shellcheck
--version as before.
- Around line 2103-2109: The loop unconditionally calls brew and prints
duplicate success; change it to use the detected pkg_manager and the existing
installer helper: replace the for loop over missing_tools that calls
run_with_spinner "Installing $tool" brew install "$tool" with a call to
install_packages "$pkg_manager" "$tool" (or invoke install_packages once with
the missing_tools list if supported) and remove the redundant print_success call
(keep run_with_spinner's success/failure handling via run_with_spinner and its
exit codes). Ensure you reference the existing symbols missing_tools,
pkg_manager, run_with_spinner, and install_packages when updating the code.
🧹 Nitpick comments (2)
.agents/scripts/linters-local.sh (1)
240-246: File collection pattern is duplicated betweenrun_shfmtandrun_shellcheck.Both functions iterate
.agents/scripts/*.shwith the same guard. Consider extracting a shared helper (e.g.,collect_shell_files()) to keep things DRY. Not blocking — just a future-proofing nit.♻️ Example extraction
+# Collect shell files for linting +collect_shell_files() { + local -n _files_ref=$1 + for file in .agents/scripts/*.sh; do + [[ -f "$file" ]] && _files_ref+=("$file") + done + return 0 +} + run_shfmt() { ... - local sh_files=() - for file in .agents/scripts/*.sh; do - [[ -f "$file" ]] && sh_files+=("$file") - done + local sh_files=() + collect_shell_files sh_files ...setup.sh (1)
2160-2176: Consider runningmigrate --dry-runfirst before the actual migration.When the user answers "y" at line 2163, the script jumps straight to
migrate(line 2167), which performs actual package reinstallation. Given that the PR provides a--dry-runmode, it would be safer to show a preview first and then confirm.♻️ Proposed two-step migration flow
if [[ "$run_audit" =~ ^[Yy]$ ]]; then local script_dir script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" - if [[ -f "$script_dir/.agents/scripts/rosetta-audit-helper.sh" ]]; then - bash "$script_dir/.agents/scripts/rosetta-audit-helper.sh" migrate - elif [[ -f "$HOME/.aidevops/agents/scripts/rosetta-audit-helper.sh" ]]; then - bash "$HOME/.aidevops/agents/scripts/rosetta-audit-helper.sh" migrate + local audit_script="" + if [[ -f "$script_dir/.agents/scripts/rosetta-audit-helper.sh" ]]; then + audit_script="$script_dir/.agents/scripts/rosetta-audit-helper.sh" + elif [[ -f "$HOME/.aidevops/agents/scripts/rosetta-audit-helper.sh" ]]; then + audit_script="$HOME/.aidevops/agents/scripts/rosetta-audit-helper.sh" + fi + if [[ -n "$audit_script" ]]; then + bash "$audit_script" migrate --dry-run + echo "" + local confirm_migrate + read -r -p "Proceed with migration? [y/N]: " confirm_migrate + if [[ "$confirm_migrate" =~ ^[Yy]$ ]]; then + bash "$audit_script" migrate + else + print_info "Skipped — run manually: rosetta-audit-helper.sh migrate" + fi else print_warning "rosetta-audit-helper.sh not found — run setup again after deployment" fi
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable rosetta-audit-helper.sh script for Apple Silicon users and significantly improves the performance of linters-local.sh. The changes are well-structured and the new features are a great addition. I've identified a few areas for improvement, mainly concerning script robustness, adherence to the repository's style guide, and a bug in the setup script that affects non-macOS users. My comments provide specific suggestions to address these points.
| local migratable_file non_migratable_file | ||
| migratable_file=$(mktemp "${TMPDIR:-/tmp}/rosetta-migrate.XXXXXX") | ||
| non_migratable_file=$(mktemp "${TMPDIR:-/tmp}/rosetta-nomigrate.XXXXXX") | ||
| _SCAN_CLEANUP_FILES="$migratable_file $non_migratable_file" |
There was a problem hiding this comment.
The script's temporary file handling can be improved for robustness and to adhere to the style guide.
- Guaranteed Cleanup: A
trapshould be used to ensure temporary files are removed even if the script exits unexpectedly. This is required by style guide rule feat: add /session-review and /full-loop commands for comprehensive AI workflow #33. - Robust File List: The
_SCAN_CLEANUP_FILESglobal variable should be an array to be robust against filenames with special characters and avoidshellcheck disable=SC2086.
I recommend defining _SCAN_CLEANUP_FILES as an array and setting an EXIT trap at the top of the script.
At top of script (e.g., after set -euo pipefail):
_SCAN_CLEANUP_FILES=()
trap 'rm -f "${_SCAN_CLEANUP_FILES[@]}"' EXITThen, this block can be updated to populate the array. The explicit rm calls at the end of cmd_scan can then be removed.
| local migratable_file non_migratable_file | |
| migratable_file=$(mktemp "${TMPDIR:-/tmp}/rosetta-migrate.XXXXXX") | |
| non_migratable_file=$(mktemp "${TMPDIR:-/tmp}/rosetta-nomigrate.XXXXXX") | |
| _SCAN_CLEANUP_FILES="$migratable_file $non_migratable_file" | |
| local migratable_file non_migratable_file | |
| migratable_file=$(mktemp "${TMPDIR:-/tmp}/rosetta-migrate.XXXXXX") | |
| non_migratable_file=$(mktemp "${TMPDIR:-/tmp}/rosetta-nomigrate.XXXXXX") | |
| _SCAN_CLEANUP_FILES=("$migratable_file" "$non_migratable_file") |
References
- Temp files must have
trapcleanup (RETURN or EXIT). The current implementation creates temp files but does not guarantee their cleanup if the script exits unexpectedly. (link)
setup.sh
Outdated
|
|
||
| if [[ "$install_linters" =~ ^[Yy]?$ ]]; then | ||
| for tool in "${missing_tools[@]}"; do | ||
| if run_with_spinner "Installing $tool" brew install "$tool"; then |
There was a problem hiding this comment.
The installation command for shell linting tools is hardcoded to brew install. This will fail on Linux systems where apt, dnf, etc., should be used. The install_packages helper function should be used here with the detected $pkg_manager to make the installation cross-platform.
| if run_with_spinner "Installing $tool" brew install "$tool"; then | |
| if run_with_spinner "Installing $tool" install_packages "$pkg_manager" "$tool"; then |
| echo " [DRY RUN] Would remove x86: $pkg (ARM version already installed)" | ||
| ((removed_dups++)) | ||
| else | ||
| if "$X86_BREW" uninstall "$pkg" 2>/dev/null; then |
There was a problem hiding this comment.
Redirecting stderr to /dev/null suppresses potentially useful error messages from brew uninstall. This makes debugging failures difficult and violates style guide rule #50, which states that 2>/dev/null should only be used when redirecting to log files, not for blanket suppression. Since the command is inside an if statement, removing the redirection will print the error without exiting the script, providing better diagnostic information on failure.
| if "$X86_BREW" uninstall "$pkg" 2>/dev/null; then | |
| if "$X86_BREW" uninstall "$pkg"; then |
References
2>/dev/nullis acceptable ONLY when redirecting to log files, not blanket suppression. This code uses it to suppress errors from brew commands, which hinders debugging. (link)
marcusquinn
force-pushed
the
feature/t301-rosetta-audit
branch
from
3385c5b to
7f8e858
Compare
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Thu Feb 12 00:42:10 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
…etup steps (t301) - rosetta-audit-helper.sh: add migrate, dry-run, categorisation, caching, defensive returns, cleanup pattern, proper exit codes - setup.sh: add setup_shell_linting_tools and setup_rosetta_audit steps, use install_packages (not hardcoded brew), handle universal binaries - linters-local.sh: add shfmt pre-pass, convert shellcheck to batch mode - Addresses CodeRabbit and Gemini review feedback on PR #1184
marcusquinn
force-pushed
the
feature/t301-rosetta-audit
branch
from
7f8e858 to
013b271
Compare
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Thu Feb 12 00:58:06 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
1 participant
Summary
New
rosetta-audit-helper.sh: Detects x86 Homebrew packages running under Rosetta 2 emulation on Apple Silicon Macs. Categorises packages as migratable (ARM formula available), duplicate (already in both brews), or x86-only (no ARM formula). Supportsscan,migrate --dry-run, andmigratecommands. Intel Macs and Linux skip gracefully.setup.sh integration: New
setup_shell_linting_toolsstep installs shellcheck + shfmt with spinner feedback. Newsetup_rosetta_auditstep detects dual-brew setups and offers migration. Both use existingrun_with_spinnerandconfirm_steppatterns.linters-local.sh improvements: Added shfmt as a fast advisory pre-pass. Converted shellcheck from per-file invocation to batch mode (single process for all files) — significantly faster. Shows issue counts and file-level detail.
Benchmark Data (M1 Max)
Testing
rosetta-audit-helper.sh scan— verified on Apple Silicon with dual-brew setup (122 x86 + 128 ARM packages)rosetta-audit-helper.sh migrate --dry-run— correctly categorises 75 migratable, 44 duplicates, 3 non-migratablerosetta-audit-helper.sh status— quick summary with cachelinters-local.sh— shfmt pre-pass + batched shellcheck both workingsetup.sh— syntax verified withbash -nFiles Changed
.agents/scripts/rosetta-audit-helper.shsetup.sh,.agents/scripts/linters-local.sh,.agents/subagent-index.toon,README.mdCloses t301
Summary by CodeRabbit
New Features
Chores