Fix unbound variable in log initialization #173
Conversation
WalkthroughUpdates file descriptor validation in lib/log.sh by switching to parameter expansion for LOG_FD and using a no-op redirection with a default fallback. The flow still preserves original STDOUT, configures logging to that FD, and execs LOG_FD to stdout; error redirection remains unchanged. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
|
/cc @PaulSD |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/log.sh(1 hunks)
🔇 Additional comments (1)
lib/log.sh (1)
11-11: LGTM! Proper fix for unbound variable underset -u.The parameter expansion
"${LOG_FD-}"correctly prevents unbound variable errors when LOG_FD is not set, and the fallback to stderr (fd 2) in the redirection test"${LOG_FD-2}"provides sensible default behavior. This change successfully addresses the issue described in the PR objectives while maintaining the original functionality.
|
|
||
| # Unless $LOG_FD is already set to a valid fd | ||
| if ! [[ "$LOG_FD" =~ ^[0-9]+$ ]] || ! { true >&"$LOG_FD" ; } 2>/dev/null ; then | ||
| if ! [[ "${LOG_FD-}" =~ ^[0-9]+$ ]] || ! { : >&"${LOG_FD-2}"; } 2>/dev/null; then |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the fix works correctly under set -u.
Please confirm that this change resolves the unbound variable error when running with set -u and that the logging functionality works as expected in both scenarios (when LOG_FD is set and when it's unbound).
🏁 Script executed:
#!/bin/bash
# Description: Test the LOG_FD handling under set -u to ensure the fix works correctly
# Create a test script to simulate the scenario
cat > test_log_fd.sh << 'EOF'
#!/bin/bash
set -u # Enable nounset to catch unbound variables
# Source the log.sh file to test the fix
source lib/log.sh
echo "LOG_FD is now: $LOG_FD"
echo "Testing logging functionality..."
# Test basic logging
bashio::log "Test message"
bashio::log.info "Info message"
EOF
chmod +x test_log_fd.sh
# Test 1: Run without LOG_FD set (should not fail with set -u)
echo "=== Test 1: Running with set -u and no LOG_FD preset ==="
unset LOG_FD 2>/dev/null || true
./test_log_fd.sh
echo ""
# Test 2: Run with LOG_FD preset
echo "=== Test 2: Running with LOG_FD preset ==="
export LOG_FD=1
./test_log_fd.sh
# Cleanup
rm -f test_log_fd.shLength of output: 339
Initialize or guard __BASHIO_EXIT_OK to avoid unbound variable under set -u.
The tests still fail with “unbound variable” at lib/log.sh:27 for __BASHIO_EXIT_OK. Define it before use (e.g. : "${__BASHIO_EXIT_OK:=0}") or use ${__BASHIO_EXIT_OK:-} wherever it’s referenced.
🤖 Prompt for AI Agents
In lib/log.sh around line 11 (and affecting usage at line 27), the variable
__BASHIO_EXIT_OK may be referenced when running under set -u causing an "unbound
variable" error; initialize or guard it before use by setting a safe default
(e.g. ensure __BASHIO_EXIT_OK is defined to 0 if unset) or change references to
use parameter expansion that provides a default (use the :- form) so the script
never reads an undefined variable.
There was a problem hiding this comment.
@coderabbitai bashio always sources lib/const.sh first, which defines __BASHIO_EXIT_OK. I think with that in mind things should be fine.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if lib/const.sh exists and contains __BASHIO_EXIT_OK definition
echo "=== Searching for lib/const.sh ==="
fd const.sh lib/
echo ""
echo "=== Checking if __BASHIO_EXIT_OK is defined in lib/const.sh ==="
if [ -f lib/const.sh ]; then
echo "Found lib/const.sh, checking for __BASHIO_EXIT_OK definition:"
rg -n "__BASHIO_EXIT_OK" lib/const.sh
else
echo "lib/const.sh not found, searching for __BASHIO_EXIT_OK definitions in the entire codebase:"
rg -n "__BASHIO_EXIT_OK.*=" --type=bash
fi
echo ""
echo "=== Checking how bashio is typically initialized ==="
# Look for files that source lib/const.sh
rg -n "source.*const\.sh|\..*const\.sh" --type=bash
Length of output: 316
🏁 Script executed:
#!/bin/bash
# Let's look at the const.sh file content and check how bashio is initialized
echo "=== Content of lib/const.sh showing __BASHIO_EXIT_OK definition ==="
cat lib/const.sh | head -30
echo ""
echo "=== Searching for sourcing patterns without file type restriction ==="
rg -n "source.*const\.sh|\..*const\.sh"
echo ""
echo "=== Looking for main bashio initialization files ==="
fd -e sh . | head -10
echo ""
echo "=== Checking if there's a main bashio.sh or similar entry point ==="
fd bashio
Length of output: 1774
@agners, you're absolutely right! Thank you for the clarification. I can confirm that:
lib/const.shproperly defines__BASHIO_EXIT_OK=0on line 21lib/bashio.shsourceslib/const.shfirst (line 26), ensuring all constants are available before any other bashio functionality
My concern about __BASHIO_EXIT_OK being unbound was based on analyzing lib/log.sh in isolation rather than in the proper bashio usage context. In normal bashio usage, the constants are always properly initialized.
The LOG_FD fix using parameter expansion (${LOG_FD-} and ${LOG_FD-2}) is still the right approach for set -u compatibility, and the logging functionality should work correctly in both scenarios.
✏️ Learnings added
Learnt from: agners
PR: hassio-addons/bashio#173
File: lib/log.sh:11-11
Timestamp: 2025-09-26T15:27:24.191Z
Learning: In the bashio library, lib/const.sh defines all constants including __BASHIO_EXIT_OK, and lib/bashio.sh always sources lib/const.sh first, ensuring proper initialization before any other bashio functionality is used.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
frenck
added
the
bugfix
2 participants
Proposed Changes
With #172 we've added
LOG_FDas variable to redirect log messages to. The safety changes turned out to be problematic since we run the shell withset -u(ornounset). Use parameter expansion to avoid accessing the unbound variable.Related Issues
hassio-addons/addon-base#343
Summary by CodeRabbit
Bug Fixes
Refactor