Skip to content

Comments

t1034: Fix PULSE_LOCK_DIR unbound variable — t1031 modularisation regression#1373

Merged
marcusquinn merged 1 commit intomainfrom
hotfix/pulse-lock-dir-unbound
Feb 13, 2026
Merged

t1034: Fix PULSE_LOCK_DIR unbound variable — t1031 modularisation regression#1373
marcusquinn merged 1 commit intomainfrom
hotfix/pulse-lock-dir-unbound

Conversation

@marcusquinn
Copy link
Owner

@marcusquinn marcusquinn commented Feb 13, 2026

Summary

  • Moves all supervisor globals (SUPERVISOR_DIR, PULSE_LOCK_DIR, SUPERVISOR_DB, etc.) before the module source block in supervisor-helper.sh
  • The t1031 modularisation left these defined after modules were sourced, causing PULSE_LOCK_DIR: unbound variable errors under set -euo pipefail
  • This broke the cron pulse — no tasks have been dispatched since t1031 merged

Root Cause

supervisor-helper.sh has set -euo pipefail. The module source block was at lines 170-187, but globals like PULSE_LOCK_DIR were defined at line 202. When release_pulse_lock() (in utility.sh) was called via EXIT trap in subshells, the variable wasn't in scope, producing the unbound variable error.

Fix

Move the entire globals block (lines 189-205) to before the module source block (now lines 167-188). No functional change — just reordering declarations.

Verification

  • bash -n syntax check passes
  • shellcheck -S error clean
  • supervisor-helper.sh help loads correctly
  • Live-deployed to ~/.aidevops/agents/scripts/ — next cron pulse will verify

Closes #1372

Summary by CodeRabbit

  • Chores
    • Improved initialization order and variable scoping in internal configuration systems to enhance stability and prevent potential runtime errors.

…ion)

The t1031 modularisation left PULSE_LOCK_DIR, SUPERVISOR_DIR, and other
globals defined AFTER the module source block. With set -euo pipefail,
any function or trap referencing these variables hit 'unbound variable'
errors. This broke the cron pulse — no tasks were dispatched since t1031
merged. Fix: define all globals before sourcing modules.
@gemini-code-assist
Copy link

Summary of Changes

Hello @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 regression that emerged from a previous modularization effort. The issue caused cron pulses to fail due to an 'unbound variable' error, effectively halting task dispatch. The fix involves a strategic reordering of global variable declarations within the main supervisor script, ensuring that all essential variables are properly defined and accessible before any modules that rely on them are sourced. This change restores the stability and functionality of the cron pulse mechanism.

Highlights

  • Critical Regression Fix: Resolved a regression introduced by t1031 modularization that caused PULSE_LOCK_DIR: unbound variable errors, leading to cron pulse failures and preventing task dispatch.
  • Global Variable Reordering: Moved all supervisor global variable definitions (e.g., SUPERVISOR_DIR, PULSE_LOCK_DIR) to occur before the module source block in supervisor-helper.sh.
  • Enhanced Robustness: Ensured that all necessary global variables are defined and in scope when modules are sourced, preventing set -u (unbound variable) errors in subshells and traps.
Changelog
  • .agents/scripts/supervisor-helper.sh
    • Reordered the definition and export of supervisor global variables to occur before the sourcing of supervisor modules.
    • Added readonly attribute to SCRIPT_DIR.
Activity
  • The pull request author identified the root cause as global variables being defined after modules were sourced, leading to unbound variable errors under set -euo pipefail.
  • The fix involved moving the entire globals block to before the module source block.
  • Verification steps included bash -n syntax check, shellcheck -S error clean, and successful loading of supervisor-helper.sh help.
  • The change was live-deployed to ~/.aidevops/agents/scripts/ for real-world verification with the next cron pulse.
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@github-actions
Copy link

🔍 Code Quality Report

