GH#2941: fix multi-repo worktree auto-cleanup in pulse-wrapper.sh#2944
GH#2941: fix multi-repo worktree auto-cleanup in pulse-wrapper.sh#2944marcusquinn merged 1 commit intomainfrom
Conversation
The jq query in cleanup_worktrees() used '.[]' to iterate repos.json, but repos.json has top-level keys 'git_parent_dirs' and 'initialized_repos' — not a top-level array. This caused the query to silently return no results, falling through to the single-repo fallback every time. Multi-repo worktree cleanup has been broken since it was added. Fix: use '.initialized_repos[]' matching the pattern used by prefetch_state() and other functions in the same file. Also: defensive wc whitespace trimming, null-safe local_only check, total cleanup summary logging, and enhanced function documentation.
Summary of ChangesHello, 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 resolves a critical bug in the 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
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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 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: Fri Mar 6 01:50:10 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![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 effectively fixes a bug in the multi-repo worktree cleanup logic by correcting the jq query. The added defensive improvements, suchs as the null-safe local_only check and whitespace-safe wc output, enhance the script's robustness. The expanded documentation is also a great addition for clarity. I have a couple of minor suggestions to improve debuggability by removing unnecessary error suppression, in line with the repository's general guidelines.
Note: Security Review did not run due to the size of the PR.
| # worktree-helper.sh uses gh pr list for squash-merge detection. | ||
| local repo_paths | ||
| repo_paths=$(jq -r '.[] | select(.local_only != true) | .path' "$repos_json" 2>/dev/null || echo "") | ||
| repo_paths=$(jq -r '.initialized_repos[] | select((.local_only // false) == false) | .path' "$repos_json" 2>/dev/null || echo "") |
There was a problem hiding this comment.
While the || echo "" correctly handles potential jq command failures when set -e is active, suppressing stderr with 2>/dev/null can hide valuable diagnostic information. If the repos.json file is malformed or there are permission issues, those errors would be silenced. It's better to let these errors be visible for easier debugging.
As per the repository's general rules, it's preferable to avoid blanket error suppression.
| repo_paths=$(jq -r '.initialized_repos[] | select((.local_only // false) == false) | .path' "$repos_json" 2>/dev/null || echo "") | |
| repo_paths=$(jq -r '.initialized_repos[] | select((.local_only // false) == false) | .path' "$repos_json" || echo "") |
References
- In shell scripts with 'set -e' enabled, use '|| true' to prevent the script from exiting when a command like 'jq' fails on an optional lookup. Do not suppress stderr with '2>/dev/null' so that actual syntax or system errors remain visible for debugging.
| local cleaned_output | ||
| cleaned_output=$(git -C "$repo_path" worktree list 2>/dev/null | wc -l) | ||
| local wt_count | ||
| wt_count=$(git -C "$repo_path" worktree list 2>/dev/null | wc -l | tr -d ' ') |
There was a problem hiding this comment.
Similar to the jq command, suppressing stderr for this git command with 2>/dev/null is not ideal. Although you check for the existence of the .git directory beforehand, other issues like file permissions could cause git worktree list to fail. Allowing these errors to be reported would make debugging easier.
The repository's general rules advise against suppressing stderr when a file's existence has already been checked, as it can mask other important issues.
| wt_count=$(git -C "$repo_path" worktree list 2>/dev/null | wc -l | tr -d ' ') | |
| wt_count=$(git -C "$repo_path" worktree list | wc -l | tr -d ' ') |
References
- Avoid using
2>/dev/nullto suppress errors on file operations if the file's existence has already been verified by a preceding check (e.g.,[[ -f "$file" ]]or an early return). This practice is redundant for 'file not found' errors and can mask other important issues like permissions problems.
Remove 2>/dev/null from jq and git worktree list commands in cleanup_worktrees() — both had prior existence checks making the suppression redundant for file-not-found errors, while masking permission issues and malformed JSON diagnostics. Addresses review feedback from Gemini on PR #2944. Closes #2946
Remove 2>/dev/null from jq and git worktree list commands in cleanup_worktrees() — both had prior existence checks making the suppression redundant for file-not-found errors, while masking permission issues and malformed JSON diagnostics. Addresses review feedback from Gemini on PR #2944. Closes #2946
1 participant
Summary
cleanup_worktrees()that prevented multi-repo worktree cleanup from ever running —.[]iterated top-level JSON keys instead of.initialized_repos[]wcoutput, null-safelocal_onlycheck, total cleanup summary loggingRoot Cause
repos.jsonstructure is{"git_parent_dirs": [...], "initialized_repos": [...]}. The old queryjq -r '.[] | select(.local_only != true) | .path'iterated the two top-level values (an array and an array), not the repo entries insideinitialized_repos. This silently returned no paths, so the function always fell through to the single-repo fallback — meaning only the aidevops repo itself was ever cleaned, not the other managed repos.Testing
prefetch_state(),update_health_issues(), andrun_daily_quality_sweep()in the same fileCloses #2941
Summary by CodeRabbit
Bug Fixes
Chores