fix: prevent aidevops update from dirtying git working tree#2287
fix: prevent aidevops update from dirtying git working tree#2287marcusquinn merged 1 commit intomainfrom
Conversation
Root cause: set_permissions() in setup-modules/core.sh used relative paths that resolved to the git repo when called from cmd_update(), changing file modes on tracked files. update_scan_results_log() in security-helper.sh used git rev-parse --show-toplevel to write scan results into the repo. Fix (3 parts): - set_permissions(): target deployed dir (~/.aidevops/agents/scripts/) instead of repo-relative .agents/scripts/ - update_scan_results_log(): write to deployed copy at ~/.aidevops/agents/SKILL-SCAN-RESULTS.md instead of repo working tree - cmd_update(): add pre-pull cleanup of stale changes (fixes existing broken installs) and post-setup safety net (git checkout -- .)
|
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. 📒 Files selected for processing (3)
✨ 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 |
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 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
|
🔍 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 25 15:21:57 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
🤖 Augment PR SummarySummary: Prevents Changes:
Technical Notes: Aims to keep updates compatible with 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| # SCAN_RESULTS_FILE is ".agents/SKILL-SCAN-RESULTS.md" (repo-relative); | ||
| # strip the ".agents/" prefix to get the deployed path | ||
| local results_filename="${SCAN_RESULTS_FILE#.agents/}" | ||
| local results_file="${deployed_dir}/${results_filename}" |
There was a problem hiding this comment.
update_scan_results_log() now writes to results_file under ~/.aidevops/agents, but the success message later still prints ${SCAN_RESULTS_FILE} (repo-relative), which may mislead users when they try to locate the log output.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| # See: https://github.com/marcusquinn/aidevops/issues/2286 | ||
| if ! git diff --quiet 2>/dev/null; then | ||
| print_info "Cleaning up stale working tree changes..." | ||
| git checkout -- . 2>/dev/null || true |
There was a problem hiding this comment.
This git checkout -- . will discard any local working tree edits in $INSTALL_DIR, not just the stale chmod/scan-log changes from prior updates; that’s potentially surprising data loss if someone has custom local modifications.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| # (e.g. chmod on tracked scripts, scan results written to repo) | ||
| # This ensures git pull --ff-only won't be blocked. | ||
| # See: https://github.com/marcusquinn/aidevops/issues/2286 | ||
| if ! git diff --quiet 2>/dev/null; then |
There was a problem hiding this comment.
git diff --quiet only checks unstaged working tree changes; staged/index changes could still block git pull --ff-only, so the “won’t be blocked” comment here may not hold in that case.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
![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 resolves the issue where aidevops update would dirty the git working tree, preventing future updates. The changes correctly retarget file operations to the deployed application directory instead of the source repository. The addition of cleanup steps in cmd_update to handle previously broken installations is a thoughtful touch. My review focuses on improving error handling by removing unnecessary stderr suppression, which aligns with the repository's general scripting guidelines and will make the scripts more robust and easier to debug.
| local deployed_dir="$HOME/.aidevops/agents" | ||
| # SCAN_RESULTS_FILE is ".agents/SKILL-SCAN-RESULTS.md" (repo-relative); | ||
| # strip the ".agents/" prefix to get the deployed path | ||
| local results_filename="${SCAN_RESULTS_FILE#.agents/}" |
There was a problem hiding this comment.
The parameter expansion ${SCAN_RESULTS_FILE#.agents/} is a clean way to strip the prefix. However, for improved readability and to make the intent clearer, using basename might be a better choice, especially if the structure of SCAN_RESULTS_FILE were to change in the future.
| local results_filename="${SCAN_RESULTS_FILE#.agents/}" | |
| local results_filename="$(basename "$SCAN_RESULTS_FILE")" |
| # (e.g. chmod on tracked scripts, scan results written to repo) | ||
| # This ensures git pull --ff-only won't be blocked. | ||
| # See: https://github.com/marcusquinn/aidevops/issues/2286 | ||
| if ! git diff --quiet 2>/dev/null; then |
There was a problem hiding this comment.
Using 2>/dev/null here suppresses potentially useful error messages from git. The git diff --quiet command is already silent on success, so this redirection only hides problems. Removing it will make debugging easier if something unexpected occurs, which is in line with the project's scripting guidelines.
| if ! git diff --quiet 2>/dev/null; then | |
| if ! git diff --quiet; then |
References
- Avoid using '2>/dev/null' for blanket suppression of command errors in shell scripts to ensure that authentication, syntax, or system issues remain visible for debugging.
| # See: https://github.com/marcusquinn/aidevops/issues/2286 | ||
| if ! git diff --quiet 2>/dev/null; then | ||
| print_info "Cleaning up stale working tree changes..." | ||
| git checkout -- . 2>/dev/null || true |
There was a problem hiding this comment.
Blanket suppression of stderr with 2>/dev/null is discouraged as it can hide underlying issues, such as file permission problems during checkout. Since git checkout -- . is silent on success, there's no need to redirect its output. The || true is sufficient to handle expected failures. Removing the redirection aligns with project best practices for error visibility.
| git checkout -- . 2>/dev/null || true | |
| git checkout -- . || true |
References
- Avoid using '2>/dev/null' for blanket suppression of command errors in shell scripts to ensure that authentication, syntax, or system issues remain visible for debugging.
| # Safety net: discard any working tree changes setup.sh may have introduced | ||
| # (e.g. chmod on tracked scripts, scan results written to repo) | ||
| # See: https://github.com/marcusquinn/aidevops/issues/2286 | ||
| git checkout -- . 2>/dev/null || true |
There was a problem hiding this comment.
This git checkout command also unnecessarily suppresses stderr. For better error visibility and easier debugging, it's recommended to remove 2>/dev/null. The || true will still prevent the script from exiting on failure. This change aligns with the project's general rule to avoid hiding potential errors.
| git checkout -- . 2>/dev/null || true | |
| git checkout -- . || true |
References
- Avoid using '2>/dev/null' for blanket suppression of command errors in shell scripts to ensure that authentication, syntax, or system issues remain visible for debugging.
| # Safety net: discard any working tree changes setup.sh may have introduced | ||
| # (e.g. chmod on tracked scripts, scan results written to repo) | ||
| # See: https://github.com/marcusquinn/aidevops/issues/2286 | ||
| git checkout -- . 2>/dev/null || true |
There was a problem hiding this comment.
To maintain consistency and improve debuggability, please remove the stderr suppression (2>/dev/null) from this git checkout command as well. This will allow any unexpected errors to be reported, following the project's guideline on error handling.
| git checkout -- . 2>/dev/null || true | |
| git checkout -- . || true |
References
- Avoid using '2>/dev/null' for blanket suppression of command errors in shell scripts to ensure that authentication, syntax, or system issues remain visible for debugging.
| chmod +x "$deployed_dir/scripts/"*.sh 2>/dev/null || true | ||
| # Also handle modularised subdirectories (e.g. memory/, supervisor-modules/) | ||
| find "$deployed_dir/scripts" -mindepth 2 -name "*.sh" -exec chmod +x {} + 2>/dev/null || true | ||
| fi | ||
|
|
||
| # Secure configuration files (these are in the user's config dir, not the repo) | ||
| chmod 600 "$HOME/.config/aidevops/"*.json 2>/dev/null || true | ||
| # Also secure repo-local configs if present (for interactive setup from repo root) | ||
| chmod 600 configs/*.json 2>/dev/null || true |
There was a problem hiding this comment.
Throughout this function, 2>/dev/null is used to suppress errors from chmod and find. While this handles cases where glob patterns don't match any files, it also hides potentially critical errors like permission issues. It's better to remove 2>/dev/null to improve debuggability. The || true is sufficient to prevent the script from exiting on non-fatal errors (like no files matching a glob). This change would align with the project's general rules promoting error visibility.
| chmod +x "$deployed_dir/scripts/"*.sh 2>/dev/null || true | |
| # Also handle modularised subdirectories (e.g. memory/, supervisor-modules/) | |
| find "$deployed_dir/scripts" -mindepth 2 -name "*.sh" -exec chmod +x {} + 2>/dev/null || true | |
| fi | |
| # Secure configuration files (these are in the user's config dir, not the repo) | |
| chmod 600 "$HOME/.config/aidevops/"*.json 2>/dev/null || true | |
| # Also secure repo-local configs if present (for interactive setup from repo root) | |
| chmod 600 configs/*.json 2>/dev/null || true | |
| chmod +x "$deployed_dir/scripts/"*.sh || true | |
| # Also handle modularised subdirectories (e.g. memory/, supervisor-modules/) | |
| find "$deployed_dir/scripts" -mindepth 2 -name "*.sh" -exec chmod +x {} + || true | |
| fi | |
| # Secure configuration files (these are in the user's config dir, not the repo) | |
| chmod 600 "$HOME/.config/aidevops/"*.json || true | |
| # Also secure repo-local configs if present (for interactive setup from repo root) | |
| chmod 600 configs/*.json || true |
References
- Avoid using '2>/dev/null' to 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. - 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.
… detached HEAD Fixes #2288 — aidevops update stuck on old version (e.g. 2.125 while 2.128.3 available). Root cause: git pull --ff-only fails when the working tree is dirty or local history has diverged, with no recovery path. Users on pre-PR#2287 versions could not get the dirty-tree fix because the fix itself required a successful pull. Three changes: 1. Ensure local repo is on main branch before pulling (handles detached HEAD) 2. Clean both staged and unstaged changes before pull (extends PR #2287 fix) 3. Fall back to git reset --hard origin/main when --ff-only fails, instead of returning an error — the install repo should always mirror origin/main exactly
… detached HEAD (#2289) Fixes #2288 — aidevops update stuck on old version (e.g. 2.125 while 2.128.3 available). Root cause: git pull --ff-only fails when the working tree is dirty or local history has diverged, with no recovery path. Users on pre-PR#2287 versions could not get the dirty-tree fix because the fix itself required a successful pull. Three changes: 1. Ensure local repo is on main branch before pulling (handles detached HEAD) 2. Clean both staged and unstaged changes before pull (extends PR #2287 fix) 3. Fall back to git reset --hard origin/main when --ff-only fails, instead of returning an error — the install repo should always mirror origin/main exactly
…manence trap Adds _is_bot_generated_cleanup_issue() helper that detects issues created by post-merge-review-scanner.sh (labels: review-followup or source:review- scanner). Extracts the NMR approval gate from _dispatch_dedup_check_layers into _check_nmr_approval_gate() to keep the parent function under the 100-line complexity threshold. When the gate evaluates, if: (a) the issue is bot-generated cleanup AND (b) the needs-maintainer-review label is NOT currently present, then the historical timeline-based ever-NMR check is skipped, allowing dispatch to proceed. The exemption does NOT fire when NMR is currently present — the active case preserves t1894's security posture (maintainer-gated issues still require cryptographic approval). The exemption surgically unblocks the drift case where the fast-fail escalation path applied NMR, the maintainer subsequently removed it, and the historical timeline event keeps the issue permanently blocked. Live evidence from the 2026-04-13 awardsapp queue drain: #2294, #2293, #2292, #2290, #2287 all stuck on ever-NMR with current labels: [auto-dispatch, origin:worker, origin:interactive, review- followup, source:review-scanner]. No current NMR label. Fix 3a immediately unblocks them on the next pulse cycle. 9 new unit tests cover: review-followup detection, source:review-scanner detection, both labels together, regular issue rejection, empty labels array, empty meta_json, invalid JSON, partial-name rejection (avoid 'review-followup-queued' false positive), and the exact awardsapp#2294 regression scenario. Function lengths after refactor: _dispatch_dedup_check_layers: 77 lines (was 83 pre-Fix-3, 103 post-Fix-3 before _check_nmr_approval_gate extraction) _check_nmr_approval_gate: 23 lines (new) _is_bot_generated_cleanup_issue: 6 lines (new) Scope: intentionally surgical. Does NOT change timeline API semantics, remove NMR from scanner output (GH#18538), or touch the origin:worker vs origin:interactive double-labeling (separate bug). Fixes #18648
…manence trap (#18649) Adds _is_bot_generated_cleanup_issue() helper that detects issues created by post-merge-review-scanner.sh (labels: review-followup or source:review- scanner). Extracts the NMR approval gate from _dispatch_dedup_check_layers into _check_nmr_approval_gate() to keep the parent function under the 100-line complexity threshold. When the gate evaluates, if: (a) the issue is bot-generated cleanup AND (b) the needs-maintainer-review label is NOT currently present, then the historical timeline-based ever-NMR check is skipped, allowing dispatch to proceed. The exemption does NOT fire when NMR is currently present — the active case preserves t1894's security posture (maintainer-gated issues still require cryptographic approval). The exemption surgically unblocks the drift case where the fast-fail escalation path applied NMR, the maintainer subsequently removed it, and the historical timeline event keeps the issue permanently blocked. Live evidence from the 2026-04-13 awardsapp queue drain: #2294, #2293, #2292, #2290, #2287 all stuck on ever-NMR with current labels: [auto-dispatch, origin:worker, origin:interactive, review- followup, source:review-scanner]. No current NMR label. Fix 3a immediately unblocks them on the next pulse cycle. 9 new unit tests cover: review-followup detection, source:review-scanner detection, both labels together, regular issue rejection, empty labels array, empty meta_json, invalid JSON, partial-name rejection (avoid 'review-followup-queued' false positive), and the exact awardsapp#2294 regression scenario. Function lengths after refactor: _dispatch_dedup_check_layers: 77 lines (was 83 pre-Fix-3, 103 post-Fix-3 before _check_nmr_approval_gate extraction) _check_nmr_approval_gate: 23 lines (new) _is_bot_generated_cleanup_issue: 6 lines (new) Scope: intentionally surgical. Does NOT change timeline API semantics, remove NMR from scanner output (GH#18538), or touch the origin:worker vs origin:interactive double-labeling (separate bug). Fixes #18648
1 participant
Summary
Fixes #2286 —
aidevops updateleft uncommitted changes in the git working tree, causing subsequent updates to fail with merge conflicts.Root Cause
Two functions modified tracked files in the git repo during
setup.sh --non-interactive(called bycmd_update()):set_permissions()(setup-modules/core.sh:539) — used relative paths (.agents/scripts/*.sh) that resolved to the git repo, changing file modes on tracked filesupdate_scan_results_log()(security-helper.sh:394) — usedgit rev-parse --show-toplevelto find the repo root, then wrote timestamps and scan history to.agents/SKILL-SCAN-RESULTS.mdin the working treeFix (3 parts)
set_permissions(): Target the deployed directory (~/.aidevops/agents/scripts/) instead of repo-relative.agents/scripts/update_scan_results_log(): Write to the deployed copy at~/.aidevops/agents/SKILL-SCAN-RESULTS.mdinstead of the git working treecmd_update(): Add pre-pull cleanup of stale changes (fixes existing broken installs) and post-setup safety net (git checkout -- .)Testing
test-smoke-help.shbash -n