t1031: Restore missing supervisor globals + add globals test#1361
t1031: Restore missing supervisor globals + add globals test#1361marcusquinn merged 2 commits intomainfrom
Conversation
…IR) dropped during t1031 modularization Add test-supervisor-globals.sh to catch unbound variables in modules. Tests: source with set -u, syntax check all modules, verify key globals defined, cross-reference module variable usage against definitions.
|
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. ✨ 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 a critical 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: Fri Feb 13 14:20:15 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 restores the missing global variables in supervisor-helper.sh, fixing the unbound variable errors. The addition of test-supervisor-globals.sh is an excellent step towards preventing similar issues in the future. My review focuses on improving the robustness of the new test script and ensuring consistency in the main script, in line with the repository's style guide.
| "VALID_TRANSITIONS" | ||
| ) | ||
|
|
||
| all_files="$SUPERVISOR $(echo "$REPO_DIR/.agents/scripts/supervisor/"*.sh)" |
There was a problem hiding this comment.
Constructing the file list as a space-separated string is not robust. It is subject to word-splitting (hence the shellcheck disable on line 76) and will behave incorrectly if the glob pattern .../*.sh matches no files. Using a bash array is a safer and more robust way to handle lists of files. This would require updating the grep calls on lines 77 and 95 to use array expansion ("${all_files[@]}").
| if grep -rq "\$${var}\b\|\${${var}}" "$module_dir/" 2>/dev/null; then | ||
| # Check if defined in monolith or _common.sh | ||
| # shellcheck disable=SC2086 | ||
| if ! grep -q "^[[:space:]]*\(readonly \)\{0,1\}${var}=" $all_files 2>/dev/null; then |
There was a problem hiding this comment.
The regex in this grep command is incorrect and inconsistent with the one used in Test 3 (line 77). It only checks for definitions like readonly VAR= and misses plain assignments (VAR=) or array assignments (readonly -a VAR=). To correctly detect if a variable is defined, you should use grep -E with the same robust pattern from Test 3.
| if ! grep -q "^[[:space:]]*\(readonly \)\{0,1\}${var}=" $all_files 2>/dev/null; then | |
| if ! grep -qE "^[[:space:]]*(readonly( -a)? )?${var}=" $all_files 2>/dev/null; then |
| SUPERVISOR_LOG_DIR="${HOME}/.aidevops/logs" | ||
| mkdir -p "$SUPERVISOR_LOG_DIR" 2>/dev/null || true | ||
| SUPERVISOR_LOG="${SUPERVISOR_LOG_DIR}/supervisor.log" |
There was a problem hiding this comment.
The new variables SUPERVISOR_LOG_DIR (line 199) and SUPERVISOR_LOG (line 201) are not declared readonly. This is inconsistent with other constants in this file (e.g., PULSE_LOCK_DIR) and the convention for UPPER_SNAKE variables. They should be made readonly to prevent accidental modification.
References
- Variables named in
UPPER_SNAKEare for constants/env vars. Constants should be immutable. (link)
| readonly SESSION_CHECKPOINT_HELPER="${SCRIPT_DIR}/session-checkpoint-helper.sh" # Used by respawn (t264.1) | ||
| readonly RESPAWN_LOG="${HOME}/.aidevops/logs/respawn-history.log" # Persistent respawn log (t264.1) | ||
| SUPERVISOR_LOG_DIR="${HOME}/.aidevops/logs" | ||
| mkdir -p "$SUPERVISOR_LOG_DIR" 2>/dev/null || true |
There was a problem hiding this comment.
The use of 2>/dev/null here to suppress errors from mkdir -p is not ideal. According to the repository style guide, 2>/dev/null should only be used for redirection to log files, not for blanket suppression. mkdir -p is idempotent and won't fail if the directory already exists. Suppressing stderr can hide important permission or filesystem errors. The || true guard is sufficient to prevent the script from exiting on failure.
| mkdir -p "$SUPERVISOR_LOG_DIR" 2>/dev/null || true | |
| mkdir -p "$SUPERVISOR_LOG_DIR" || true |
References
- The style guide states that
2>/dev/nullis acceptable ONLY when redirecting to log files, not for blanket suppression of command output. (link)
| pass() { | ||
| PASS=$((PASS + 1)) | ||
| echo " PASS: $1" | ||
| } | ||
| fail() { | ||
| FAIL=$((FAIL + 1)) | ||
| echo " FAIL: $1" | ||
| } |
There was a problem hiding this comment.
These helper functions are missing explicit return statements, which is required by the repository style guide.
| pass() { | |
| PASS=$((PASS + 1)) | |
| echo " PASS: $1" | |
| } | |
| fail() { | |
| FAIL=$((FAIL + 1)) | |
| echo " FAIL: $1" | |
| } | |
| pass() { | |
| PASS=$((PASS + 1)) | |
| echo " PASS: $1" | |
| return 0 | |
| } | |
| fail() { | |
| FAIL=$((FAIL + 1)) | |
| echo " FAIL: $1" | |
| return 0 | |
| } |
References
- All functions must have explicit
returnstatements. (link)
1 participant
Summary
SUPERVISOR_LOG,SUPERVISOR_LOG_DIR,PULSE_LOCK_DIR, andPULSE_LOCK_TIMEOUTglobals that were dropped during t1031 modularization (PR t1031: Modularize supervisor-helper.sh into domain modules #1359)unbound variableerrors when runningsupervisor-helper.sh pulsetests/test-supervisor-globals.sh— a permanent guard that catches this class of bug:set -u(catches unbound variables)This is the permanent solution: any future modularization that drops a global will be caught by the test before merge.
Fixes the supervisor pulse failure introduced by #1359.