fix: stash dirty working tree before batch auto-release (t276)#1119
fix: stash dirty working tree before batch auto-release (t276)#1119marcusquinn merged 1 commit intomainfrom
Conversation
trigger_batch_release() fails when version-manager.sh encounters uncommitted changes (e.g. todo/VERIFY.md from parallel sessions). Stash before pull+release, restore after regardless of outcome.
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 addresses an issue where automated batch releases could fail if the Git working directory contained uncommitted changes. It introduces a robust solution to temporarily stash these changes before the release process and restore them afterwards, ensuring the release can proceed smoothly without losing local modifications. Highlights
Changelog
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
|
🔍 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: Wed Feb 11 14:03:41 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
![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 helpful mechanism to stash a dirty working tree before an auto-release and restore it afterward, preventing release failures on uncommitted changes. The implementation is mostly correct, but there are a few issues related to error handling and adherence to the repository's shell scripting style guide. Specifically, 2>/dev/null is used in several places, which violates the style guide and in one case can hide a critical failure. My review provides suggestions to fix these issues by improving error logging and handling.
| log_warn "Fast-forward pull failed, trying rebase..." | ||
| git -C "$repo" pull --rebase origin main 2>/dev/null || { | ||
| log_error "Failed to pull latest main for release" | ||
| [[ "$stashed" == "true" ]] && git -C "$repo" stash pop 2>/dev/null || true |
There was a problem hiding this comment.
The use of || true here swallows the exit code from git stash pop, which means a failure to restore the stashed changes will go unnoticed. This could lead to lost work if the user isn't aware their changes weren't restored. Additionally, using 2>/dev/null violates the repository style guide (rule 50). It's better to handle the error explicitly with a warning message and redirect stderr to the log file.
| [[ "$stashed" == "true" ]] && git -C "$repo" stash pop 2>/dev/null || true | |
| [[ "$stashed" == "true" ]] && git -C "$repo" stash pop 2>>"$SUPERVISOR_LOG" || log_warn "Failed to restore stashed changes on pull failure. Manual recovery may be needed." |
References
- Rule 50 states that
2>/dev/nullis acceptable only when redirecting to log files, not for blanket suppression. The code uses2>/dev/nullto suppress stderr without redirecting to a log. (link)
| if [[ -n "$(git -C "$repo" status --porcelain 2>/dev/null)" ]]; then | ||
| log_info "Stashing dirty working tree before release..." | ||
| if git -C "$repo" stash push -m "auto-release-stash-$(date +%Y%m%d%H%M%S)" 2>/dev/null; then |
There was a problem hiding this comment.
According to the repository style guide (rule 50), 2>/dev/null should only be used when redirecting to a log file. These lines suppress stderr from git status and git stash push, which can hide important error information. To improve observability and adhere to the style guide, please redirect stderr to the $SUPERVISOR_LOG file.
| if [[ -n "$(git -C "$repo" status --porcelain 2>/dev/null)" ]]; then | |
| log_info "Stashing dirty working tree before release..." | |
| if git -C "$repo" stash push -m "auto-release-stash-$(date +%Y%m%d%H%M%S)" 2>/dev/null; then | |
| if [[ -n "$(git -C "$repo" status --porcelain 2>>"$SUPERVISOR_LOG")" ]]; then | |
| log_info "Stashing dirty working tree before release..." | |
| if git -C "$repo" stash push -m "auto-release-stash-$(date +%Y%m%d%H%M%S)" &>>"$SUPERVISOR_LOG"; then |
References
- Rule 50 states that
2>/dev/nullis acceptable only when redirecting to log files, not for blanket suppression. The code uses2>/dev/nullto suppress stderr without redirecting to a log. (link)
| # t276: Restore stashed changes after release (regardless of success/failure) | ||
| if [[ "$stashed" == "true" ]]; then | ||
| log_info "Restoring stashed working tree..." | ||
| git -C "$repo" stash pop 2>/dev/null || log_warn "git stash pop failed (may need manual recovery)" |
There was a problem hiding this comment.
This line violates the repository style guide (rule 50) by using 2>/dev/null to suppress stderr. While the error is handled by the || log_warn clause, the style guide is strict about redirecting stderr to a log file for better observability.
| git -C "$repo" stash pop 2>/dev/null || log_warn "git stash pop failed (may need manual recovery)" | |
| git -C "$repo" stash pop 2>>"$SUPERVISOR_LOG" || log_warn "git stash pop failed (may need manual recovery)" |
References
- Rule 50 states that
2>/dev/nullis acceptable only when redirecting to log files, not for blanket suppression. The code uses2>/dev/nullto suppress stderr without redirecting to a log. (link)
|
Caution Review failedThe pull request is closed. WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Poem
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
1 participant
Summary
trigger_batch_release()fails whenversion-manager.shencounters uncommitted changes (e.g.todo/VERIFY.mdfrom parallel sessions)git pull+ release, restores after regardless of outcomeTask
t276
Testing
bash -nsyntax check: passshellcheck -S warning: no new warnings (only pre-existing SC2034)Summary by CodeRabbit