t1084: Fix auto-update-helper.sh CodeRabbit feedback — input validation, help text, pgrep pattern#1597
Conversation
Three issues from PR #1591 review: 1. Validate AIDEVOPS_SKILL_FRESHNESS_HOURS is a positive integer before arithmetic expansion — non-numeric value crashes under set -e; falls back to DEFAULT_SKILL_FRESHNESS_HOURS with a warning log entry. 2. Fix help text: document 'skill-update-helper.sh check --auto-update --quiet' to match the actual runtime invocation (was missing 'check'). 3. Tighten pgrep -f pattern in is_update_running() from generic 'setup\.sh' to '${INSTALL_DIR}/setup\.sh' to avoid false matches from unrelated projects' setup scripts. ShellCheck zero violations, bash -n syntax OK.
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 several critical issues identified during a CodeRabbit review, significantly enhancing the robustness and accuracy of the 'auto-update-helper.sh' script. The changes prevent potential script failures due to invalid user input, correct misleading documentation, and improve the reliability of process detection to avoid erroneous update skips. 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
|
WalkthroughThis PR refines the auto-update-helper.sh script with targeted improvements: stricter process name matching using full paths to reduce false positives, numeric validation for the AIDEVOPS_SKILL_FRESHNESS_HOURS environment variable with safe fallback behavior, and synchronization of help documentation with the actual invocation pattern. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related issues
Possibly related PRs
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: Wed Feb 18 00:30:42 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 effectively addresses three issues identified in a previous review, enhancing the robustness and correctness of auto-update-helper.sh. The changes include tightening a pgrep pattern for more specific matching, adding input validation to prevent crashes from non-numeric input, and correcting the help text to match the actual command invocation. These are all valuable fixes. I have one suggestion to further improve the robustness of the pgrep pattern matching for future-proofing.
| # Use full path to avoid matching unrelated projects' setup.sh scripts | ||
| if pgrep -f "${INSTALL_DIR}/setup\.sh" >/dev/null 2>&1; then |
There was a problem hiding this comment.
For improved robustness, it's a good practice to escape the INSTALL_DIR variable to handle any potential regex metacharacters it might contain. While it's unlikely to be an issue with the current hardcoded path, this would prevent pgrep from misinterpreting the pattern if the path were to change to include characters like . or * in a username.
| # Use full path to avoid matching unrelated projects' setup.sh scripts | |
| if pgrep -f "${INSTALL_DIR}/setup\.sh" >/dev/null 2>&1; then | |
| # Use full path to avoid matching unrelated projects' setup.sh scripts | |
| # Escape INSTALL_DIR for safe use in regex | |
| local escaped_install_dir | |
| escaped_install_dir=$(sed 's/[][.^*+?(){}|$\\]/\\&/g' <<< "$INSTALL_DIR") | |
| if pgrep -f "${escaped_install_dir}/setup\.sh" >/dev/null 2>&1; then |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.agents/scripts/auto-update-helper.sh (1)
7-8:⚠️ Potential issue | 🟡 MinorStale header comment — missing
checksubcommand.The PR correctly updated the
cmd_helptext (line 627) and theHOW IT WORKSblock, but the top-of-file header comment still shows the old invocation without thechecksubcommand, out of sync with the actual call at line 296.📝 Proposed fix
-# Also runs a daily skill freshness check: calls skill-update-helper.sh -# --auto-update --quiet to pull upstream changes for all imported skills. +# Also runs a daily skill freshness check: calls skill-update-helper.sh check +# --auto-update --quiet to pull upstream changes for all imported skills.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/auto-update-helper.sh around lines 7 - 8, Update the top-of-file header comment to reflect the actual invocation used later: include the `check` subcommand in the example that calls `skill-update-helper.sh` (matching the `cmd_help` text and the updated HOW IT WORKS block). Locate the header comment near the top of the script and change the line that currently documents the invocation to include `check` (e.g., `skill-update-helper.sh --auto-update --quiet check`) so it stays consistent with the runtime call referenced in the script.
🧹 Nitpick comments (1)
.agents/scripts/auto-update-helper.sh (1)
172-173: pgrep precision improvement looks good — optional: escapeINSTALL_DIRin ERE context.The narrowed pattern eliminates false positives from unrelated
setup.shprocesses. One low-risk edge case:${INSTALL_DIR}is expanded verbatim into a POSIX ERE, so any regex metacharacters in$HOME(e.g., a.in a username likejohn.doe) would be treated as "match any character" rather than literals. In practice the path structure makes this a non-issue, but for strict correctness you could pre-escape it:♻️ Proposed optional hardening
+ local escaped_dir + escaped_dir=$(printf '%s' "${INSTALL_DIR}" | sed 's/[.[\*^$]/\\&/g') - if pgrep -f "${INSTALL_DIR}/setup\.sh" >/dev/null 2>&1; then + if pgrep -f "${escaped_dir}/setup\.sh" >/dev/null 2>&1; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/auto-update-helper.sh around lines 172 - 173, The pgrep pattern uses ${INSTALL_DIR} directly which can introduce unintended ERE metacharacter matches (e.g., dots in usernames); create an escaped version of INSTALL_DIR (e.g., escaped_install_dir) by backslash-escaping regex metacharacters (such as .[]^$*\/(){} etc.) and then use that escaped variable in the pgrep -f pattern (e.g., pgrep -f "${escaped_install_dir}/setup\.sh"); alternatively, build the check using a fixed-string search approach (escape or use ps + grep -F) instead of embedding raw INSTALL_DIR into an ERE.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.agents/scripts/auto-update-helper.sh:
- Around line 7-8: Update the top-of-file header comment to reflect the actual
invocation used later: include the `check` subcommand in the example that calls
`skill-update-helper.sh` (matching the `cmd_help` text and the updated HOW IT
WORKS block). Locate the header comment near the top of the script and change
the line that currently documents the invocation to include `check` (e.g.,
`skill-update-helper.sh --auto-update --quiet check`) so it stays consistent
with the runtime call referenced in the script.
---
Nitpick comments:
In @.agents/scripts/auto-update-helper.sh:
- Around line 172-173: The pgrep pattern uses ${INSTALL_DIR} directly which can
introduce unintended ERE metacharacter matches (e.g., dots in usernames); create
an escaped version of INSTALL_DIR (e.g., escaped_install_dir) by
backslash-escaping regex metacharacters (such as .[]^$*\/(){} etc.) and then use
that escaped variable in the pgrep -f pattern (e.g., pgrep -f
"${escaped_install_dir}/setup\.sh"); alternatively, build the check using a
fixed-string search approach (escape or use ps + grep -F) instead of embedding
raw INSTALL_DIR into an ERE.
1 participant
Summary
Addresses 3 issues raised by CodeRabbit in PR #1591 review.
Fix 1: Input validation for AIDEVOPS_SKILL_FRESHNESS_HOURS (crash under set -e)
Non-numeric value in AIDEVOPS_SKILL_FRESHNESS_HOURS caused a bash arithmetic error that crashes the script under set -e. Added regex validation before the arithmetic expansion; falls back to DEFAULT_SKILL_FRESHNESS_HOURS with a log_warn entry.
Fix 2: Help text / invocation mismatch
Help text documented 'skill-update-helper.sh --auto-update --quiet' but the actual runtime call was 'skill-update-helper.sh check --auto-update --quiet'. Updated help text to include the check subcommand, matching the real invocation.
Fix 3: pgrep -f pattern too broad
pgrep -f 'setup.sh' could match unrelated projects' setup.sh scripts, causing a false-positive 'update already running' skip. Tightened to pgrep -f '${INSTALL_DIR}/setup.sh' to scope the match to the aidevops install directory.
Testing
bash -n .agents/scripts/auto-update-helper.sh # Syntax OK
shellcheck -x -S warning .agents/scripts/auto-update-helper.sh # Zero violations
Ref #1596
Summary by CodeRabbit
Bug Fixes
Documentation