t1403: fix macOS timeout compatibility and UTC timezone parsing in aidevops update#2909
Conversation
Summary of ChangesHello, 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 significantly enhances the cross-platform compatibility and correctness of the 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
|
|
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. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThis PR makes macOS date parsing UTC-aware in three freshness-check functions by prefixing Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
aidevops.sh (1)
31-44: Portable timeout implementation looks solid for cross-platform compatibility.The fallback chain (native
timeout→ perl alarm → gtimeout → direct execution) correctly handles macOS environments lacking coreutils. The perlalarmapproach is a well-known portable pattern.One consideration: the final fallback (lines 42-43) runs the command without any timeout, which could still cause hangs if no timeout mechanism is available. Given the PR's goal of preventing indefinite hangs, consider logging a warning when falling back to no-timeout execution:
🛡️ Optional: Add warning for no-timeout fallback
else + # Warn: no timeout mechanism available - command may hang + [[ -t 2 ]] && echo -e "\033[1;33m[WARN]\033[0m No timeout command available - running without timeout" >&2 "$@" fiNote: Exit codes differ between mechanisms (Linux
timeout: 124, perlalarm: 142). Current callers don't check for specific timeout exit codes, so this is fine.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aidevops.sh` around lines 31 - 44, The final else branch of _timeout_cmd currently executes the command with no timeout; add a warning before that fallback so callers/ops know the command may hang. In the _timeout_cmd function, update the last branch (the else that currently runs "$@") to first emit a concise warning to stderr (e.g., printf or echo >&2) indicating "no timeout available, running command without timeout" and then run the command, preserving existing behavior and exit code propagation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@aidevops.sh`:
- Around line 31-44: The final else branch of _timeout_cmd currently executes
the command with no timeout; add a warning before that fallback so callers/ops
know the command may hang. In the _timeout_cmd function, update the last branch
(the else that currently runs "$@") to first emit a concise warning to stderr
(e.g., printf or echo >&2) indicating "no timeout available, running command
without timeout" and then run the command, preserving existing behavior and exit
code propagation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dcc41fae-2aef-45ec-ad5d-ce5e4200cf84
📒 Files selected for processing (3)
.agents/scripts/auto-update-helper.sh.agents/scripts/tool-version-check.shaidevops.sh
|
This PR is from an external contributor (@johnwaldo). Auto-merge is disabled for external PRs — a maintainer must review and merge manually. |
marcusquinn
added
the
external-contributor
![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 important compatibility fixes for macOS by adding a portable timeout implementation and correcting a UTC date parsing issue. The changes are logical and well-targeted. I've suggested a small improvement to the timeout function to prefer gtimeout over the perl fallback for better robustness. Additionally, I've pointed out a few places where error output is being suppressed with 2>/dev/null, which goes against the repository's conventions for error handling and debugging. Removing these suppressions will make the script more transparent when issues occur.
aidevops.sh
Outdated
| elif command -v perl &>/dev/null; then | ||
| perl -e 'alarm shift; exec @ARGV' "$secs" "$@" | ||
| elif command -v gtimeout &>/dev/null; then | ||
| gtimeout "$secs" "$@" |
There was a problem hiding this comment.
The gtimeout command (from GNU coreutils, often installed on macOS via Homebrew) is a more robust implementation of timeout than the perl one-liner, as it properly handles exit codes. It's better to prefer gtimeout if it's available. I suggest checking for gtimeout before falling back to perl.
| elif command -v perl &>/dev/null; then | |
| perl -e 'alarm shift; exec @ARGV' "$secs" "$@" | |
| elif command -v gtimeout &>/dev/null; then | |
| gtimeout "$secs" "$@" | |
| elif command -v gtimeout &>/dev/null; then | |
| gtimeout "$secs" "$@" | |
| elif command -v perl &>/dev/null; then | |
| perl -e 'alarm shift; exec @ARGV' "$secs" "$@" |
aidevops.sh
Outdated
| if [[ "$pkg_ref" == brew:* ]]; then | ||
| local brew_pkg="${pkg_ref#brew:}" | ||
| latest=$(timeout 30 brew info --json=v2 "$brew_pkg" 2>/dev/null | jq -r '.formulae[0].versions.stable // empty' 2>/dev/null || true) | ||
| latest=$(_timeout_cmd 30 brew info --json=v2 "$brew_pkg" 2>/dev/null | jq -r '.formulae[0].versions.stable // empty' 2>/dev/null || true) |
There was a problem hiding this comment.
This line suppresses stderr from both brew info and jq using 2>/dev/null. This goes against the project's general rules, which advise against blanket error suppression to ensure that issues like authentication failures, syntax errors, or system problems remain visible for debugging. Please remove the 2>/dev/null redirections to improve error visibility. The || true at the end is sufficient to prevent the script from exiting on failure.
| latest=$(_timeout_cmd 30 brew info --json=v2 "$brew_pkg" 2>/dev/null | jq -r '.formulae[0].versions.stable // empty' 2>/dev/null || true) | |
| latest=$(_timeout_cmd 30 brew info --json=v2 "$brew_pkg" | jq -r '.formulae[0].versions.stable // empty' || 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.
- 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 for debugging.
aidevops.sh
Outdated
| latest=$(_timeout_cmd 30 brew info --json=v2 "$brew_pkg" 2>/dev/null | jq -r '.formulae[0].versions.stable // empty' 2>/dev/null || true) | ||
| else | ||
| latest=$(timeout 30 npm view "$pkg_ref" version 2>/dev/null || true) | ||
| latest=$(_timeout_cmd 30 npm view "$pkg_ref" version 2>/dev/null || true) |
There was a problem hiding this comment.
This line suppresses stderr from npm view using 2>/dev/null. This can hide important error messages related to network issues or non-existent packages. According to the project's general rules, it's better to avoid blanket error suppression. Please remove 2>/dev/null to make debugging easier. The || true will still prevent the script from exiting on failure.
| latest=$(_timeout_cmd 30 npm view "$pkg_ref" version 2>/dev/null || true) | |
| latest=$(_timeout_cmd 30 npm view "$pkg_ref" version || 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.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.agents/scripts/tool-version-check.sh (1)
374-387: Excellent fix for the macOS SIGALRM pipe deadlock.The temp file approach is the right solution here. The comments clearly explain the root cause (perl alarm doesn't close the pipe's write end), and the implementation is clean:
- Proper
mktempusage withTMPDIRfallback- Output captured to temp file, then displayed via
tail -2- Cleanup via
rm -fensures removal in both success and failure pathsFor extra robustness against script interruption (Ctrl+C between mktemp and rm), you could add trap-based cleanup. This is optional since orphaned temp files in
/tmpare low-risk:🛡️ Optional: trap-based cleanup for interrupt resilience
# causing tail to block forever. Use a temp file instead. local _update_log _update_log=$(mktemp "${TMPDIR:-/tmp}/tool-update.XXXXXX") + trap 'rm -f "$_update_log"' EXIT if timeout_sec 120 bash -c "$update_cmd" >"$_update_log" 2>&1; then tail -2 "$_update_log" echo -e " ${GREEN}✓ Updated${NC}" else tail -2 "$_update_log" echo -e " ${RED}✗ Failed${NC}" fi rm -f "$_update_log" + trap - EXIT echo ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/tool-version-check.sh around lines 374 - 387, Add a trap to ensure the temporary file created by mktemp (stored in _update_log) is removed on script exit or interruption: after creating _update_log, register a trap for EXIT/INT/TERM that runs the same cleanup as rm -f "$_update_log" (and clear the trap after cleanup if needed). Ensure the trap is set before calling timeout_sec and that rm -f "$_update_log" remains in the success/failure branches so normal completion still works; reference the local _update_log variable, mktemp usage, timeout_sec invocation, and existing rm -f call when adding the trap.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.agents/scripts/tool-version-check.sh:
- Around line 374-387: Add a trap to ensure the temporary file created by mktemp
(stored in _update_log) is removed on script exit or interruption: after
creating _update_log, register a trap for EXIT/INT/TERM that runs the same
cleanup as rm -f "$_update_log" (and clear the trap after cleanup if needed).
Ensure the trap is set before calling timeout_sec and that rm -f "$_update_log"
remains in the success/failure branches so normal completion still works;
reference the local _update_log variable, mktemp usage, timeout_sec invocation,
and existing rm -f call when adding the trap.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3cc14b29-64d1-4bb1-b92a-2f0b778fd376
📒 Files selected for processing (1)
.agents/scripts/tool-version-check.sh
|
Permission check failed for this PR (HTTP 403 from collaborator permission API). Unable to determine if @johnwaldo is a maintainer or external contributor. A maintainer must review and merge this PR manually. This is a fail-closed safety measure — the pulse will not auto-merge until the permission API succeeds. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.agents/scripts/tool-version-check.sh (1)
121-130: Guardmktempfailures to keep update checks resilient underset -e.Both temp-log paths assume
mktempalways succeeds. If it fails, redirection/tail paths become brittle and can abort the run without a clear recovery path.♻️ Suggested hardening patch
- local _ver_log - _ver_log=$(mktemp "${TMPDIR:-/tmp}/tool-ver.XXXXXX") + local _ver_log + if ! _ver_log=$(mktemp "${TMPDIR:-/tmp}/tool-ver.XXXXXX"); then + echo "unknown" + return 0 + fi @@ - local _update_log - _update_log=$(mktemp "${TMPDIR:-/tmp}/tool-update.XXXXXX") + local _update_log + if ! _update_log=$(mktemp "${TMPDIR:-/tmp}/tool-update.XXXXXX"); then + echo -e " ${RED}✗ Failed to create temp log${NC}" + continue + fiAs per coding guidelines ".agents/scripts/*.sh: Automation scripts - focus on: Reliability and robustness ... Error recovery mechanisms."
Also applies to: 386-395
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/tool-version-check.sh around lines 121 - 130, The mktemp call that sets local _ver_log may fail and leave _ver_log unset, causing subsequent redirections and head/grep to error under set -e; fix by checking the mktemp return and providing a safe fallback file (e.g., _ver_log="${TMPDIR:-/tmp}/tool-ver.$$") when mktemp fails, ensure _ver_log is non-empty before using it with timeout_sec "$cmd" $ver_flag and head/grep, and only rm -f "$_ver_log" if the variable is set and the file exists; reference the local variable _ver_log and the usage sites of timeout_sec, "$cmd", and $ver_flag to locate where to add the guard and fallback logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/scripts/tool-version-check.sh:
- Around line 382-388: The timeout_sec function currently prefers perl over
gtimeout; change its precedence to prefer gtimeout when available by checking
for gtimeout first and only falling back to perl if gtimeout is absent — update
the logic in the timeout_sec function to test for the gtimeout command before
testing for perl (refer to the timeout_sec function and the gtimeout and perl
checks) so the gtimeout implementation is used whenever present for more
reliable timeout and pipe handling.
---
Nitpick comments:
In @.agents/scripts/tool-version-check.sh:
- Around line 121-130: The mktemp call that sets local _ver_log may fail and
leave _ver_log unset, causing subsequent redirections and head/grep to error
under set -e; fix by checking the mktemp return and providing a safe fallback
file (e.g., _ver_log="${TMPDIR:-/tmp}/tool-ver.$$") when mktemp fails, ensure
_ver_log is non-empty before using it with timeout_sec "$cmd" $ver_flag and
head/grep, and only rm -f "$_ver_log" if the variable is set and the file
exists; reference the local variable _ver_log and the usage sites of
timeout_sec, "$cmd", and $ver_flag to locate where to add the guard and fallback
logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 838533dc-967f-41ab-938e-50904de79aa5
📒 Files selected for processing (2)
.agents/scripts/tool-version-check.shaidevops.sh
…g in auto-update The original timeout fix (PR marcusquinn#2384) patched version check functions but missed two locations that still use bare `timeout` (unavailable on macOS): 1. tool-version-check.sh:374 — update execution path. When a user answers 'y' to 'Run full tool update?', the update commands run without any timeout on macOS, causing indefinite hangs on slow npm installs. 2. aidevops.sh:852-854 — quick key-tool staleness check in `aidevops status`. Uses bare `timeout 30` which silently fails on macOS (exit 127), making the staleness check always report 'up to date'. Additionally, auto-update-helper.sh has a timezone bug that prevents tool, skill, and OpenClaw freshness checks from ever triggering on macOS: - State file stores timestamps in UTC (e.g., 2026-03-05T08:35:51Z) - macOS `date -j -f` ignores the Z suffix and interprets as local time - This produces negative elapsed times, so the time gate always skips - Fix: prefix with TZ=UTC to force correct interpretation Note: ~20 other scripts in the repo have the same date -j -f pattern (model-registry-helper, pulse, dispatch, etc.). Those are lower-risk (archived or less frequently hit) and should be fixed in a follow-up.
…acOS The perl alarm fallback (used on macOS which lacks timeout) doesn't close the pipe's write end on SIGALRM. When timeout_sec output is piped to tail -2, tail blocks forever waiting for EOF. Use a temp file instead.
…timeout warning - Swap gtimeout before perl in timeout_sec() for more robust timeout handling - Guard mktemp failures in get_installed_version() and update loop - Add stderr warning when no timeout mechanism is available - Addresses CodeRabbit CHANGES_REQUESTED and Gemini review comments
marcusquinn
force-pushed
the
bugfix/fix-macos-timeout-and-timezone
branch
from
fc66c31 to
808cbe3
Compare
3 participants
Summary
timeoutcommand —aidevops updatecallstimeout 30 npm view ...andtimeout 120 bash -c "$update_cmd"which silently fail (exit 127) or hang on macOS. Added_timeout_cmd()inaidevops.shandtimeout_sec()intool-version-check.shusing perl alarm fallback (available on all macOS).tailhangs with perl alarm — even withtimeout_secworking,timeout_sec 120 bash -c "$cmd" 2>&1 | tail -2hangs forever on macOS. The perl alarm (SIGALRM) kills the process but doesn't close the pipe's write end, sotailblocks waiting for EOF. Replaced with temp file: write output to file, thentail -2the file.date -j -fon macOS ignores theZsuffix, interpreting UTC timestamps in local time. This caused freshness checks to be off by the local UTC offset (e.g., 7 hours for MST), potentially skipping update checks. Fixed by prefixing withTZ=UTC.Files Changed
aidevops.sh— Added_timeout_cmd()portable function, replaced 2 baretimeoutcalls.agents/scripts/tool-version-check.sh— Addedtimeout_sec()portable function, replaced baretimeout 120in update execution, replaced| tail -2pipe with temp file to prevent SIGALRM hang.agents/scripts/auto-update-helper.sh— Fixed 3date -jcalls to useTZ=UTCRoot Cause Analysis
Two separate bugs causing the same symptom (hang at tool update):
Missing
timeoutcommand — macOS has notimeout. The shell tries to execute it, fails with exit 127, and|| trueswallows the error. Fixed withtimeout_sec()/_timeout_cmd()usingperl -e 'alarm shift; exec @ARGV'.SIGALRM + pipe deadlock — Even with the perl alarm working, piping its output to
tail -2creates a deadlock. When SIGALRM fires, perl sends the signal to the exec'd process, but the pipe's write-end file descriptor isn't closed cleanly.tailholds the read end open, waiting for EOF that never comes. Linux'stimeoutcommand avoids this by using process groups (kill -TERM -- -$pgid). Fixed by redirecting to a temp file instead of piping.Testing
Verified on macOS (no native
timeout):_timeout_cmdcorrectly times out via perl alarm (exit 142)timeout_seccorrectly wraps npm/brew queriesCloses #2490
Summary by CodeRabbit
Bug Fixes
Chores