�[0;35m[MONITOR]�[0m Code Review Monitoring Report

�[0;34m[INFO]�[0m Latest Quality Status:
SonarCloud: 0 bugs, 0 vulnerabilities, 19 code smells

�[0;34m[INFO]�[0m Recent monitoring activity:
Fri Feb 13 14:38:35 UTC 2026: Code review monitoring started
Fri Feb 13 14:38:36 UTC 2026: SonarCloud - Bugs: 0, Vulnerabilities: 0, Code Smells: 19

📈 Current Quality Metrics

  • BUGS: 0
  • CODE SMELLS: 19
  • VULNERABILITIES: 0

Generated on: Fri Feb 13 14:38:38 UTC 2026


Generated by AI DevOps Framework Code Review Monitoring

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 13, 2026

Walkthrough

Reorganized global variable declarations in a shell script to initialize and export them before sourcing modules, rather than after. This prevents unbound variable errors in strict mode by ensuring dependencies are available when modules are loaded.

Changes

Cohort / File(s) Summary
Global Variable Initialization
.agents/scripts/supervisor-helper.sh
Relocated SCRIPT_DIR, SUPERVISOR_DIR, SUPERVISOR_DB, and helper path variables (MAIL_HELPER, MEMORY_HELPER, etc.) from post-sourcing declarations to pre-sourcing declarations. Made SUPERVISOR_LOG_DIR and SUPERVISOR_LOG explicitly exported. Removed duplicate declarations that appeared after module sourcing. This reordering ensures all globals are available in strict mode before modules reference them, fixing the PULSE_LOCK_DIR unbound variable regression from modularization.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

  • PR#1061: Modifies the same supervisor-helper.sh file and references $SUPERVISOR_LOG usage—this PR's reordering of supervisor globals and their export directly supports that PR's error-logging patterns.

Poem

🔧 A variable dance, where order is key—
Before the modules, let globals run free!
No more unbound in strict mode's embrace,
Initialization finds its proper place. ✨

🚥 Pre-merge checks | ✅ 5 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Merge Conflict Detection ⚠️ Warning ❌ Merge conflicts detected (2 files):

⚔️ .agents/scripts/supervisor-helper.sh (content)
⚔️ TODO.md (content)

These conflicts must be resolved before merging into main.
Resolve conflicts locally and push changes to this branch.
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the primary change: reordering global variable declarations to fix PULSE_LOCK_DIR unbound variable errors caused by t1031 modularisation.
Linked Issues check ✅ Passed The PR directly addresses the coding objective from #1372: restoring correct ordering of global variable declarations before module sourcing to prevent unbound variable errors under strict mode.
Out of Scope Changes check ✅ Passed All changes are scoped to supervisor-helper.sh global variable declaration reordering; no unrelated modifications or scope creep detected.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch hotfix/pulse-lock-dir-unbound
⚔️ Resolve merge conflicts (beta)
  • Auto-commit resolved conflicts to branch hotfix/pulse-lock-dir-unbound
  • Create stacked PR with resolved conflicts
  • Post resolved changes as copyable diffs in a comment

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments
.agents/scripts/supervisor-helper.sh (1)

169-189: Correct fix — globals hoisted above module source block, resolving the set -u unbound variable regression.

The root cause analysis in the comment block is thorough and the reorder is precisely what's needed. With set -euo pipefail, any EXIT trap referencing PULSE_LOCK_DIR before it was declared would be fatal — this eliminates that.

One minor observation: SUPERVISOR_LOG_DIR (Line 183) and SUPERVISOR_LOG (Line 185) are the only two path variables in this block that aren't readonly, while all others are. If nothing downstream reassigns them, consider making them readonly after the mkdir -p for consistency.

Optional: make log paths readonly for consistency
 SUPERVISOR_LOG_DIR="${HOME}/.aidevops/logs"
 mkdir -p "$SUPERVISOR_LOG_DIR" 2>/dev/null || true
-SUPERVISOR_LOG="${SUPERVISOR_LOG_DIR}/supervisor.log"
+readonly SUPERVISOR_LOG_DIR
+readonly SUPERVISOR_LOG="${SUPERVISOR_LOG_DIR}/supervisor.log"

,


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@sonarqubecloud
Copy link

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request correctly fixes an 'unbound variable' regression by reordering global variable definitions to occur before modules that depend on them are sourced. The change is logical and well-explained. I have one suggestion regarding error handling during directory creation to improve the script's robustness by adhering more closely to the repository's style guide.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This line suppresses all errors from mkdir -p, including critical ones like permission denied. If mkdir -p fails, the script will continue silently due to || true, but will likely fail later when trying to write to the log file, making the root cause harder to debug.

This violates the repository style guide:

  • Line 13: || true guards are intended for commands like grep that can "fail" in normal operation, not for mkdir where a failure is a genuine error.
  • Line 50: 2>/dev/null is used for blanket suppression, which is disallowed.

Removing the error suppression will allow set -e to correctly halt the script on a legitimate directory creation failure, making it more robust.

Suggested change
mkdir -p "$SUPERVISOR_LOG_DIR" 2>/dev/null || true
mkdir -p "$SUPERVISOR_LOG_DIR"
References
  1. Use || true guards only for commands that are expected to fail during normal execution under set -e, like grep with no matches or arithmetic evaluations. mkdir failing is a genuine error. (link)
  2. Error stream redirection to /dev/null is only acceptable when redirecting to log files, not for blanket suppression of potential errors. (link)

@marcusquinn marcusquinn merged commit c0339c5 into main Feb 13, 2026
14 of 15 checks passed
marcusquinn added a commit that referenced this pull request Feb 13, 2026
@marcusquinn marcusquinn deleted the hotfix/pulse-lock-dir-unbound branch February 21, 2026 01:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

t1034: Fix PULSE_LOCK_DIR unbound variable — t1031 modularisation regression breaking cron pulse dispatch

1 participant