Lint bash scripts with custom shebangs #3824
Conversation
Many scripts in this repository have this shebang:
#!/usr/bin/with-contenv bashio
However, this doesn't match the shebang regex in action-shellcheck
and therefore those files wouldn't get included in the linting check.
|
Note Reviews pausedUse the following commands to manage reviews:
📝 Walkthrough📝 WalkthroughWalkthroughThe changes include updates to multiple Bash scripts and a GitHub Actions workflow file. Key modifications involve the addition of an Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GitHub Actions
participant ShellCheck
User->>GitHub Actions: Trigger lint workflow
GitHub Actions->>ShellCheck: Run ShellCheck with additional_files
ShellCheck->>GitHub Actions: Return lint results
GitHub Actions->>User: Provide lint results
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (1)
.github/workflows/lint.yml (1)
Line range hint
4-4: Consider updating hadolint version.The current hadolint version (v1.17.2) is quite old. Consider updating to a newer version to benefit from the latest rules and improvements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/lint.yml(1 hunks)
🔇 Additional comments (1)
.github/workflows/lint.yml (1)
Line range hint 4-6: Environment configuration is well-aligned with custom shebang handling.
The SHELLCHECK_OPTS configuration with -e SC1008 appropriately handles the custom shebang scenario by disabling the "not following" warning, while -s bash ensures proper bash script checking.
|
Update: pushed a commit that fixes/allowlists all the shellcheck violations in scripts that are now newly included in the linting process. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (42)
silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/cpcd/finish (1)
10-13: Consider adding context about exit codes.The shellcheck disable directive and exit code handling logic are correct. However, consider adding a comment explaining why exit code 256 is treated specially (typically indicates "service disabled" in s6) and log the reason before halting.
# shellcheck disable=SC2086 +# Exit code 256 typically means service is disabled in s6, which is expected if [ ${1} -ne 0 ] && [ ${1} -ne 256 ]; then + bashio::log.error "CPC daemon failed unexpectedly, halting container..." /run/s6/basedir/bin/halt fiopenwakeword/rootfs/etc/s6-overlay/s6-rc.d/openwakeword/run (1)
Line range hint
12-18: Add input validation and improve error handlingThe current implementation has several potential issues:
- Configuration values are used without validation
- The SC2068 directive disables important quote checking
- Missing error handling for required configuration
Consider applying this safer implementation:
-# shellcheck disable=SC2068 +# Validate required configuration +if ! bashio::config.exists 'threshold'; then + bashio::exit.nok "Threshold configuration is required" +fi + +if ! bashio::config.exists 'trigger_level'; then + bashio::exit.nok "Trigger level configuration is required" +fi + +# Validate threshold and trigger_level values +threshold=$(bashio::config 'threshold') +trigger_level=$(bashio::config 'trigger_level') + +if ! [[ "$threshold" =~ ^[0-9]+(\.[0-9]+)?$ ]]; then + bashio::exit.nok "Invalid threshold value: $threshold" +fi + +if ! [[ "$trigger_level" =~ ^[0-9]+(\.[0-9]+)?$ ]]; then + bashio::exit.nok "Invalid trigger level value: $trigger_level" +fi + exec python3 -m wyoming_openwakeword \ --uri 'tcp://0.0.0.0:10400' \ --preload-model 'ok_nabu' \ --custom-model-dir /share/openwakeword \ - --threshold "$(bashio::config 'threshold')" \ - --trigger-level "$(bashio::config 'trigger_level')" ${flags[@]} + --threshold "${threshold}" \ + --trigger-level "${trigger_level}" \ + "${flags[@]+"${flags[@]}"}"silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/zigbeed/run (1)
6-7: Consider restructuring the directory operations.While the current implementation works, it could be more readable and avoid the need for SC2015 suppression.
Here's a clearer alternative:
-# shellcheck disable=SC2015 -mkdir -p /data/zigbeed && cd /data/zigbeed || bashio::exit.nok "Could not change to zigbeed working directory" +if ! mkdir -p /data/zigbeed; then + bashio::exit.nok "Could not create zigbeed working directory" +fi +if ! cd /data/zigbeed; then + bashio::exit.nok "Could not change to zigbeed working directory" +fipiper/rootfs/etc/s6-overlay/s6-rc.d/piper/run (3)
Line range hint
15-27: Consider removing SC2068 disable and improve array handlingThe shellcheck disable for SC2068 can be avoided by properly quoting the array expansion. Additionally, consider adding error handling for required configuration values.
Here's the suggested improvement:
-# shellcheck disable=SC2068 +# Verify required configurations +if ! bashio::config.exists 'voice'; then + bashio::exit.nok "Voice configuration is required" +fi + exec python3 -m wyoming_piper \ --piper '/usr/share/piper/piper' \ --uri 'tcp://0.0.0.0:10200' \ --length-scale "$(bashio::config 'length_scale')" \ --noise-scale "$(bashio::config 'noise_scale')" \ --noise-w "$(bashio::config 'noise_w')" \ --speaker "$(bashio::config 'speaker')" \ --voice "$(bashio::config 'voice')" \ --max-piper-procs "$(bashio::config 'max_piper_procs')" \ --data-dir /data \ --data-dir /share/piper \ - --download-dir /data ${flags[@]} + --download-dir /data "${flags[@]+"${flags[@]}"}"
Line range hint
6-13: Consider adding debug logging for flag constructionFor better observability, especially when debug_logging is enabled, consider logging the constructed flags.
flags=() if bashio::config.true 'update_voices'; then flags+=('--update-voices') fi if bashio::config.true 'debug_logging'; then flags+=('--debug') + bashio::log.debug "Constructed flags: ${flags[*]}" fi
Shebang path inconsistency found across repository
The verification reveals two different shebang paths being used across the repository:
#!/command/with-contenv bashio(40 files)#!/usr/bin/with-contenv bashio(74 files)This inconsistency should be standardized to maintain uniformity. The majority of scripts (74) use
#!/usr/bin/with-contenv bashio, suggesting this is the preferred path.🔗 Analysis chain
Line range hint
1-1: Verify shebang path consistency across the repositoryThe shebang path
#!/command/with-contenv bashiodiffers from the one mentioned in the PR description (#!/usr/bin/with-contenv bashio). This inconsistency should be addressed to maintain uniformity across scripts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for different shebang variations in the repository # Expected: Consistent shebang usage across scripts # Search for different shebang patterns echo "Checking shebang patterns:" rg -l "^#!.*with-contenv bashio" | while read -r file; do head -n1 "$file" done | sort | uniq -cLength of output: 9380
nginx_proxy/rootfs/etc/s6-overlay/s6-rc.d/crond/finish (1)
6-8: Consider adding error handling for missing exit code fileThe script assumes
/run/s6-linux-init-container-results/exitcodeexists. While this is likely true in the S6 context, defensive programming would check for file existence.Consider adding a check:
+if [[ ! -f /run/s6-linux-init-container-results/exitcode ]]; then + bashio::log.error "Container exit code file not found" + echo 1 > /run/s6-linux-init-container-results/exitcode + exec /run/s6/basedir/bin/halt +fi readonly exit_code_container=$(</run/s6-linux-init-container-results/exitcode)whisper/rootfs/etc/s6-overlay/s6-rc.d/whisper/finish (2)
Line range hint
7-11: LGTM! Good use of readonly variablesThe variable declarations are well-structured with:
- Proper use of readonly for immutable values
- Clear variable naming
- Appropriate shellcheck directive for SC2155
Consider adding comments describing what each exit code represents for better maintainability.
Minor inconsistency found in exit code signal handling
Most finish scripts follow the same pattern, but there's a small inconsistency in the arithmetic expression for signal handling:
- Some scripts use
$((128 + exit_code_signal))(correct)- Others use
$((128 + $exit_code_signal))(with extra$)Affected files:
samba/rootfs/etc/s6-overlay/s6-rc.d/smbd/finishsamba/rootfs/etc/s6-overlay/s6-rc.d/nmbd/finishmariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-core/finishWhile both forms work in bash arithmetic context, the extra
$is unnecessary and inconsistent with the majority pattern.🔗 Analysis chain
Line range hint
18-26: LGTM! Robust exit code handlingThe exit code handling logic is well-structured and follows s6-overlay conventions:
- Proper signal handling for exit code 256
- Appropriate container exit code preservation logic
- Correct SIGTERM handling
Let's verify similar exit code handling in other service finish scripts:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find and check other finish scripts for consistent exit code handling fd 'finish$' --type f --exec sh -c ' echo "=== {} ===" grep -A 5 "exit_code_signal" "{}" || true echo 'Length of output: 9414
assist_microphone/rootfs/etc/s6-overlay/s6-rc.d/assist_microphone/finish (1)
Line range hint
17-26: Add comments explaining signal handling constantsThe logic is correct, but would benefit from documentation explaining the significance of the magic numbers:
+# Exit code 256 indicates the process was terminated by a signal if [[ "${exit_code_service}" -eq 256 ]]; then if [[ "${exit_code_container}" -eq 0 ]]; then + # Calculate standard Linux exit code for signal termination (128 + signal number) echo $((128 + exit_code_signal)) > /run/s6-linux-init-container-results/exitcode fi + # Signal 15 is SIGTERM - graceful shutdown [[ "${exit_code_signal}" -eq 15 ]] && exec /run/s6/basedir/bin/halt elif [[ "${exit_code_service}" -ne 0 ]]; then if [[ "${exit_code_container}" -eq 0 ]]; then echo "${exit_code_service}" > /run/s6-linux-init-container-results/exitcode fi + # Halt the container due to service failure exec /run/s6/basedir/bin/halt ficonfigurator/rootfs/etc/s6-overlay/s6-rc.d/configurator/finish (2)
7-10: Consider splitting variable declarations and assignmentsInstead of disabling SC2155, consider splitting the variable declarations and assignments for better error handling:
-# shellcheck disable=SC2155 -readonly exit_code_container=$(</run/s6-linux-init-container-results/exitcode) +declare exit_code_container +exit_code_container=$(</run/s6-linux-init-container-results/exitcode) +readonly exit_code_containerThis approach:
- Allows error detection if the command substitution fails
- Maintains the readonly protection
- Follows shellcheck best practices
Line range hint
12-26: Consider enhancing logging for shutdown decisionsWhile the basic logging is present, adding more detailed logging before critical actions would improve debugging capabilities.
bashio::log.info \ "Service exited with code ${exit_code_service}" \ "(by signal ${exit_code_signal})" if [[ "${exit_code_service}" -eq 256 ]]; then if [[ "${exit_code_container}" -eq 0 ]]; then + bashio::log.info "Setting container exit code to $((128 + exit_code_signal))" echo $((128 + exit_code_signal)) > /run/s6-linux-init-container-results/exitcode fi + [[ "${exit_code_signal}" -eq 15 ]] && bashio::log.info "Received SIGTERM, initiating graceful shutdown" [[ "${exit_code_signal}" -eq 15 ]] && exec /run/s6/basedir/bin/halt elif [[ "${exit_code_service}" -ne 0 ]]; then if [[ "${exit_code_container}" -eq 0 ]]; then + bashio::log.info "Service failed, setting container exit code to ${exit_code_service}" echo "${exit_code_service}" > /run/s6-linux-init-container-results/exitcode fi + bashio::log.info "Initiating shutdown due to service failure" exec /run/s6/basedir/bin/halt fiopenwakeword/rootfs/etc/s6-overlay/s6-rc.d/openwakeword/finish (1)
7-10: Consider adding error handling for the exit code file read operation.While the current implementation is functional, it could benefit from explicit error handling when reading the container exit code file.
-readonly exit_code_container=$(</run/s6-linux-init-container-results/exitcode) +if [[ ! -f /run/s6-linux-init-container-results/exitcode ]]; then + bashio::log.error "Container exit code file not found" + readonly exit_code_container=1 +else + readonly exit_code_container=$(</run/s6-linux-init-container-results/exitcode) +fipiper/rootfs/etc/s6-overlay/s6-rc.d/piper/finish (2)
Line range hint
7-11: Consider separating variable declaration and assignmentWhile disabling SC2155 works, it's better to separate declaration and assignment to catch potential errors in command substitution.
Consider this safer approach:
-# shellcheck disable=SC2155 -readonly exit_code_container=$(</run/s6-linux-init-container-results/exitcode) +declare exit_code_container +exit_code_container=$(</run/s6-linux-init-container-results/exitcode) +readonly exit_code_container
Line range hint
18-26: Consider improving clarity of exit code handlingWhile the logic is correct, it could benefit from better documentation and structure.
Consider these improvements:
- Add constants to explain magic numbers:
# Add at the top of the script readonly SIGNAL_EXIT_CODE=256 readonly SIGTERM=15 readonly SIGNAL_EXIT_OFFSET=128
- Consider simplifying the nested conditions:
-if [[ "${exit_code_service}" -eq 256 ]]; then +if [[ "${exit_code_service}" -eq ${SIGNAL_EXIT_CODE} ]]; then if [[ "${exit_code_container}" -eq 0 ]]; then - echo $((128 + exit_code_signal)) > /run/s6-linux-init-container-results/exitcode + echo $((SIGNAL_EXIT_OFFSET + exit_code_signal)) > /run/s6-linux-init-container-results/exitcode fi - [[ "${exit_code_signal}" -eq 15 ]] && exec /run/s6/basedir/bin/halt + [[ "${exit_code_signal}" -eq ${SIGTERM} ]] && exec /run/s6/basedir/bin/halt
- Add a comment explaining the signal handling logic:
# When service exits due to a signal (256), we calculate the actual exit code # by adding 128 to the signal number, following the Linux convention. # Special handling for SIGTERM (15) to ensure proper container shutdown.whisper/rootfs/etc/s6-overlay/s6-rc.d/whisper/run (5)
Line range hint
6-8: Improve AVX detection precisionThe current grep pattern might match partial words. Consider using word boundaries for more precise matching.
-if [ "$(uname -m)" == "x86_64" ] && ! grep -qw 'avx' /proc/cpuinfo; then +if [ "$(uname -m)" == "x86_64" ] && ! grep -E '\bavx\b' /proc/cpuinfo; then
Line range hint
10-17: Add model path validationThe script should verify that the specified model exists or is accessible before proceeding.
model="$(bashio::config 'model')" if [ "${model}" = 'custom' ]; then # Override with custom model model="$(bashio::config 'custom_model')" if [ -z "${model}" ]; then bashio::exit.nok "Custom model is not set" fi + # Verify model accessibility + if ! bashio::fs.file_exists "${model}" && ! bashio::fs.directory_exists "${model}"; then + bashio::exit.nok "Custom model '${model}' not found or inaccessible" + fi fi
Line range hint
19-21: Initialize flags arrayThe flags array should be initialized before use to prevent potential issues.
+declare -a flags=() if bashio::config.true 'debug_logging'; then flags+=('--debug') fi
Line range hint
24-32: Improve command parameter handlingSeveral improvements can be made to handle parameters more safely:
- Quote the flags array expansion
- Add validation for required config values
- Handle spaces in parameters properly
+# Validate required parameters +bashio::config.require 'beam_size' +bashio::config.require 'language' + # shellcheck disable=SC2068 exec python3 -m wyoming_faster_whisper \ --uri 'tcp://0.0.0.0:10300' \ --model "${model}" \ --beam-size "$(bashio::config 'beam_size')" \ --language "$(bashio::config 'language')" \ --initial-prompt "$(bashio::config 'initial_prompt')" \ --data-dir /data \ - --download-dir /data ${flags[@]} + --download-dir /data "${flags[@]+"${flags[@]}"}"
Missing documentation for
debug_loggingconfiguration optionAll configuration options used in the script are documented in DOCS.md except for
debug_logging. The following options are properly documented:
modelandcustom_modelunder "Option: model" and "Option: custom_model" sectionsbeam_sizeunder "Option: beam_size" sectionlanguageunder "Option: language" sectioninitial_promptunder "Option: initial_prompt" sectionThe
debug_loggingoption needs to be added to the documentation.🔗 Analysis chain
Line range hint
1-32: Verify configuration documentationEnsure that all configuration options used in this script (
model,custom_model,beam_size,language,initial_prompt,debug_logging) are properly documented in the addon's configuration documentation.
Let me check the actual content of the DOCS.md file to ensure our grep patterns aren't missing any alternative documentation formats.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if all config options are documented config_options=("model" "custom_model" "beam_size" "language" "initial_prompt" "debug_logging") docs_file="whisper/DOCS.md" if [ -f "$docs_file" ]; then echo "Checking configuration documentation..." for option in "${config_options[@]}"; do if ! grep -q "^\s*\`$option\`:" "$docs_file"; then echo "Warning: Configuration option '$option' might be missing from documentation" fi done else echo "Documentation file not found at $docs_file" fiLength of output: 1794
Script:
#!/bin/bash # Display the content of DOCS.md cat whisper/DOCS.mdLength of output: 3904
duckdns/rootfs/etc/s6-overlay/s6-rc.d/duckdns/finish (1)
19-20: Consider adding a comment explaining the exit code calculation.The
128 + exit_code_signalcalculation follows Unix convention for signal exit codes, but this might not be immediately obvious to all maintainers.+ # Convert signal to standard Unix exit code (128 + signal number) echo $((128 + exit_code_signal)) > /run/s6-linux-init-container-results/exitcodenginx_proxy/rootfs/etc/s6-overlay/s6-rc.d/nginx/run (4)
Line range hint
19-24: Improve DH params generation error handlingThe current implementation suppresses all output from openssl, making it difficult to diagnose issues.
Apply these changes for better error handling:
# Generate dhparams if ! bashio::fs.file_exists "${DHPARAMS_PATH}"; then bashio::log.info "Generating dhparams (this will take some time)..." - openssl dhparam -dsaparam -out "$DHPARAMS_PATH" 4096 > /dev/null + if ! openssl dhparam -dsaparam -out "$DHPARAMS_PATH" 4096 2> /tmp/dhparam.error; then + bashio::log.error "$(cat /tmp/dhparam.error)" + bashio::exit.nok "Failed to generate DH parameters" + fi fi
Line range hint
26-47: Add robust error handling for Cloudflare configurationThe current implementation lacks proper error handling for network operations and file handling.
Apply these changes for improved reliability:
if bashio::config.true 'cloudflare'; then # Generate cloudflare.conf if ! bashio::fs.file_exists "${CLOUDFLARE_CONF}"; then bashio::log.info "Creating 'cloudflare.conf' for real visitor IP address..." - echo "# Cloudflare IP addresses" > $CLOUDFLARE_CONF; + TEMP_CONF=$(mktemp) + { + echo "# Cloudflare IP addresses" + echo "" + echo "# - IPv4" + if ! IPV4_RANGES=$(curl --max-time 10 -fsSL https://www.cloudflare.com/ips-v4); then + bashio::exit.nok "Failed to fetch Cloudflare IPv4 ranges" + fi + for i in ${IPV4_RANGES}; do + echo "set_real_ip_from ${i};" + done + + echo "" + echo "# - IPv6" + if ! IPV6_RANGES=$(curl --max-time 10 -fsSL https://www.cloudflare.com/ips-v6); then + bashio::exit.nok "Failed to fetch Cloudflare IPv6 ranges" + fi + for i in ${IPV6_RANGES}; do + echo "set_real_ip_from ${i};" + done + + echo "" + echo "real_ip_header CF-Connecting-IP;" + } > "${TEMP_CONF}" - echo "" >> $CLOUDFLARE_CONF; - - echo "# - IPv4" >> $CLOUDFLARE_CONF; - for i in $(curl https://www.cloudflare.com/ips-v4); do - echo "set_real_ip_from ${i};" >> $CLOUDFLARE_CONF; - done - - echo "" >> $CLOUDFLARE_CONF; - echo "# - IPv6" >> $CLOUDFLARE_CONF; - for i in $(curl https://www.cloudflare.com/ips-v6); do - echo "set_real_ip_from ${i};" >> $CLOUDFLARE_CONF; - done - - echo "" >> $CLOUDFLARE_CONF; - echo "real_ip_header CF-Connecting-IP;" >> $CLOUDFLARE_CONF; + # Atomically move the temporary file to the final location + if ! mv "${TEMP_CONF}" "${CLOUDFLARE_CONF}"; then + rm -f "${TEMP_CONF}" + bashio::exit.nok "Failed to create Cloudflare configuration" + fi fi fi
Line range hint
49-52: Add certificate validation and error handlingThe current implementation doesn't validate the certificate file or handle potential errors.
Apply these changes for improved reliability:
# start server bashio::log.info "Running nginx..." -stat "/ssl/$(bashio::config 'certfile')" -c %y > /tmp/certificate_timestamp +CERT_FILE="/ssl/$(bashio::config 'certfile')" +if [[ ! -f "${CERT_FILE}" ]]; then + bashio::exit.nok "Certificate file ${CERT_FILE} does not exist" +fi + +if ! stat "${CERT_FILE}" -c %y > /tmp/certificate_timestamp; then + bashio::exit.nok "Failed to read certificate timestamp" +fi + exec nginx -c /etc/nginx.conf < /dev/null
Line range hint
1-52: Consider adding nginx configuration validation and health checksTo improve reliability and security, consider these architectural improvements:
- Add nginx configuration validation before starting the service
- Implement basic health checks after startup
- Add configuration backup/rollback mechanism
Example implementation:
# Validate nginx configuration if ! nginx -t -c /etc/nginx.conf; then bashio::exit.nok "Invalid nginx configuration" fi # Start nginx in background for health check nginx -c /etc/nginx.conf sleep 2 # Basic health check if ! curl -sf http://localhost:${PORT}/health 2>/dev/null; then nginx -s stop bashio::exit.nok "Nginx health check failed" fi # If health check passes, restart in foreground nginx -s stop exec nginx -c /etc/nginx.conf < /dev/nullopenthread_border_router/rootfs/etc/s6-overlay/s6-rc.d/otbr-agent/finish (4)
Line range hint
1-17: Add documentation for exit code handling logicThe exit code handling logic is correct, but it would benefit from additional documentation explaining why 256 is treated specially and what the significance of adding 128 to $2 is.
Add a comment block like this:
#!/usr/bin/with-contenv bashio # shellcheck shell=bash #============================================================================== # OpenThread BorderRouter Daemon finish script +# +# Exit code handling: +# - Exit code 256 indicates a signal termination, where $2 contains the signal number +# - Adding 128 to the signal number follows the Linux convention for signal exit codes #==============================================================================
Line range hint
20-28: Add timeout to ipset destruction retry loopThe while loop could potentially run indefinitely if the ipset remains in use. Consider adding a timeout mechanism.
Here's a suggested implementation:
ipset_destroy_if_exist() { # The ipset seems to be in use by the kernel for a brief period, # retry destroying it + local attempts=0 + local max_attempts=10 while ipset list -n "$1" 2> /dev/null; do + if [ "$attempts" -ge "$max_attempts" ]; then + bashio::log.warning "Failed to destroy ipset $1 after $max_attempts attempts" + return 1 + fi ipset destroy "$1" || true + attempts=$((attempts + 1)) + sleep 0.5 done }
Line range hint
30-45: Improve error handling and address shellcheck warningsThe code disables multiple shellcheck warnings (SC2154, SC2086) which might indicate potential issues:
- SC2154 warns about unassigned variables
- SC2086 warns about word splitting/globbing
Consider these improvements:
-# shellcheck disable=SC2154,SC2086 -while ip6tables -C FORWARD -o $thread_if -j $otbr_forward_ingress_chain 2> /dev/null; do +if [[ -z "${thread_if:-}" ]] || [[ -z "${otbr_forward_ingress_chain:-}" ]]; then + bashio::log.error "Required variables thread_if or otbr_forward_ingress_chain not set" + exit 1 +fi + +while ip6tables -C FORWARD -o "$thread_if" -j "$otbr_forward_ingress_chain" 2> /dev/null; do # shellcheck disable=SC2086 - ip6tables -D FORWARD -o $thread_if -j $otbr_forward_ingress_chain + ip6tables -D FORWARD -o "$thread_if" -j "$otbr_forward_ingress_chain" || \ + bashio::log.warning "Failed to delete FORWARD rule" done
Line range hint
47-56: Reduce code duplication by extracting chain cleanup logicThe ingress and egress cleanup logic is very similar and could be consolidated into a reusable function.
Consider refactoring like this:
+cleanup_forward_chain() { + local interface="$1" + local direction="$2" + local chain="$3" + + while ip6tables -C FORWARD "$direction" "$interface" -j "$chain" 2> /dev/null; do + ip6tables -D FORWARD "$direction" "$interface" -j "$chain" || \ + bashio::log.warning "Failed to delete FORWARD rule for $chain" + done + + if ip6tables -L "$chain" 2> /dev/null; then + ip6tables -w -F "$chain" + ip6tables -w -X "$chain" + fi +} + +cleanup_forward_chain "$thread_if" "-o" "$otbr_forward_ingress_chain" +cleanup_forward_chain "$thread_if" "-i" "$otbr_forward_egress_chain"openthread_border_router/rootfs/etc/s6-overlay/scripts/universal-silabs-flasher-up (1)
Line range hint
15-18: Enhance error messages for better debugging.While the error handling is good, the messages could be more detailed to aid in troubleshooting.
function exit_no_firmware { - bashio::log.warning "No firmware found for the selected device, assuming firmware is installed." + bashio::log.warning "No firmware found for device at ${device} (Manufacturer: ${usb_manufacturer:-unknown}, Product: ${usb_product:-unknown})" exit 0 } if [ ! -f "${usb_device_path}/idProduct" ]; then - bashio::log.info "The selected serial port is not a USB device." + bashio::log.info "The selected serial port ${device} is not a USB device (path: ${usb_device_path})" exit_no_firmware fiAlso applies to: 42-45
silabs_flasher/rootfs/etc/s6-overlay/scripts/universal-silabs-flasher-up (3)
Line range hint
1-18: Consider enhancing error handling with additional shell options.While
set -eis good for error handling, consider addingset -uto catch undefined variables andset -o pipefailto ensure pipeline failures are caught.set -e +set -u +set -o pipefail
Line range hint
34-41: Consider adding comments explaining the hardware-specific paths.The path
/sys/devices/platform/soc/fe201800.serial/tty/ttyAMA1is hardware-specific. Adding a comment explaining why this path indicates a Home Assistant Yellow device would improve maintainability.+# Home Assistant Yellow uses the BCM2711 UART4 mapped to ttyAMA1 if [ -d /sys/devices/platform/soc/fe201800.serial/tty/ttyAMA1 ] && [ "${device}" == "/dev/ttyAMA1" ]; then
Line range hint
92-98: Improve command execution robustness.The current implementation has unquoted variables in the command execution which could cause issues with special characters or spaces.
Consider using an array to handle the arguments more safely:
-# shellcheck disable=SC2086 -universal-silabs-flasher \ - ${verbose} \ - --device ${device} \ - --bootloader-baudrate "${bootloader_baudrate}" \ - ${gpio_reset_flag} \ - flash --force --firmware "/root/${firmware}" +# Build command arguments array +cmd=( + "universal-silabs-flasher" + "${verbose}" + "--device" "${device}" + "--bootloader-baudrate" "${bootloader_baudrate}" +) + +# Add GPIO reset flag if set +if [[ -n "${gpio_reset_flag}" ]]; then + cmd+=($gpio_reset_flag) +fi + +# Add final arguments +cmd+=( + "flash" + "--force" + "--firmware" "/root/${firmware}" +) + +# Execute command +"${cmd[@]}"silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/otbr-agent/run (4)
Line range hint
61-96: Consider adding documentation for the firewall rules.The firewall setup is comprehensive and well-structured. However, adding comments to explain the purpose of each rule would improve maintainability.
Consider adding comments like this:
ipset create -exist otbr-ingress-deny-src hash:net family inet6 +# Create swap set for atomic updates ipset create -exist otbr-ingress-deny-src-swap hash:net family inet6 +# Allow specific destination networks ipset create -exist otbr-ingress-allow-dst hash:net family inet6
Line range hint
98-113: Consider improving port validation.While the port handling is generally good, consider adding validation for port availability before starting the service.
Here's a suggested improvement:
if ! bashio::var.has_value "${otbr_rest_listen_port}"; then otbr_rest_listen="$(bashio::addon.ip_address)" otbr_rest_listen_port="8081" elif [ "${otbr_rest_listen_port}" != "8081" ]; then bashio::log.warning "Custom OpenThread REST API port is not supported. Using 8081." otbr_rest_listen_port="8081" +else + # Check if port is already in use + if netstat -ln | grep -q ":${otbr_rest_listen_port} "; then + bashio::log.warning "Port ${otbr_rest_listen_port} is already in use. Service might fail to start." + fi fi
Line range hint
117-122: Consider documenting the timeout values.The service execution is well-configured, but the s6-notifyoncheck timeout values (300 seconds) should be documented to explain why this specific duration was chosen.
Add a comment explaining the timeout values:
+# Use 5-minute timeout for both startup and readiness checks +# as Thread network formation and backbone interface setup might take time exec s6-notifyoncheck -d -s 300 -w 300 -n 0 \
Variables marked as unused are actually unused in both scripts
The variables
device,baudrate, andflow_controlare declared but never used in either the main script or the sourced common script. These variables can be safely removed as they serve no purpose.
silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/otbr-agent/run: Remove the following unused variable declarations:# shellcheck disable=SC2034 declare device # shellcheck disable=SC2034 declare baudrate # shellcheck disable=SC2034 declare flow_control🔗 Analysis chain
Line range hint
2-20: LGTM! Verify unused variables.The script setup and variable declarations follow best practices. The shellcheck directives are properly used to suppress specific warnings.
Let's verify if the "unused" variables are actually used in the sourced script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the variables are used in the common script rg -A 5 "device|baudrate|flow_control" "/etc/s6-overlay/scripts/otbr-agent-common"Length of output: 164
Script:
#!/bin/bash # Let's first find the correct path to the common script fd "otbr-agent-common" # Then search for variable usage in all script files rg -A 5 "device|baudrate|flow_control" $(fd -t f -e sh)Length of output: 5488
Script:
#!/bin/bash # Let's check the content of the common script specifically cat silabs-multiprotocol/rootfs/etc/s6-overlay/scripts/otbr-agent-common # Also check if these variables are used in the run script itself cat silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/otbr-agent/runLength of output: 4660
letsencrypt/rootfs/etc/services.d/lets-encrypt/run (5)
Line range hint
137-137: Correct the function name for requiring TransIP usernameThe function
bashio::config.require.usernameis not valid. It should bebashio::config.require 'dns.transip_username'.Apply this diff to fix the issue:
-bashio::config.require.username 'dns.transip_username' +bashio::config.require 'dns.transip_username'
Line range hint
85-90: Add DNS challenge check for Cloudflare providerFor consistency and to prevent potential misconfigurations, add the check for
${CHALLENGE} == "dns"in the Cloudflare provider block, similar to other DNS providers.Apply this diff:
-elif [ "${DNS_PROVIDER}" == "dns-cloudflare" ]; then +elif [ "${CHALLENGE}" == "dns" ] && [ "${DNS_PROVIDER}" == "dns-cloudflare" ]; then
Line range hint
86-95: Require Cloudflare email and API key when token is absentWhen
dns.cloudflare_api_tokenis not provided, the script usesdns.cloudflare_emailanddns.cloudflare_api_keywithout ensuring they are set. Addbashio::config.requirestatements to prevent runtime errors due to missing configurations.Apply this diff:
else + bashio::config.require 'dns.cloudflare_email' + bashio::config.require 'dns.cloudflare_api_key' bashio::log.warning "Use CloudFlare global key (not recommended!)" echo -e "dns_cloudflare_email = $(bashio::config 'dns.cloudflare_email')\n" \ "dns_cloudflare_api_key = $(bashio::config 'dns.cloudflare_api_key')\n" >> "/data/dnsapikey"
Line range hint
215-221: Use consistent credential file naming for Azure providerIn the Azure provider block, the credentials file is copied to
/data/azure_creds, which may cause conflicts if multiple credentials are used. Consider using the original filename to maintain consistency and flexibility.Apply this diff:
- cp -f "/share/${AZURE_CREDS}" "/data/azure_creds" - chmod 600 "/data/azure_creds" + cp -f "/share/${AZURE_CREDS}" "/data/${AZURE_CREDS}" + chmod 600 "/data/${AZURE_CREDS}"Update the provider arguments accordingly:
-PROVIDER_ARGUMENTS+=("--authenticator" "${DNS_PROVIDER}" "--${DNS_PROVIDER}-config" "/data/azure_creds") +PROVIDER_ARGUMENTS+=("--authenticator" "${DNS_PROVIDER}" "--${DNS_PROVIDER}-config" "/data/${AZURE_CREDS}")
Line range hint
95-104: Ensure at least one Gandi credential is providedThe Gandi provider block does not require any credentials, which may lead to configuration errors. Ensure that at least one of
dns.gandi_sharing_id,dns.gandi_token, ordns.gandi_api_keyis provided.Consider adding the following check:
if ! bashio::config.has_value 'dns.gandi_sharing_id' && \ ! bashio::config.has_value 'dns.gandi_token' && \ ! bashio::config.has_value 'dns.gandi_api_key'; then bashio::log.fatal "At least one Gandi credential must be provided." exit 1 fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (19)
assist_microphone/rootfs/etc/s6-overlay/s6-rc.d/assist_microphone/finish(2 hunks)configurator/rootfs/etc/s6-overlay/s6-rc.d/configurator/finish(2 hunks)duckdns/rootfs/etc/s6-overlay/s6-rc.d/duckdns/finish(2 hunks)letsencrypt/rootfs/etc/services.d/lets-encrypt/run(2 hunks)nginx_proxy/rootfs/etc/s6-overlay/s6-rc.d/crond/finish(2 hunks)nginx_proxy/rootfs/etc/s6-overlay/s6-rc.d/nginx/finish(2 hunks)nginx_proxy/rootfs/etc/s6-overlay/s6-rc.d/nginx/run(1 hunks)openthread_border_router/rootfs/etc/s6-overlay/s6-rc.d/otbr-agent/finish(4 hunks)openthread_border_router/rootfs/etc/s6-overlay/scripts/universal-silabs-flasher-up(3 hunks)openwakeword/rootfs/etc/s6-overlay/s6-rc.d/openwakeword/finish(2 hunks)openwakeword/rootfs/etc/s6-overlay/s6-rc.d/openwakeword/run(1 hunks)piper/rootfs/etc/s6-overlay/s6-rc.d/piper/finish(2 hunks)piper/rootfs/etc/s6-overlay/s6-rc.d/piper/run(1 hunks)silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/cpcd/finish(1 hunks)silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/otbr-agent/run(4 hunks)silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/zigbeed/run(2 hunks)silabs_flasher/rootfs/etc/s6-overlay/scripts/universal-silabs-flasher-up(3 hunks)whisper/rootfs/etc/s6-overlay/s6-rc.d/whisper/finish(2 hunks)whisper/rootfs/etc/s6-overlay/s6-rc.d/whisper/run(1 hunks)
🔇 Additional comments (28)
silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/cpcd/finish (2)
1-2: LGTM! Shell directive enables proper linting.
The addition of # shellcheck shell=bash directive properly enables shellcheck to lint this script despite the custom shebang, which aligns with the PR objectives.
1-13: Verify consistent error handling across services.
Let's ensure this error handling pattern is consistent with other critical services in the system.
✅ Verification successful
Error handling is appropriately unique to each service's requirements
After analyzing the finish scripts across all services in the multiprotocol add-on:
- The
cpcdservice's error handling (halting on non-zero/non-256 exits) is appropriately unique to its critical role as the Co-Processor Communication daemon - Other services have different requirements:
otbr-agenthas extensive cleanup of firewall rules and custom exit code handlingotbr-web,zigbeed, andmdnsonly log their exit status without container termination
This variation in error handling is correct by design, as each service has different responsibilities and failure impacts on the system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check error handling patterns in other finish scripts
# Expected: Similar exit code handling patterns in other critical services
# Find and examine other finish scripts
fd finish "rootfs/etc/s6-overlay/s6-rc.d" --type f --exec sh -c '
echo "=== {} ==="
cat "{}"
echo
'
# Look for documentation about service behavior
fd . -e md -e txt --exec grep -l -i "cpcd\|silicon labs\|multiprotocol" {} \;
Length of output: 503
Script:
#!/bin/bash
# Let's adjust the path to check finish scripts
fd finish "silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d" --type f --exec sh -c '
echo "=== {} ==="
cat "{}"
echo
'
# Let's also check the documentation specifically for cpcd service behavior
cat silabs-multiprotocol/DOCS.md
Length of output: 8797
openwakeword/rootfs/etc/s6-overlay/s6-rc.d/openwakeword/run (1)
Line range hint 12-18: Consider security and operational improvements
A few architectural considerations:
- The service is binding to all interfaces (0.0.0.0). Consider restricting this to specific interfaces if external access isn't required.
- No health check mechanism is visible. Consider adding a health check to ensure the service is functioning correctly.
- No resource limits are specified. Consider adding memory and CPU limits to prevent resource exhaustion.
Let's verify if this service is meant to be externally accessible:
silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/zigbeed/run (3)
1-2: LGTM! Proper shell specification for linting.
The addition of # shellcheck shell=bash directive ensures proper linting while maintaining the required custom shebang for the Home Assistant add-on environment.
19-22: Verify radio URL configuration and parameter handling.
The unquoted variables for listen parameters are intentionally left unquoted (hence SC2086 suppression), but we should verify the radio URL configuration.
Let's check for consistency in radio URL usage:
#!/bin/bash
# Check for other instances of radio URL configuration
rg "spinel\+cpc://" --type sh
# Look for any documentation or configuration files that specify the radio URL format
fd -e md -e yaml -e conf -X rg "radio.*url"Line range hint 9-16: LGTM! Secure network configuration with proper fallback.
The network configuration logic is well-implemented with appropriate security considerations. When no user port is specified, it defaults to listening only on the local interface.
Let's verify the port configuration handling:
✅ Verification successful
Port configuration is correctly implemented and documented
The verification confirms that port 9999 is:
- Properly configured in
config.yamlas a configurable TCP port - Well-documented in translations as "EmberZNet EZSP/ASH port"
- Consistently used as the default fallback in the zigbeed service
- Referenced in patches and documentation for TCP socket implementation
The network configuration logic is secure and follows best practices:
- Default listening on all interfaces (::) only when user configures the port
- Falls back to local-only interface with default port 9999 when unconfigured
- Port configuration is properly exposed through the add-on configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any other instances where port 9999 is hardcoded
rg -l "9999" --type sh
# Check for consistent port configuration patterns across other services
ast-grep --pattern 'bashio::addon.port $_'
Length of output: 77
Script:
#!/bin/bash
# Let's try a more comprehensive search for port configuration
rg "9999" -A 2 -B 2
# Search for port configuration in YAML files (addon configuration)
fd -e yaml -X rg "9999|ports?"
# Look for network configuration patterns
rg "ezsp_listen" -A 2 -B 2
Length of output: 10209
nginx_proxy/rootfs/etc/s6-overlay/s6-rc.d/crond/finish (2)
18-18: Correct arithmetic evaluation syntax
The change from $exit_code_signal to exit_code_signal in the arithmetic context is correct. In bash arithmetic evaluation ($(( ))), variables don't require the $ prefix.
Line range hint 6-26: Verify error handling in the supervision tree
The script's error handling logic appears robust, but let's verify the integration with the S6 supervision tree:
- It properly captures container and service exit codes
- Handles signal 15 (SIGTERM) specially
- Propagates non-zero exit codes correctly
- Initiates system halt when needed
Let's verify the error handling patterns across similar service finish scripts:
✅ Verification successful
Error handling is consistent across S6 supervision tree
The verification shows that the error handling in the crond finish script follows the standard pattern used across multiple services in the codebase. All finish scripts that implement error handling use identical logic for:
- Capturing container and service exit codes
- Special handling of SIGTERM (signal 15)
- Propagating exit codes to container results
- Initiating system halt on errors
The implementation is verified to be correct and consistent with other services like whisper, samba, nginx, mariadb, etc.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in error handling across finish scripts
# Look for other finish scripts with similar patterns
# Find all finish scripts in s6-rc.d directories
fd --type f '^finish$' --exec sh -c '
echo "=== {} ==="
# Check for exit code handling patterns
rg -A 2 "exit_code_(container|service|signal)" {} || true
echo ""
'
# Verify halt command usage
rg --type sh "exec.*/run/s6/basedir/bin/halt"
Length of output: 10679
nginx_proxy/rootfs/etc/s6-overlay/s6-rc.d/nginx/finish (3)
Line range hint 1-9: LGTM! Well-structured variable declarations.
Good practices observed:
- Using
readonlyfor constants - Explicit shellcheck configuration
- Necessary shellcheck disable for the container exit code reading
Line range hint 11-14: LGTM! Clear and structured logging implementation.
The logging provides good visibility into service termination with all relevant exit details.
Line range hint 16-25: LGTM! Robust exit code handling with proper signal propagation.
The implementation correctly handles:
- Signal termination (exit code 256)
- Error conditions (non-zero exit)
- Clean shutdown (SIGTERM)
Let's verify that similar exit code handling is consistent across other service finish scripts:
✅ Verification successful
Exit code handling is consistent across all service finish scripts
The verification shows that the exit code handling pattern in nginx_proxy's finish script is consistently implemented across multiple services including assist_microphone, configurator, duckdns, mariadb, nginx_proxy, openwakeword, piper, samba, and whisper. All scripts follow the same robust pattern:
- Signal termination (exit code 256)
- Error conditions (non-zero exit)
- Clean shutdown (SIGTERM)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in exit code handling across finish scripts
# Look for other finish scripts and compare their exit code handling patterns
# Find all finish scripts
fd finish 'rootfs/etc/s6-overlay/s6-rc.d'
# Check exit code handling patterns in finish scripts
rg -A 5 'exit_code_signal' 'rootfs/etc/s6-overlay/s6-rc.d/*/finish'
Length of output: 305
Script:
#!/bin/bash
# Let's first locate the correct path by searching from root
fd -t f finish
# Once we find the paths, let's examine the exit code handling
rg -A 5 'exit_code_' $(fd -t f finish)
Length of output: 24336
whisper/rootfs/etc/s6-overlay/s6-rc.d/whisper/finish (2)
Line range hint 1-6: LGTM! Well-documented script header
The script header is properly configured with:
- Appropriate shebang for s6-overlay environment
- Explicit shellcheck shell directive
- Clear documentation with reference to s6-overlay docs
Line range hint 13-16: LGTM! Clear logging implementation
Good use of structured logging with bashio to capture both the exit code and signal information.
assist_microphone/rootfs/etc/s6-overlay/s6-rc.d/assist_microphone/finish (2)
Line range hint 12-15: LGTM! Good logging practices
The logging implementation follows best practices by using structured logging and providing clear context about the service state.
Line range hint 1-26: Verify consistency with other S6 finish scripts
Let's ensure this implementation is consistent with other S6 finish scripts in the repository.
✅ Verification successful
Let me try a different approach to locate and verify the S6 scripts.
Implementation is consistent with S6 finish scripts across the repository
The verification shows that the finish script implementation in assist_microphone follows the standard pattern used across the repository's S6-managed services. Key observations:
- The same exit code handling logic is consistently used in other services like
configurator,duckdns,nginx_proxy,openwakeword,piper, andwhisper - The script properly handles both service exit codes and signals in line with S6 overlay requirements
- The implementation matches the reference pattern for container shutdown behavior
The script is well-integrated with the S6 supervision tree and maintains consistency with other services in the repository.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find and compare all S6 finish scripts
echo "Searching for other S6 finish scripts..."
fd --type f "finish$" "rootfs/etc/s6-overlay/s6-rc.d" | while read -r script; do
echo "=== $script ==="
# Check for consistent shebang
head -n 1 "$script"
# Check for consistent exit code handling
grep -A 5 "exit_code_service" "$script" || true
echo
done
Length of output: 278
Script:
#!/bin/bash
# First, let's find the correct path structure
fd --type f finish
# Then check the content of found files
for script in $(fd --type f finish); do
echo "=== $script ==="
head -n 1 "$script"
grep -A 5 "exit_code_service" "$script" || true
echo
done
Length of output: 22643
configurator/rootfs/etc/s6-overlay/s6-rc.d/configurator/finish (2)
Line range hint 1-1: Verify script permissions
Ensure this critical s6-overlay script has the correct executable permissions.
#!/bin/bash
# Description: Check permissions of s6 finish scripts
# Find the finish script and verify its permissions
fd finish "configurator/rootfs/etc/s6-overlay/s6-rc.d" --type f --exec stat -c "%a %n" {}Line range hint 18-26: LGTM! Proper signal handling implementation
The exit code calculation follows POSIX standards for signal handling (128 + signal_number). The logic properly manages:
- Signal terminations (exit 256)
- Normal service failures (non-zero exit)
- Graceful exits
Let's verify other similar scripts follow the same pattern:
✅ Verification successful
Signal handling implementation is unique to this service
The signal handling implementation (128 + signal) appears to be specific to the configurator service. This is expected as:
- The directory structure shows only two services:
configuratoranduser - The signal handling pattern (128 + exit_code_signal) is only found in
configurator/finish - The
userservice appears to be a container for service dependencies rather than an actual service
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in signal handling across s6 finish scripts
# Find all finish scripts and check their signal handling implementation
fd finish "rootfs/etc/s6-overlay/s6-rc.d" --type f --exec sh -c '
echo "=== {} ==="
grep -A 5 "exit_code_signal" "{}" || true
echo
'
Length of output: 306
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to find signal handling patterns
# across finish scripts in the s6 overlay directory
rg -A 5 "exit_code_signal" "configurator/rootfs/etc/s6-overlay/s6-rc.d" --type sh
Length of output: 79
Script:
#!/bin/bash
# Let's try one more time with a broader search in the configurator directory
# to find all finish scripts and their signal handling patterns
# First, let's see what files exist in the s6-rc.d directory
ls -R configurator/rootfs/etc/s6-overlay/s6-rc.d/
# Then search for signal handling patterns in shell scripts
rg -A 5 "128 \+" configurator/rootfs/etc/s6-overlay/
Length of output: 1183
openwakeword/rootfs/etc/s6-overlay/s6-rc.d/openwakeword/finish (2)
Line range hint 12-14: LGTM! Well-structured logging implementation.
The logging statement provides clear visibility into service termination with both exit code and signal information.
Line range hint 16-25: Verify signal handling implementation across services.
The signal handling logic follows Unix standards (128 + signal number). Let's verify this implementation is consistent across other service finish scripts.
✅ Verification successful
Signal handling implementation is consistent across services
After analyzing all finish scripts in the repository, I can confirm that the signal handling implementation is consistent and follows Unix standards. The scripts follow two main patterns:
- Full signal handling (like in openwakeword, piper, whisper, etc.):
if [[ "${exit_code_service}" -eq 256 ]]; then
if [[ "${exit_code_container}" -eq 0 ]]; then
echo $((128 + exit_code_signal)) > /run/s6-linux-init-container-results/exitcode
fi
[[ "${exit_code_signal}" -eq 15 ]] && exec /run/s6/basedir/bin/halt- Simplified signal handling (like in mosquitto, dnsmasq, etc.):
if [[ "$1" -ne 0 ]] && [[ "$1" -ne 256 ]]; then
bashio::log.warning "Halt add-on"
exec /run/s6/basedir/bin/halt
fiBoth patterns correctly handle signal termination (256) and follow the Unix standard of using 128 + signal number for the exit code. The implementation in openwakeword is consistent with other critical services in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent signal handling across finish scripts
# Expected: All finish scripts should handle signals similarly
# Find all finish scripts and check their signal handling implementation
fd finish "rootfs/etc/s6-overlay/s6-rc.d" --type f --exec sh -c '
echo "=== {} ==="
# Look for signal handling patterns
rg -A 2 "exit_code_signal" {} || true
echo
'
Length of output: 289
Script:
#!/bin/bash
# Let's first locate the finish scripts in the repository
fd finish --type f
# Then examine their content
fd finish --type f --exec sh -c '
echo "=== {} ==="
cat {}
echo -e "\n"
'
Length of output: 30101
piper/rootfs/etc/s6-overlay/s6-rc.d/piper/finish (2)
Line range hint 1-6: LGTM! Well-documented script header
The shebang and documentation comments provide clear context about the script's purpose and relevant documentation links.
Line range hint 13-16: LGTM! Clear and informative logging
Good use of structured logging to capture both the exit code and signal information.
duckdns/rootfs/etc/s6-overlay/s6-rc.d/duckdns/finish (2)
Line range hint 7-11: LGTM! Good use of readonly declarations.
The shellcheck disable for SC2155 is appropriate here since the command substitution is guaranteed to succeed in a running container.
Line range hint 18-26: Verify consistent error handling across other service finish scripts.
The error handling logic here is robust. Let's ensure this pattern is consistently applied across other service finish scripts in the repository.
✅ Verification successful
Error handling pattern is consistently implemented across service finish scripts
The verification shows that the error handling pattern is consistently implemented across the repository's service finish scripts. Key findings:
- All s6-rc.d finish scripts follow the same robust pattern:
- Handle signal-based exits (code 256)
- Special handling for SIGTERM (signal 15)
- Preserve first error by checking container exit code
- Use
exec /run/s6/basedir/bin/haltfor abnormal termination
The error handling in the duckdns finish script follows the established best practices used throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in error handling across finish scripts
# Expected results: Similar error handling patterns in other finish scripts
# Find all finish scripts
echo "Searching for finish scripts with similar error handling patterns..."
rg -l "s6-linux-init-container-results/exitcode" --type bash
# Check for consistent signal handling
echo -e "\nChecking signal handling consistency..."
rg "exit_code_signal" --type bash -C 2
# Check for consistent halt command usage
echo -e "\nChecking halt command usage..."
rg "exec.*halt" --type bash -C 2
Length of output: 540
Script:
#!/bin/bash
# Let's try without the --type bash flag since it's not recognized
# and use more generic pattern matching
# Find all finish scripts
echo "Searching for finish scripts with error handling patterns..."
rg -l "s6-linux-init-container-results/exitcode"
# Check for consistent signal handling
echo -e "\nChecking signal handling consistency..."
rg "exit_code_signal" -C 2
# Check for consistent halt command usage
echo -e "\nChecking halt command usage..."
rg "exec.*halt" -C 2
# Additionally, let's find all finish scripts by path pattern
echo -e "\nLocating finish scripts by path..."
fd "finish$" --type f
Length of output: 34646
openthread_border_router/rootfs/etc/s6-overlay/s6-rc.d/otbr-agent/finish (1)
Line range hint 58-66: LGTM: Proper S6 error handling implementation
The error handling follows S6 supervision tree conventions correctly:
- Writes exit code to the proper location
- Uses exit code 125 to indicate permanent failure
- Includes helpful documentation
openthread_border_router/rootfs/etc/s6-overlay/scripts/universal-silabs-flasher-up (2)
Line range hint 2-13: LGTM! Well-structured script setup.
Good practices observed:
- Explicit shell directive for shellcheck
- Clear variable declarations upfront
- Proper use of shellcheck directives
Line range hint 55-69: Consider making firmware versions configurable.
While the device detection logic is solid, hardcoded firmware versions might make updates difficult.
Consider moving firmware versions to a configuration file:
+ # Source firmware versions from config
+ source /etc/universal-silabs-flasher/firmware.conf
+
if [[ "${usb_manufacturer}" == "Nabu Casa" && "${usb_product}" == "SkyConnect"* ]]; then
- firmware="skyconnect_openthread_rcp_2.4.4.0_GitHub-7074a43e4_gsdk_4.4.4.gbl"
+ firmware="${SKYCONNECT_FIRMWARE}"Let's verify supported devices against documentation:
silabs_flasher/rootfs/etc/s6-overlay/scripts/universal-silabs-flasher-up (1)
Line range hint 20-32: Well-implemented cleanup and exit handling!
The cleanup function properly handles:
- Exit status logging
- S6-overlay exit code reporting
- Graceful shutdown via halt
silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/otbr-agent/run (1)
55-60: LGTM! Good error handling.
The fallback to "eth0" when no primary interface is found is a reasonable safety measure. The warning message is clear and informative.
assist_microphone/rootfs/etc/s6-overlay/s6-rc.d/assist_microphone/finish
Show resolved
Hide resolved
openthread_border_router/rootfs/etc/s6-overlay/scripts/universal-silabs-flasher-up
Show resolved
Hide resolved
openthread_border_router/rootfs/etc/s6-overlay/scripts/universal-silabs-flasher-up
Show resolved
Hide resolved
silabs_flasher/rootfs/etc/s6-overlay/scripts/universal-silabs-flasher-up
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (113)
dnsmasq/rootfs/etc/services.d/dnsmasq/run (1)
Line range hint
11-11: Document the purpose of /dev/null redirection.The redirection
< /dev/nullshould be documented to explain its purpose in preventing potential stdin-related issues.-exec dnsmasq -C "${CONFIG}" -z < /dev/null +# Redirect from /dev/null to ensure dnsmasq doesn't inherit stdin +exec dnsmasq -C "${CONFIG}" -z < /dev/nullvlc/rootfs/etc/cont-init.d/secret.sh (3)
Line range hint
7-8: Add error handling for file permission issuesThe file existence check should also verify that the file is readable and has appropriate permissions.
-if bashio::fs.file_exists /data/secret; then +if bashio::fs.file_exists /data/secret && [[ -r /data/secret ]]; then bashio::exit.ok +else + bashio::log.debug "Secret file does not exist or is not readable" fi
Line range hint
11-11: Enhance security of password file creationThe script should:
- Set restrictive permissions on the secret file
- Verify pwgen's success
- Validate the generated password
-pwgen 64 1 > /data/secret +if ! pwgen -s 64 1 > /data/secret; then + bashio::exit.nok "Failed to generate password" +fi + +# Secure the file permissions +chmod 600 /data/secret + +# Validate password was written +if ! [[ -s /data/secret ]]; then + bashio::exit.nok "Failed to write password to file" +fiNote: Added
-sflag to pwgen for more secure random passwords.
Line range hint
1-12: Consider implementing password rotation mechanismThe current implementation only generates a password if it doesn't exist. Consider adding a mechanism for password rotation, especially for security-sensitive deployments.
Would you like me to propose an implementation for a password rotation mechanism?
deconz/rootfs/etc/services.d/nginx/run (2)
Line range hint
8-11: Add error handling and improve loggingThe script could benefit from better error handling and logging:
- No error handling if the port wait times out
- No verification that nginx started successfully
- Limited logging around the startup process
Consider applying these improvements:
# Wait for deCONZ to start before continuing -bashio::net.wait_for 40850 +if ! bashio::net.wait_for 40850; then + bashio::log.error "deCONZ failed to start on port 40850" + exit 1 +fi bashio::log.info "Starting Nginx..." -exec nginx +if ! exec nginx; then + bashio::log.error "Failed to start Nginx" + exit 1 +fi
Line range hint
10-11: Verify nginx configuration before startingConsider adding a configuration test before starting nginx to catch any potential issues early.
Add this before starting nginx:
bashio::log.info "Starting Nginx..." +if ! nginx -t; then + bashio::log.error "Nginx configuration test failed" + exit 1 +fi exec nginxdeconz/rootfs/etc/cont-init.d/novnc.sh (1)
Line range hint
6-8: Add error handling and validationThe script could benefit from additional error handling and validation:
- Validate the
ingress_entryvalue is not empty- Add error handling for the
sedoperation- Verify the HTML file exists before modification
- Consider creating a backup before in-place modification
Here's a suggested improvement:
declare ingress_entry ingress_entry=$(bashio::addon.ingress_entry) -sed -i "s#websockify#${ingress_entry#?}/novnc/websockify#g" /usr/share/novnc/vnc_lite.html + +if [[ -z "${ingress_entry}" ]]; then + bashio::exit.nok "Failed to get ingress entry" +fi + +HTML_FILE="/usr/share/novnc/vnc_lite.html" +if [[ ! -f "${HTML_FILE}" ]]; then + bashio::exit.nok "noVNC HTML file not found: ${HTML_FILE}" +fi + +if ! sed -i.backup "s#websockify#${ingress_entry#?}/novnc/websockify#g" "${HTML_FILE}"; then + bashio::exit.nok "Failed to update noVNC configuration" +fisilabs-multiprotocol/rootfs/etc/s6-overlay/scripts/socat-cpcd-tcp-enable-check.sh (1)
Line range hint
7-11: Consider adding error handling and idempotency checksWhile the logic is straightforward, consider these improvements for better robustness:
- Add error handling for file creation operations
- Add idempotency checks to prevent duplicate operations
- Consider using variables for repeated path segments
Here's a suggested improvement:
+readonly S6_PATH="/etc/s6-overlay/s6-rc.d" + if bashio::config.has_value 'network_device'; then - touch /etc/s6-overlay/s6-rc.d/user/contents.d/socat-cpcd-tcp - touch /etc/s6-overlay/s6-rc.d/cpcd/dependencies.d/socat-cpcd-tcp - bashio::log.info "Enabled socat-cpcd-tcp." + if ! touch "${S6_PATH}/user/contents.d/socat-cpcd-tcp" || \ + ! touch "${S6_PATH}/cpcd/dependencies.d/socat-cpcd-tcp"; then + bashio::log.error "Failed to enable socat-cpcd-tcp service" + exit 1 + fi + bashio::log.info "Enabled socat-cpcd-tcp" fisilabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/socat-cpcd-tcp/run (1)
Line range hint
7-9: Add validation for network_device configuration.The script should validate the network_device configuration before attempting to use it in the TCP connection. A missing or invalid value could cause silent failures.
Consider adding validation:
declare network_device network_device=$(bashio::config 'network_device') +if ! bashio::config.has_value 'network_device'; then + bashio::log.error "Network device configuration is required" + exit 1 +fissh/rootfs/etc/services.d/sshd/run (2)
Line range hint
8-11: Consider adding error handling for port validationWhile the port check is functional, consider adding explicit validation for port configuration errors.
if ! bashio::var.has_value "$(bashio::addon.port 22)"; then bashio::log.warning "SSH port is disabled. Prevent start of SSH server." + # Exit with success as this is a valid configuration state exec sleep infinity +else + # Validate port is within valid range + port=$(bashio::addon.port 22) + if ! bashio::var.is_number "${port}" || [ "${port}" -lt 1 ] || [ "${port}" -gt 65535 ]; then + bashio::log.error "Invalid SSH port configuration: ${port}" + exit 1 + fi fi
Line range hint
1-14: Consider adding S6 notification readinessFor better integration with S6 supervision, consider notifying when the service is ready.
bashio::log.info "Starting the SSH daemon..." +# Notify supervisor that service is ready +s6-notifyoncheck -d -n 300 -w 1000 \ exec /usr/sbin/sshd -D -eThis ensures proper service orchestration and dependency management in the S6 supervision tree.
zwave_js/rootfs/etc/cont-init.d/structure.sh (1)
Line range hint
6-16: Add error handling for directory operationsThe directory operations could benefit from additional error handling and safety checks:
- Directory creation should verify success
- Migration operation should handle potential failures
- Consider adding appropriate permissions
Consider applying these improvements:
-mkdir -p /data/db +if ! mkdir -p /data/db; then + bashio::log.error "Failed to create database directory" + exit 1 +fi if bashio::fs.directory_exists '/data/cache'; then bashio::log.info "Migrating 'cache' folder from private storage to /addon_configs/core_zwave_js" - mv /data/cache /config/cache + if ! mv -n /data/cache /config/cache; then + bashio::log.error "Failed to migrate cache directory" + exit 1 + fi fi if ! bashio::fs.directory_exists '/config/cache'; then - mkdir -p /config/cache + if ! mkdir -p /config/cache; then + bashio::log.error "Failed to create cache directory" + exit 1 + fi + chmod 755 /config/cache fissh/rootfs/etc/cont-init.d/apks.sh (2)
Line range hint
11-17: Enhance error handling and reporting.The current implementation exits on the first failure. Consider collecting all failed packages and reporting them together for better user experience.
Here's a suggested improvement:
+declare -a failed_packages=() for package in $(bashio::config "apks"); do - apk add "$package" \ - || bashio::exit.nok "Failed installing ${package}" + if ! apk add "$package"; then + failed_packages+=("$package") + bashio::log.error "Failed installing ${package}" + fi done + +if [ ${#failed_packages[@]} -ne 0 ]; then + bashio::exit.nok "Failed installing packages: ${failed_packages[*]}" +fi + +# Clean up +rm -rf /var/cache/apk/*
Based on the gathered information, I can now provide the final response:
Package installation security concerns require attention
The current implementation has some built-in security measures but could be improved:
- The add-on runs in a container with limited privileges
- Documentation explicitly states it won't allow root-level package installations
- However, the package installation process still lacks:
- Package name validation/sanitization
- No allowlist/blocklist for packages
- No verification of package signatures or sources
- No cleanup of package cache after installation
Recommendations:
- Add package name validation using alphanumeric and hyphen checks
- Implement an allowlist of safe packages
- Add package signature verification
- Clean up package cache after installation
- Add logging of installed packages for audit purposes
🔗 Analysis chain
Line range hint
7-17: Consider adding security constraints and improving robustness.The current implementation allows installation of arbitrary packages without validation, which could pose security risks. Additionally, the package management process could be more robust.
Consider these improvements:
- Add package name validation
- Implement package allowlist/blocklist
- Add package cache cleanup
- Handle concurrent apk operations
Let's check if there are any security measures in place elsewhere:
Let me gather more information about the configuration schema and validation:
Let me check for any existing package validation or restrictions in the configuration:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for package restrictions in other configuration files echo "Searching for package-related configuration..." rg -l "apks?" config.* || true rg -l "packages?" config.* || true # Check for other scripts using apk echo "Checking other apk usage patterns..." rg "apk (add|update)" || trueLength of output: 1721
Script:
#!/bin/bash # Check for config schema echo "Checking for config schema..." fd config.json --exec cat {} \; # Check for documentation echo -e "\nChecking documentation..." fd "DOCS.md|README.md" --exec cat {} \; # Check for other security measures echo -e "\nChecking for security measures..." rg -i "security|validation|sanitiz|restrict" ssh/Length of output: 168273
Script:
#!/bin/bash # Check for package validation in config files echo "Checking config files..." fd "config.json|config.yaml" ssh/ --exec cat {} \; # Check for package restrictions in init scripts echo -e "\nChecking init scripts..." fd "^init" ssh/ --exec cat {} \; # Check for any package-related security measures echo -e "\nChecking for package security patterns..." rg -A 5 -B 5 "apk|package" ssh/Length of output: 6356
vlc/rootfs/etc/s6-overlay/s6-rc.d/vlc/run (1)
Line range hint
9-13: Consider error handling for VLC startupThe script should handle potential VLC startup failures gracefully within the S6 supervision tree.
Consider adding error handling:
# Run daemon -exec cvlc -I http --no-dbus --extraintf telnet --http-host 127.0.0.1 --http-password "INGRESS" --telnet-password "${PASSWORD}" --aout=pulse +if ! exec cvlc -I http --no-dbus --extraintf telnet --http-host 127.0.0.1 \ + --http-password "INGRESS" --telnet-password "${PASSWORD}" --aout=pulse +then + bashio::log.error "Failed to start VLC" + exit 1 +fiAlso, consider splitting the long command line for better readability.
openthread_border_router/rootfs/etc/s6-overlay/scripts/otbr-agent-configure.sh (2)
Line range hint
7-11: Add error handling for ot-ctl commandsWhile the NAT64 configuration logic is correct, the script should handle potential failures of the
ot-ctlcommands to ensure robust operation.Consider applying this improvement:
if bashio::config.true 'nat64'; then bashio::log.info "Enabling NAT64." - ot-ctl nat64 enable - ot-ctl dns server upstream enable + if ! ot-ctl nat64 enable; then + bashio::log.error "Failed to enable NAT64" + exit 1 + fi + if ! ot-ctl dns server upstream enable; then + bashio::log.error "Failed to enable DNS server upstream" + exit 1 + fi fi
Line range hint
14-15: Improve TX power configuration robustnessWhile the TX power configuration is well-documented, consider these improvements:
- Add error handling for the
ot-ctlcommand- Consider making the TX power value configurable through add-on configuration
Here's a suggested implementation:
# To avoid asymmetric link quality the TX power from the controller should not # exceed that of what other Thread routers devices typically use. -ot-ctl txpower 6 +# Use configured TX power or default to 6 +TX_POWER="$(bashio::config 'tx_power' 6)" +if ! ot-ctl txpower "${TX_POWER}"; then + bashio::log.error "Failed to set TX power to ${TX_POWER}" + exit 1 +finginx_proxy/rootfs/etc/periodic/daily/check_certificate_renewal (1)
Line range hint
8-14: Add error handling and improve reliabilityThe certificate renewal check needs several improvements for better reliability and observability:
- Add error handling for critical operations
- Fix potential race conditions
- Improve variable quoting
- Enhance logging
Here's the suggested implementation:
#!/command/with-contenv bashio # shellcheck shell=bash # ============================================================================== # Check certificate renewal # ============================================================================== -CERTFILE=$(bashio::config 'certfile') +if ! CERTFILE=$(bashio::config 'certfile'); then + bashio::log.error "Failed to retrieve certificate file configuration" + exit 1 +fi -if [ ! -f /tmp/certificate_timestamp ] || [[ $(stat "/ssl/${CERTFILE}" -c %y) != $(cat /tmp/certificate_timestamp) ]]; then - bashio::log.info "Reloading nginx to reload new certificate file" - stat "/ssl/${CERTFILE}" -c %y > /tmp/certificate_timestamp - nginx -c /etc/nginx.conf -s reload +# Check if certificate file exists +if [ ! -f "/ssl/${CERTFILE}" ]; then + bashio::log.error "Certificate file /ssl/${CERTFILE} not found" + exit 1 +fi + +# Get current certificate timestamp +CERT_TIME=$(stat "/ssl/${CERTFILE}" -c %y) || { + bashio::log.error "Failed to get certificate timestamp" + exit 1 +} + +# Check if certificate was renewed +if [ ! -f /tmp/certificate_timestamp ] || [[ "${CERT_TIME}" != "$(cat /tmp/certificate_timestamp 2>/dev/null)" ]]; then + bashio::log.info "Certificate renewal detected for ${CERTFILE}" + + # Update timestamp first to prevent repeated reloads on failure + echo "${CERT_TIME}" > /tmp/certificate_timestamp || { + bashio::log.error "Failed to update certificate timestamp" + exit 1 + } + + # Reload nginx + if ! nginx -c /etc/nginx.conf -s reload; then + bashio::log.error "Failed to reload nginx" + exit 1 + fi + + bashio::log.info "Successfully reloaded nginx with renewed certificate" fiKey improvements:
- Added error handling for config retrieval, file operations, and nginx reload
- Improved timestamp file race condition handling
- Added proper quoting for variables
- Enhanced logging for better observability
- Updated timestamp file before nginx reload to prevent repeated reloads on failure
silabs-multiprotocol/rootfs/etc/s6-overlay/scripts/otbr-agent-rest-discovery.sh (1)
Line range hint
13-17: Enhance error handling and logging.The current implementation discards the actual error details by redirecting output to /dev/null. Consider capturing and logging the specific error message for better debugging.
Here's a suggested improvement:
-if bashio::discovery "otbr" "${config}" > /dev/null; then +if output=$(bashio::discovery "otbr" "${config}" 2>&1); then bashio::log.info "Successfully sent discovery information to Home Assistant." else - bashio::log.error "Discovery message to Home Assistant failed!" + bashio::log.error "Discovery message to Home Assistant failed: ${output}" + exit 1 fivlc/rootfs/etc/s6-overlay/scripts/vlc-discovery (2)
Line range hint
8-13: Consider adding error handling for password file accessThe script reads the password directly from
/data/secretwithout checking if the file exists or is readable. This could lead to silent failures or unclear error messages.Consider adding error handling:
# Prepare discovery payload +if [[ ! -r /data/secret ]]; then + bashio::log.error "Cannot read password file: /data/secret" + exit 1 +fi + ha_config=$(\ bashio::var.json \ host "$(hostname)" \ port "^4212" \ password "$(cat /data/secret)" \ )
Line range hint
15-19: Consider adding specific error details for discovery failuresWhile the script logs discovery status, it doesn't provide specific error details when the discovery fails, which could make troubleshooting difficult.
Consider capturing and logging the specific error:
-if bashio::discovery "vlc_telnet" "${ha_config}" > /dev/null; then +if output=$(bashio::discovery "vlc_telnet" "${ha_config}" 2>&1); then bashio::log.info "Successfully send discovery information to Home Assistant." else - bashio::log.error "Discovery message to Home Assistant failed!" + bashio::log.error "Discovery message to Home Assistant failed: ${output}" + exit 1 fiopenthread_border_router/rootfs/etc/s6-overlay/scripts/otbr-agent-rest-discovery.sh (2)
Line range hint
15-19: Consider enhancing error handlingThe error handling could be more robust by:
- Capturing and logging specific error details
- Setting an appropriate exit code on failure
Consider this enhancement:
+# Send discovery info with enhanced error handling +if ! discovery_output=$(bashio::discovery "otbr" "${config}" 2>&1); then + bashio::log.error "Discovery message to Home Assistant failed: ${discovery_output}" + exit 1 +fi +bashio::log.info "Successfully sent discovery information to Home Assistant." -# Send discovery info -if bashio::discovery "otbr" "${config}" > /dev/null; then - bashio::log.info "Successfully sent discovery information to Home Assistant." -else - bashio::log.error "Discovery message to Home Assistant failed!" -fi
The search results reveal that the caret prefix (
^) in the port value is unique to the discovery scripts and not used in any other port configurations. In fact, theotbr-agent/runscripts explicitly enforce port 8081 without any prefix. This inconsistency, combined with the lack of error handling for the firmware version command, warrants a response.Port format is incorrect and firmware version needs error handling
The port value should not include the caret prefix as it's inconsistent with other port configurations in the codebase. Additionally, the firmware version command needs error handling.
config=$(bashio::var.json \ host "$(bashio::addon.hostname)" \ - port "^8081" \ + port "8081" \ device "$(bashio::config 'device')" \ - firmware "$(ot-ctl rcp version | head -n 1)" \ + firmware "$(ot-ctl rcp version | head -n 1 || echo 'unknown')" \ )
- The caret prefix in port value is inconsistent with usage in:
otbr-agent/runwhich uses plain "8081"enable-check.shwhich uses port without prefix- Added fallback to 'unknown' when firmware version command fails
🔗 Analysis chain
Line range hint
8-13: Verify the port value format and add error handling for firmware versionA few concerns in the configuration setup:
- The port value contains a caret prefix (
^8081) which seems unusual. Verify if this is intentional.- The firmware version command should include error handling.
Consider this improved implementation:
config=$(bashio::var.json \ host "$(bashio::addon.hostname)" \ - port "^8081" \ + port "8081" \ device "$(bashio::config 'device')" \ - firmware "$(ot-ctl rcp version | head -n 1)" \ + firmware "$(ot-ctl rcp version | head -n 1 || echo 'unknown')" \ )Let's verify the port format usage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other instances of port configuration to verify the format rg -g '!*.{md,txt}' -A 2 -B 2 'port.*8081'Length of output: 5204
zwave_js/rootfs/etc/services.d/zwave_js/run (2)
Line range hint
20-22: Consider more robust argument handlingWhile disabling SC2086 is valid here since word splitting is intended for
--mock-driver, the argument handling could be more robust.Consider this safer approach:
-# shellcheck disable=SC2086 -exec zwave-server ${SERIAL_DEVICE} --config /etc/zwave_config.json --disable-dns-sd +if bashio::config.true 'emulate_hardware'; then + exec zwave-server --mock-driver --config /etc/zwave_config.json --disable-dns-sd +else + exec zwave-server "${SERIAL_DEVICE}" --config /etc/zwave_config.json --disable-dns-sd +fiThis eliminates the need for word splitting while properly handling both cases.
Line range hint
13-13: Add error handling for discovery processWhile discovery failure shouldn't stop the main service, adding error handling would improve debuggability.
Consider this enhancement:
-./discovery & +if ! ./discovery & then + bashio::log.warning "Failed to send discovery information to Home Assistant" +fimariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-lock-post/finish (1)
Line range hint
16-24: Consider adding error handling for the unlock operationWhile the script logs the completion, it doesn't verify if the unlock operation succeeded. Consider adding error handling to ensure the command was executed successfully.
Here's a suggested improvement:
# File descriptor &4 is used as stdin for mysql, because &0 is closed after the mariadb-lock-core service is started # shellcheck disable=SC2086 -echo "UNLOCK TABLES;" > /proc/${MARIADB_LOCK_CORE_PID}/fd/4 +if ! echo "UNLOCK TABLES;" > /proc/${MARIADB_LOCK_CORE_PID}/fd/4; then + bashio::log.error "Failed to send unlock command to MariaDB" + bashio::exit.nok +fi bashio::log.info "MariaDB tables unlocked"mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-lock-post/run (2)
9-13: Enhance process validation for better reliabilityWhile the current PID validation is good, consider adding these additional checks for more robust error handling:
# shellcheck disable=SC2086 +if ! s6-svok "/run/service/mariadb-lock-core"; then + bashio::log.error "mariadb-lock-core service does not exist" + bashio::exit.nok +fi + if bashio::var.equals ${MARIADB_LOCK_CORE_PID} -1; then bashio::log.error "Invalid mariadb-lock-core process id ${MARIADB_LOCK_CORE_PID}" bashio::exit.nok +elif ! kill -0 "${MARIADB_LOCK_CORE_PID}" 2>/dev/null; then + bashio::log.error "Process ${MARIADB_LOCK_CORE_PID} is not running" + bashio::exit.nok fi
19-19: Enhance logging for better observabilityConsider making the log message more informative for debugging purposes:
-bashio::log.info "MariaDB tables locked" +bashio::log.info "MariaDB tables locked for backup (lock-core PID: ${MARIADB_LOCK_CORE_PID})"silabs-multiprotocol/rootfs/etc/s6-overlay/scripts/otbr-enable-check.sh (2)
Line range hint
7-14: Add error handling for service removal operations.While the logic is correct, the script could benefit from more robust error handling when removing service entries.
Consider adding checks and error handling:
if bashio::config.false 'otbr_enable'; then - rm /etc/s6-overlay/s6-rc.d/user/contents.d/otbr-agent - rm /etc/s6-overlay/s6-rc.d/user/contents.d/otbr-web - rm /etc/s6-overlay/s6-rc.d/user/contents.d/otbr-agent-rest-discovery - rm /etc/s6-overlay/s6-rc.d/user/contents.d/mdns + for service in otbr-agent otbr-web otbr-agent-rest-discovery mdns; do + service_path="/etc/s6-overlay/s6-rc.d/user/contents.d/${service}" + if [[ -e "${service_path}" ]]; then + if ! rm "${service_path}"; then + bashio::log.warning "Failed to remove ${service} service entry" + fi + fi + done bashio::log.info "The otbr-agent is disabled." bashio::exit.ok fi
Line range hint
16-22: Improve readability and error handling for port check logic.While the port check logic is correct, the code could benefit from improved readability and error handling.
Consider these improvements:
-if bashio::var.has_value "$(bashio::addon.port 8080)" \ - && bashio::var.has_value "$(bashio::addon.port 8081)"; then +if ( bashio::var.has_value "$(bashio::addon.port 8080)" && + bashio::var.has_value "$(bashio::addon.port 8081)" ); then bashio::log.info "Web UI and REST API port are exposed, starting otbr-web." else - rm /etc/s6-overlay/s6-rc.d/user/contents.d/otbr-web + service_path="/etc/s6-overlay/s6-rc.d/user/contents.d/otbr-web" + if [[ -e "${service_path}" ]]; then + if ! rm "${service_path}"; then + bashio::log.warning "Failed to remove otbr-web service entry" + fi + fi bashio::log.info "The otbr-web is disabled." fimariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-pre/run (1)
14-15: Consider using s6-env or s6-envdir for environment variable managementWhile the current implementation using
catwith here-strings works, S6 provides dedicated tools for environment management that might be more appropriate.Consider using either:
-cat <<<"${MARIADB_DATA}" > /var/run/s6/container_environment/MARIADB_DATA -cat <<<"${NEW_INSTALL}" > /var/run/s6/container_environment/NEW_INSTALL +s6-env -i MARIADB_DATA="${MARIADB_DATA}" NEW_INSTALL="${NEW_INSTALL}"Or creating an envdir:
+mkdir -p /var/run/s6/container_environment +echo -n "${MARIADB_DATA}" > /var/run/s6/container_environment/MARIADB_DATA +echo -n "${NEW_INSTALL}" > /var/run/s6/container_environment/NEW_INSTALLdeconz/rootfs/etc/services.d/otau-ikea/run (3)
Line range hint
8-9: Add error handling for directory creationThe mkdir command should include error handling to ensure the service fails gracefully if directory creation fails (e.g., due to permissions or disk space issues).
-mkdir -p "/data/otau" +if ! mkdir -p "/data/otau"; then + bashio::log.error "Failed to create OTAU directory" + exec sleep 18000 +fi
Line range hint
11-15: Enhance error handling and add timeoutThe curl command should include a timeout to prevent hanging, and the error logging should provide more context about the failure.
-if ! IKEA_DATA="$(curl -sL ${URL_IKEA})"; then - bashio::log.info "Can't fetch data from ikea!" +if ! IKEA_DATA="$(curl -sL --max-time 30 "${URL_IKEA}")"; then + bashio::log.error "Failed to fetch IKEA firmware data: $(curl -sL --max-time 30 -w '%{http_code}' -o /dev/null "${URL_IKEA}")"
Line range hint
17-29: Add validation and disk space checksThe firmware download process could be more robust with these improvements:
- Check available disk space before downloading
- Validate downloaded files
- Use atomic downloads to prevent partial files
+ # Check available space (assuming average firmware size of 100MB) + if [[ $(df -P "/data" | awk 'NR==2 {print $4}') -lt 102400 ]]; then + bashio::log.error "Insufficient disk space for firmware download" + continue + fi + - curl -s -L -o "${OTAU_FILE}" "${OTAU_URL}" + # Download to temporary file first + if ! curl -s -L -o "${OTAU_FILE}.tmp" "${OTAU_URL}"; then + bashio::log.error "Failed to download firmware from ${OTAU_URL}" + rm -f "${OTAU_FILE}.tmp" + continue + fi + + # Move to final location + mv "${OTAU_FILE}.tmp" "${OTAU_FILE}"openthread_border_router/rootfs/etc/s6-overlay/scripts/enable-check.sh (2)
Line range hint
3-13: Enhance error handling and logging for service managementWhile the port check logic is correct, consider these improvements for better robustness and troubleshooting:
- Add error handling for the
rmcommand- Log the specific ports that are unavailable
- Consider using
safe-rmor checking file existence before removalif bashio::var.has_value "$(bashio::addon.port 8080)" \ && bashio::var.has_value "$(bashio::addon.port 8081)"; then bashio::log.info "Web UI and REST API port are exposed, starting otbr-web." else - rm /etc/s6-overlay/s6-rc.d/user/contents.d/otbr-web - bashio::log.info "The otbr-web is disabled." + SERVICE_FILE="/etc/s6-overlay/s6-rc.d/user/contents.d/otbr-web" + if [ -f "$SERVICE_FILE" ]; then + if rm "$SERVICE_FILE"; then + bashio::log.info "The otbr-web service is disabled (ports 8080/8081 not exposed)." + else + bashio::log.error "Failed to disable otbr-web service" + exit 1 + fi + fi fi
Line range hint
15-22: Improve service enablement robustness and documentationThe service enablement logic should include error handling and better documentation of service dependencies.
if bashio::config.has_value 'network_device'; then - touch /etc/s6-overlay/s6-rc.d/user/contents.d/socat-otbr-tcp - touch /etc/s6-overlay/s6-rc.d/otbr-agent/dependencies.d/socat-otbr-tcp - bashio::log.info "Enabled socat-otbr-tcp." + # Define service files + CONTENTS_FILE="/etc/s6-overlay/s6-rc.d/user/contents.d/socat-otbr-tcp" + DEPS_FILE="/etc/s6-overlay/s6-rc.d/otbr-agent/dependencies.d/socat-otbr-tcp" + + # Create service files with error handling + if ! touch "$CONTENTS_FILE" 2>/dev/null || ! touch "$DEPS_FILE" 2>/dev/null; then + bashio::log.error "Failed to enable socat-otbr-tcp service" + exit 1 + fi + + bashio::log.info "Enabled socat-otbr-tcp service for network device: $(bashio::config 'network_device')" fisamba/rootfs/etc/s6-overlay/s6-rc.d/nmbd/finish (2)
Line range hint
6-10: Consider separating variable declarations and assignmentsWhile disabling SC2155 works, it's better to separate declarations and assignments to catch potential errors. This is especially important when reading from files that might not exist.
Consider this safer approach:
-# shellcheck disable=SC2155 -readonly exit_code_container=$(</run/s6-linux-init-container-results/exitcode) -readonly exit_code_service="${1}" -readonly exit_code_signal="${2}" -readonly service="nmbd" +declare -r service="nmbd" +declare -r exit_code_service="${1}" +declare -r exit_code_signal="${2}" + +if [[ ! -f /run/s6-linux-init-container-results/exitcode ]]; then + bashio::log.error "Container exitcode file not found" + exit 1 +fi +declare -r exit_code_container +exit_code_container=$(</run/s6-linux-init-container-results/exitcode)
Line range hint
17-25: Consider improving exit code handling robustnessWhile the logic is correct, there are a few improvements that could make the code more robust and maintainable:
- The magic number 256 should be documented
- File writes should include error handling
- The halt command execution could benefit from logging
Consider these improvements:
+# Exit code 256 indicates the process was terminated by a signal if [[ "${exit_code_service}" -eq 256 ]]; then if [[ "${exit_code_container}" -eq 0 ]]; then - echo $((128 + exit_code_signal)) > /run/s6-linux-init-container-results/exitcode + if ! echo $((128 + exit_code_signal)) > /run/s6-linux-init-container-results/exitcode; then + bashio::log.error "Failed to update container exit code" + fi fi - [[ "${exit_code_signal}" -eq 15 ]] && exec /run/s6/basedir/bin/halt + if [[ "${exit_code_signal}" -eq 15 ]]; then + bashio::log.info "Halting container due to SIGTERM" + exec /run/s6/basedir/bin/halt + fi elif [[ "${exit_code_service}" -ne 0 ]]; then if [[ "${exit_code_container}" -eq 0 ]]; then - echo "${exit_code_service}" > /run/s6-linux-init-container-results/exitcode + if ! echo "${exit_code_service}" > /run/s6-linux-init-container-results/exitcode; then + bashio::log.error "Failed to update container exit code" + fi fi + bashio::log.info "Halting container due to service failure" exec /run/s6/basedir/bin/halt fisamba/rootfs/etc/s6-overlay/s6-rc.d/smbd/finish (2)
Line range hint
6-10: Consider separating variable declarations from assignmentsWhile using
readonlyis good practice, combining declaration with assignment can mask command failures. Consider separating them:-# shellcheck disable=SC2155 -readonly exit_code_container=$(</run/s6-linux-init-container-results/exitcode) -readonly exit_code_service="${1}" -readonly exit_code_signal="${2}" -readonly service="smbd" +exit_code_container=$(</run/s6-linux-init-container-results/exitcode) +exit_code_service="${1}" +exit_code_signal="${2}" +service="smbd" +readonly exit_code_container exit_code_service exit_code_signal serviceThis approach:
- Removes the need for shellcheck disable
- Makes command failures more visible
- Still maintains the immutability of variables
Line range hint
17-24: Consider adding comments to explain the exit code logicWhile the implementation is correct, the exit code handling logic is complex and would benefit from documentation explaining:
- Why exit code 256 gets special handling
- The significance of signal 15 (SIGTERM)
- The conditions for propagating exit codes to the container
+# Exit code 256 indicates process terminated by signal if [[ "${exit_code_service}" -eq 256 ]]; then if [[ "${exit_code_container}" -eq 0 ]]; then + # Convert signal to standard exit code format (128 + signal number) echo $((128 + exit_code_signal)) > /run/s6-linux-init-container-results/exitcode fi + # Halt only if terminated by SIGTERM (15) [[ "${exit_code_signal}" -eq 15 ]] && exec /run/s6/basedir/bin/halt elif [[ "${exit_code_service}" -ne 0 ]]; then + # For other non-zero exits, propagate the exact exit code and always halt if [[ "${exit_code_container}" -eq 0 ]]; then echo "${exit_code_service}" > /run/s6-linux-init-container-results/exitcode fi exec /run/s6/basedir/bin/halt fimariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-core/finish (2)
Line range hint
6-10: LGTM with a minor suggestion for improvement.The variable declarations are well-structured and properly marked as readonly. However, consider extracting the hardcoded service name to a common configuration file for better maintainability.
-readonly service="mariadb" +# Source from a common config file +readonly service="$(bashio::addon.name)"
Signal handling inconsistency detected across service finish scripts
The analysis reveals inconsistent signal handling patterns across the repository's finish scripts:
Some services (whisper, piper, openwakeword, samba, nginx_proxy, mariadb-core, assist_microphone, duckdns, configurator) implement complete signal handling with:
- Exit code 256 handling
- SIGTERM (15) handling with halt execution
- Signal conversion (128 + signal)
Other services lack these signal handling mechanisms entirely, which could lead to inconsistent behavior during service termination.
Recommendation: Standardize signal handling across all service finish scripts by implementing the complete pattern shown in mariadb-core/finish.
🔗 Analysis chain
Line range hint
18-24: Verify the signal handling logic across different scenarios.The exit code handling looks correct, but let's verify the signal handling logic across different services to ensure consistent behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify signal handling consistency across service finish scripts # Expected: All finish scripts should handle signals similarly # Find all finish scripts in s6-rc.d directories echo "Analyzing signal handling in finish scripts..." fd --type f "finish$" --exec sh -c ' echo "\nAnalyzing: {}" # Check for signal handling patterns echo "Exit code 256 handling:" rg -A 2 "exit_code_service.+256" "{}" || echo "No explicit 256 handling" echo "Signal 15 (SIGTERM) handling:" rg "exit_code_signal.+15" "{}" || echo "No explicit SIGTERM handling" echo "128 + signal conversion:" rg "128.+exit_code_signal" "{}" || echo "No signal conversion" 'Length of output: 11447
rpc_shutdown/data/run.sh (4)
1-3: Consider enhancing error handling for this critical scriptWhile
set -eprovides basic error handling, for a script managing system shutdowns, consider adding:#!/usr/bin/with-contenv bashio # shellcheck shell=bash set -e +set -o pipefail + +# Trap errors and exit gracefully +trap 'bashio::log.error "Unexpected error in shutdown script"; exit 1' ERR
Line range hint
5-9: Add input validation to handle edge casesThe current implementation might be vulnerable to malformed input. Consider adding validation:
# Read from STDIN aliases to send shutdown while read -r input; do + # Skip empty lines + [[ -z "${input}" ]] && continue + + # Validate JSON format + if ! bashio::jq "${input}" '.' >/dev/null 2>&1; then + bashio::log.error "Invalid JSON input: ${input}" + continue + fi + # parse JSON value input=$(bashio::jq "${input}" '.') bashio::log.info "Read alias: $input"
Line range hint
11-17: Add validation for critical configuration valuesThe script should validate required configuration values before proceeding with the shutdown:
for computer in $(bashio::config 'computers|keys'); do ALIAS=$(bashio::config "computers[${computer}].alias") ADDRESS=$(bashio::config "computers[${computer}].address") CREDENTIALS=$(bashio::config "computers[${computer}].credentials") DELAY=$(bashio::config "computers[${computer}].delay") MESSAGE=$(bashio::config "computers[${computer}].message") + + # Validate required fields + if bashio::var.is_empty "${ADDRESS}" || bashio::var.is_empty "${CREDENTIALS}"; then + bashio::log.error "Missing required configuration for computer ${computer}" + continue + fi
Line range hint
19-33: Enhance shutdown command execution and error handlingThe shutdown command execution could be safer and more informative:
- if ! msg="$(net rpc shutdown -I "$ADDRESS" -U "$CREDENTIALS" -t "$DELAY" -C "$MESSAGE")"; then - bashio::log.error "Shutdown failed: $msg" + # Validate delay is a positive number + if ! [[ "$DELAY" =~ ^[0-9]+$ ]]; then + bashio::log.error "Invalid delay value for ${ALIAS}: ${DELAY}" + continue + fi + + bashio::log.info "Initiating shutdown for ${ALIAS} (${ADDRESS}) with ${DELAY}s delay" + if ! msg="$(net rpc shutdown -I "$ADDRESS" -U "$CREDENTIALS" -t "$DELAY" -C "$MESSAGE" 2>&1)"; then + bashio::log.error "Shutdown failed for ${ALIAS} (${ADDRESS}): ${msg}" + continue fi + bashio::log.info "Shutdown command sent successfully to ${ALIAS}"google_assistant/rootfs/etc/services.d/google-assistant/run (2)
Line range hint
14-18: Consider setting secure file permissions for client secretsWhile the file copying logic is correct, sensitive files like client secrets should have restricted permissions to prevent unauthorized access.
Consider adding permission setting after copying:
if bashio::fs.file_exists "/share/${CLIENT_SECRETS}"; then bashio::log.info "Installing/Updating service client_secrets file" cp -f "/share/${CLIENT_SECRETS}" "${CLIENT_JSON}" + chmod 600 "${CLIENT_JSON}" fi
Line range hint
20-24: Consider adding JSON validation for client secretsWhile the file existence checks are good, validating the JSON content would prevent runtime errors if the file is malformed.
Consider adding JSON validation:
if ! bashio::fs.file_exists "${CRED_JSON}" && bashio::fs.file_exists "${CLIENT_JSON}"; then + if ! jq -e . "${CLIENT_JSON}" >/dev/null 2>&1; then + bashio::exit.nok "Client secrets file is not valid JSON!" + fi bashio::log.info "Starting WebUI for handling OAuth2..." python3 /usr/bin/hassio_oauth.py "${CLIENT_JSON}" "${CRED_JSON}" elif ! bashio::fs.file_exists "${CRED_JSON}"; then bashio::exit.nok "You need initialize Google Assistant with a client secret JSON!" ficonfigurator/rootfs/etc/s6-overlay/s6-rc.d/configurator/run (2)
Line range hint
12-27: Consider adding error handling for SSH operations.While the script handles missing key files, it doesn't check for failures in SSH agent startup or key addition operations. These operations could fail in various scenarios (permissions issues, invalid keys, etc.).
Consider adding error handling:
# Start the SSH agent bashio::log.info "Starting SSH agent" -eval "$(ssh-agent -s)" +if ! eval "$(ssh-agent -s)"; then + bashio::log.error "Failed to start SSH agent" + exit 1 +fi # Add the keys defined in the configuration options while read -r filename; do if bashio::fs.file_exists "${filename}"; then bashio::log.info "Adding SSH private key file \"${filename}\"" - ssh-add -q "${filename}" + if ! ssh-add -q "${filename}"; then + bashio::log.error "Failed to add SSH key ${filename}" + # Continue with other keys instead of failing completely + fi else bashio::log.error "SSH key file \"${filename}\" not found" fi done <<< "$(bashio::config 'ssh_keys')"
Line range hint
36-36: Add configuration file validation before execution.The script should verify that the configuration file exists before attempting to execute the service.
Consider adding this check:
+if ! bashio::fs.file_exists "/etc/configurator.conf"; then + bashio::log.error "Configuration file /etc/configurator.conf not found" + exit 1 +fi exec hass-configurator /etc/configurator.confsilabs-multiprotocol/rootfs/etc/s6-overlay/scripts/cpcd-config-up (4)
Line range hint
10-19: Add device path validationWhile the configuration checks are good, the script should validate that the device path exists before proceeding.
Add this validation after the device path is determined:
if bashio::config.has_value 'network_device'; then device="/tmp/ttyCPC" fi + +if [ ! -e "$device" ]; then + bashio::log.fatal "Device path ${device} does not exist!" + bashio::exit.nok +fi
Line range hint
21-29: Add baudrate validationThe script should validate that the baudrate is a positive integer to prevent configuration errors.
Add this validation after the baudrate is determined:
elif bashio::config.has_value 'baudrate'; then baudrate=$(bashio::config 'baudrate') else bashio::log.fatal "No serial port baudrate set!" bashio::exit.nok fi + +if ! [[ "$baudrate" =~ ^[0-9]+$ ]] || [ "$baudrate" -le 0 ]; then + bashio::log.fatal "Invalid baudrate: ${baudrate}. Must be a positive integer!" + bashio::exit.nok +fi
Line range hint
31-41: Refactor boolean config handling and add validationThe boolean configuration handling is duplicated and lacks validation.
Consider refactoring using a helper function:
+# Helper function to get boolean config with validation +get_boolean_config() { + local key=$1 + local value + + if ! bashio::config.has_value "$key"; then + echo "false" + return + fi + + value=$(bashio::config "$key") + if [ "$value" != "true" ] && [ "$value" != "false" ]; then + bashio::log.fatal "Invalid value for ${key}: ${value}. Must be 'true' or 'false'!" + bashio::exit.nok + fi + echo "$value" +} + -if ! bashio::config.has_value 'flow_control'; then - flow_control="false" -else - flow_control=$(bashio::config 'flow_control') -fi +flow_control=$(get_boolean_config 'flow_control') -if ! bashio::config.has_value 'cpcd_trace'; then - cpcd_trace="false" -else - cpcd_trace=$(bashio::config 'cpcd_trace') -fi +cpcd_trace=$(get_boolean_config 'cpcd_trace')
Line range hint
43-51: Add template file validationThe script should verify that the template file exists before attempting to use it.
Add this validation before the configuration generation:
bashio::log.info "Generating cpcd configuration." + +template_file="/usr/local/share/cpcd.conf" +if [ ! -f "$template_file" ]; then + bashio::log.fatal "Template file ${template_file} not found!" + bashio::exit.nok +fi + bashio::var.json \ device "${device}" \ baudrate "${baudrate}" \ flow_control "${flow_control}" \ cpcd_trace "${cpcd_trace}" \ | tempio \ - -template /usr/local/share/cpcd.conf \ + -template "$template_file" \ -out /usr/local/etc/cpcd.confssh/rootfs/etc/cont-init.d/profile.sh (4)
Line range hint
8-12: Add error handling for file operations.While the implementation is secure with proper permissions, it could be more robust.
Consider this improved implementation:
if ! bashio::fs.file_exists /data/.bash_history; then - touch /data/.bash_history + touch /data/.bash_history || bashio::exit.nok "Failed to create .bash_history" fi -chmod 600 /data/.bash_history +chmod 600 /data/.bash_history || bashio::exit.nok "Failed to set permissions on .bash_history" + +# Validate permissions even if file existed +if [[ "$(stat -c %a /data/.bash_history)" != "600" ]]; then + chmod 600 /data/.bash_history || bashio::exit.nok "Failed to correct permissions on .bash_history" +fi
Line range hint
29-33: Enhance symlink creation robustness.While the error handling is good, the symlink creation could be more robust.
Consider this improved implementation:
for dir in "${DIRECTORIES[@]}"; do + # Verify source directory exists + if ! bashio::fs.directory_exists "/${dir}"; then + bashio::log.warning "Source directory /${dir} does not exist" + continue + fi + + # Handle existing symlink + if [[ -L "${HOME}/${dir}" ]]; then + rm "${HOME}/${dir}" || bashio::log.warning "Failed to remove existing symlink: ${HOME}/${dir}" + fi + ln -s "/${dir}" "${HOME}/${dir}" \ - || bashio::log.warning "Failed linking common directory: ${dir}" + || bashio::log.warning "Failed linking /${dir} to ${HOME}/${dir}" done
Line range hint
35-40: Document legacy symlinks and enhance robustness.Similar improvements as suggested for directory symlinks would be beneficial here.
Consider adding a code comment explaining which documentation these symlinks support:
-# Some links to "old" locations, to match documentation, -# backwards compatibility and musle memory +# Legacy symlinks for backwards compatibility with: +# - Historical Home Assistant documentation prior to <version/date> +# - User muscle memory from previous installations +# - Third-party add-ons that might rely on these paths
Security improvements needed for SUPERVISOR_TOKEN handling
The verification reveals several security concerns:
- The template file exposes the token as a plain environment variable
- No explicit permissions are set for
/etc/profile.ddirectory or the generated profile script- Other sensitive files in the addon have proper 600/700 permissions set, but this one is missing
Required changes:
- Set restrictive permissions (644) on the generated
/etc/profile.d/homeassistant.sh- Set proper directory permissions (755) on
/etc/profile.d- Add error handling for failed operations
- Consider moving the token to a more secure location than profile.d
The original review comment's concerns are valid and the suggested security measures should be implemented.
🔗 Analysis chain
Line range hint
14-20: Verify secure handling of SUPERVISOR_TOKEN.The script handles sensitive token data but needs additional security measures.
Let's verify the template and file permissions:
Consider adding these security measures:
# Make Home Assistant TOKEN available on the CLI -mkdir -p /etc/profile.d +mkdir -p /etc/profile.d || bashio::exit.nok "Failed to create profile.d directory" +chmod 755 /etc/profile.d || bashio::exit.nok "Failed to set profile.d permissions" bashio::var.json \ supervisor_token "${SUPERVISOR_TOKEN}" \ | tempio \ -template /usr/share/tempio/homeassistant.profile \ - -out /etc/profile.d/homeassistant.sh + -out /etc/profile.d/homeassistant.sh \ + || bashio::exit.nok "Failed to generate profile script" +chmod 644 /etc/profile.d/homeassistant.sh || bashio::exit.nok "Failed to set profile permissions"
Let me search for the template file in the repository first.
Let me check the content of the template file and look for any existing permission settings in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check template content if [ -f /usr/share/tempio/homeassistant.profile ]; then echo "Template content:" cat /usr/share/tempio/homeassistant.profile else echo "Template file not found!" fi # Check output file permissions if [ -f /etc/profile.d/homeassistant.sh ]; then echo "Output file permissions:" ls -l /etc/profile.d/homeassistant.sh else echo "Output file not found!" fiLength of output: 211
Script:
#!/bin/bash # Search for homeassistant.profile template fd homeassistant.profile # Search for any tempio templates fd -e profile # Check the SSH addon directory structure ls -R ssh/Length of output: 807
Script:
#!/bin/bash # Check template content cat ssh/rootfs/usr/share/tempio/homeassistant.profile # Check for permission settings in Dockerfile or init scripts echo "=== Dockerfile permissions ===" rg "chmod|chown" ssh/Dockerfile echo "=== Init scripts permissions ===" rg "chmod|chown" ssh/rootfs/etc/cont-init.d/Length of output: 761
samba/rootfs/etc/s6-overlay/s6-rc.d/init-smbd/run (5)
Line range hint
9-12: Consider enhancing the error message specificity.While the validation is correct, the error message could be more specific about which required field(s) are missing.
- bashio::exit.nok "Setting a username and password is required!" + if ! bashio::config.has_value 'username'; then + bashio::exit.nok "Username is required but not set!" + fi + bashio::exit.nok "Password is required but not set!"
Line range hint
14-19: Enhance the warning message for hostname fallback.The warning message could provide more context about using the default hostname.
- bashio::log.warning "Can't read hostname, using default." + bashio::log.warning "Unable to read hostname from API, falling back to default: 'hassio'"
Line range hint
21-27: Consider improving interface logging format.The current interface logging might be hard to read if there are many interfaces. Consider formatting them one per line.
-bashio::log.info "Interfaces: $(printf '%s ' "${interfaces[@]}")" +bashio::log.info "Detected interfaces:" +for interface in "${interfaces[@]}"; do + bashio::log.info " - ${interface}" +done
Line range hint
29-33: Add error handling for configuration generation.The configuration generation commands lack error handling, which could lead to silent failures.
-jq ".interfaces = $(jq -c -n '$ARGS.positional' --args -- "${interfaces[@]}")" /data/options.json \ +if ! jq ".interfaces = $(jq -c -n '$ARGS.positional' --args -- "${interfaces[@]}")" /data/options.json \ | tempio \ -template /usr/share/tempio/smb.gtpl \ - -out /etc/samba/smb.conf + -out /etc/samba/smb.conf; then + bashio::exit.nok "Failed to generate Samba configuration" +fi
Line range hint
35-42: Add error handling and improve password security.The user initialization lacks error handling, and the password might be visible in the process list.
username=$(bashio::config 'username') password=$(bashio::config 'password') -addgroup "${username}" -adduser -D -H -G "${username}" -s /bin/false "${username}" +if ! addgroup "${username}"; then + bashio::exit.nok "Failed to create group ${username}" +fi +if ! adduser -D -H -G "${username}" -s /bin/false "${username}"; then + bashio::exit.nok "Failed to create user ${username}" +fi -echo "$password"; echo "$password") \ - | smbpasswd -a -s -c "/etc/samba/smb.conf" "${username}" +# Use file descriptor to avoid password exposure in process list +exec 3<> <(:) +echo "$password" >&3 +echo "$password" >&3 +if ! smbpasswd -a -s -c "/etc/samba/smb.conf" "${username}" <&3; then + exec 3>&- + bashio::exit.nok "Failed to set Samba password" +fi +exec 3>&-deconz/rootfs/etc/services.d/otau-ledvance/run (4)
Line range hint
1-10: Add error handling for directory creationWhile the initialization looks good, the
mkdircommand should include error handling to ensure the directory was created successfully.-mkdir -p "/data/otau" +if ! mkdir -p "/data/otau"; then + bashio::log.error "Failed to create OTAU directory" + exit 1 +fi
Line range hint
12-16: Enhance error handling and curl robustnessThe error handling and curl configuration could be improved for better reliability:
- Add timeout to prevent hanging
- Add retry mechanism
- Use a configurable sleep duration
-if ! OSRAM_DATA="$(curl -sL ${URL_OSRAM})"; then +# Define constants +readonly MAX_RETRIES=3 +readonly CURL_TIMEOUT=30 +readonly FAILURE_SLEEP=18000 + +# Retry logic for data fetching +for ((retry=1; retry<=MAX_RETRIES; retry++)); do + if OSRAM_DATA="$(curl -sL --connect-timeout $CURL_TIMEOUT --max-time $((CURL_TIMEOUT * 2)) "${URL_OSRAM}")"; then + break + fi + bashio::log.warning "Failed to fetch data from OSRAM (attempt $retry/$MAX_RETRIES)" + [[ $retry -lt $MAX_RETRIES ]] && sleep $((retry * 10)) +done + +if [[ -z "${OSRAM_DATA}" ]]; then bashio::log.info "Can't fetch data from osram!" - exec sleep 18000 + exec sleep "${FAILURE_SLEEP}" fi
Line range hint
18-35: Improve security and robustness of firmware processingSeveral improvements could enhance the security and reliability:
- Quote variables in arithmetic operations
- Validate downloaded files
- Use atomic operations for downloads
- Add more comprehensive logging
-OSRAM_DATA_SIZE="$(echo "${OSRAM_DATA}" | jq --raw-output '.firmwares | length')" -DL_DONE=0 -for (( i=0; i < "${OSRAM_DATA_SIZE}"; i++ )); do +readonly OSRAM_DATA_SIZE="$(echo "${OSRAM_DATA}" | jq --raw-output '.firmwares | length')" +if [[ ! "${OSRAM_DATA_SIZE}" =~ ^[0-9]+$ ]]; then + bashio::log.error "Invalid firmware data received" + exit 1 +fi + +DL_DONE=0 +for (( i=0; i < "${OSRAM_DATA_SIZE}"; i++ )); do OSRAM_COMPANY=$( echo "${OSRAM_DATA}" | jq --raw-output ".firmwares[$i].identity.company // empty" 2>/dev/null) OSRAM_PRODUCT=$( echo "${OSRAM_DATA}" | jq --raw-output ".firmwares[$i].identity.product // empty" 2>/dev/null) OTAU_FILENAME=$( echo "${OSRAM_DATA}" | jq --raw-output ".firmwares[$i].name // empty" 2>/dev/null) + + # Validate extracted data + if [[ -z "${OSRAM_COMPANY}" ]] || [[ -z "${OSRAM_PRODUCT}" ]] || [[ -z "${OTAU_FILENAME}" ]]; then + bashio::log.warning "Skipping invalid firmware entry at index $i" + continue + fi + OTAU_URL="$URL_OSRAM/download/${OSRAM_COMPANY}/${OSRAM_PRODUCT}/latest" OTAU_FILE="/data/otau/${OTAU_FILENAME}" - if [ -e "${OTAU_FILE}" ]; then + if [[ -e "${OTAU_FILE}" ]]; then + bashio::log.info "Firmware ${OTAU_FILENAME} already exists, skipping" continue fi - curl -s -L -o "${OTAU_FILE}" "${OTAU_URL}" || true - ((DL_DONE+1)) - if [ "$((DL_DONE % 10))" == "0" ]; then + + # Download to temporary file first + TEMP_FILE="${OTAU_FILE}.tmp" + if curl -s -L -o "${TEMP_FILE}" "${OTAU_URL}"; then + mv "${TEMP_FILE}" "${OTAU_FILE}" + bashio::log.info "Successfully downloaded ${OTAU_FILENAME}" + ((DL_DONE+=1)) + else + bashio::log.warning "Failed to download ${OTAU_FILENAME}" + rm -f "${TEMP_FILE}" + continue + fi + + if (( DL_DONE % 10 == 0 )); then # LEDVANCE/OSRAM API RateLimits : The rate limit 10 calls per 60 seconds or quota 100 MB per month. DL_DONE=0 + bashio::log.info "Rate limit reached, pausing for 65 seconds" sleep 65 fi done
Line range hint
37-37: Make service more configurable and handle shutdown gracefullyThe service loop could be improved by:
- Making the sleep duration configurable
- Adding proper shutdown handling
-exec sleep 259200 +# Define update interval (default: 3 days) +readonly UPDATE_INTERVAL=$(bashio::config 'update_interval' 259200) + +# Use trap to handle shutdown gracefully +trap 'bashio::log.info "Shutting down OSRAM OTAU service..."; exit 0' TERM + +# Keep service alive +exec sleep "${UPDATE_INTERVAL}"mosquitto/rootfs/etc/services.d/mosquitto/discovery (2)
Line range hint
7-14: Add error handling for password retrievalThe script reads sensitive passwords but lacks error handling for file read operations. Consider:
- Checking if the system user file exists
- Validating that password values are not empty
- Using readonly for password variables to prevent accidental modification
readonly SYSTEM_USER="/data/system_user.json" declare config -declare discovery_password -declare service_password +declare -r discovery_password +declare -r service_password + +# Verify system user file exists +if ! bashio::fs.file_exists "${SYSTEM_USER}"; then + bashio::log.error "System user file not found: ${SYSTEM_USER}" + exit 1 +fi # Read the existing values discovery_password=$(bashio::jq "${SYSTEM_USER}" ".homeassistant.password") service_password=$(bashio::jq "${SYSTEM_USER}" ".addons.password") + +# Validate passwords +if bashio::var.is_empty "${discovery_password}" || bashio::var.is_empty "${service_password}"; then + bashio::log.error "Failed to retrieve valid passwords from system user file" + exit 1 +fi
Line range hint
30-43: Consider refactoring duplicate configuration logicThe service configuration is nearly identical to the discovery configuration. Consider extracting the common configuration into a function.
+# Create MQTT configuration with specific credentials +mqtt_config() { + local username=$1 + local password=$2 + bashio::var.json \ + host "$(hostname)" \ + port "^1883" \ + ssl "^false" \ + protocol "3.1.1" \ + username "${username}" \ + password "${password}" +} + # Create discovery config payload for Home Assistant -config=$(bashio::var.json \ - host "$(hostname)" \ - port "^1883" \ - ssl "^false" \ - protocol "3.1.1" \ - username "homeassistant" \ - password "${discovery_password}" \ -) +config=$(mqtt_config "homeassistant" "${discovery_password}") # Send discovery info if bashio::discovery "mqtt" "${config}" > /dev/null; then bashio::log.info "Successfully send discovery information to Home Assistant." else bashio::log.error "Discovery message to Home Assistant failed!" fi # Create service config payload for other add-ons -config=$(bashio::var.json \ - host "$(hostname)" \ - port "^1883" \ - ssl "^false" \ - protocol "3.1.1" \ - username "addons" \ - password "${service_password}" \ -) +config=$(mqtt_config "addons" "${service_password}")ssh/rootfs/etc/cont-init.d/ssh.sh (4)
1-2: Consider adding more shellcheck directives for improved maintainabilityWhile the
shell=bashdirective is good, consider adding these commonly used directives for better maintainability:#!/usr/bin/with-contenv bashio # shellcheck shell=bash +# shellcheck disable=SC1091 # Don't warn about not being able to source files +# shellcheck enable=require-variable-braces # Enforce ${} usage
Line range hint
8-15: Improve error handling for directory operationsWhile the permissions are correctly set to 700, the error handling could be more specific.
if ! bashio::fs.directory_exists /data/.ssh; then - mkdir -p /data/.ssh \ - || bashio::exit.nok 'Failed to create a persistent .ssh folder' + mkdir -p /data/.ssh || bashio::exit.nok 'Failed to create a persistent .ssh folder' fi -chmod 700 /data/.ssh \ - || bashio::exit.nok \ - 'Failed setting permissions on persistent .ssh folder' +chmod 700 /data/.ssh || bashio::exit.nok 'Failed setting permissions on persistent .ssh folder'
Line range hint
24-44: Security improvements needed in authentication setupSeveral security considerations need to be addressed:
- Add validation for authorized_keys content
- Implement password strength requirements
- Improve error handling for password changes
Apply these improvements:
if bashio::config.has_value 'authorized_keys'; then bashio::log.info "Setup authorized_keys" rm -f /data/.ssh/authorized_keys while read -r line; do + # Validate key format + if ! echo "$line" | ssh-keygen -l -f - >/dev/null 2>&1; then + bashio::exit.nok "Invalid SSH key found in authorized_keys" + fi echo "$line" >> /data/.ssh/authorized_keys done <<< "$(bashio::config 'authorized_keys')" chmod 600 /data/.ssh/authorized_keys # Unlock account PASSWORD="$(pwgen -s 64 1)" - echo "root:${PASSWORD}" | chpasswd 2&> /dev/null + echo "root:${PASSWORD}" | chpasswd || bashio::exit.nok "Failed to set random password" elif bashio::config.has_value 'password'; then bashio::log.info "Setup password login" PASSWORD=$(bashio::config 'password') + # Validate password strength + if [ ${#PASSWORD} -lt 8 ]; then + bashio::exit.nok "Password must be at least 8 characters long" + fi - echo "root:${PASSWORD}" | chpasswd 2&> /dev/null + echo "root:${PASSWORD}" | chpasswd || bashio::exit.nok "Failed to set user password" elif bashio::var.has_value "$(bashio::addon.port 22)"; then bashio::exit.nok "You need to setup a login!" fi
Line range hint
46-51: Enhance SSH config generation security and error handlingThe SSH config generation needs better error handling and security controls.
-mkdir -p /etc/ssh +mkdir -p /etc/ssh || bashio::exit.nok "Failed to create SSH config directory" +chmod 755 /etc/ssh || bashio::exit.nok "Failed to set SSH config directory permissions" tempio \ -conf /data/options.json \ -template /usr/share/tempio/sshd_config \ - -out /etc/ssh/sshd_config + -out /etc/ssh/sshd_config || bashio::exit.nok "Failed to generate SSH config" +chmod 644 /etc/ssh/sshd_config || bashio::exit.nok "Failed to set SSH config permissions"silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/otbr-agent/finish (3)
Line range hint
11-20: Add timeout mechanism to prevent infinite loopsWhile the retry mechanism is good for handling kernel locks, it should include a timeout to prevent infinite loops.
ipset_destroy_if_exist() { # The ipset seems to be in use by the kernel for a brief period, # retry destroying it + local attempts=0 + local max_attempts=10 while ipset list -n "$1" 2> /dev/null; do + ((attempts++)) + if [ "$attempts" -ge "$max_attempts" ]; then + bashio::log.warning "Failed to destroy ipset $1 after $max_attempts attempts" + break + fi ipset destroy "$1" || true + sleep 0.5 done }
Line range hint
21-38: Add error logging for failed operationsWhile the operations are properly sequenced, adding error logging would help with troubleshooting.
while ip6tables -C FORWARD -o $thread_if -j $otbr_forward_ingress_chain 2> /dev/null; do # shellcheck disable=SC2086 - ip6tables -D FORWARD -o $thread_if -j $otbr_forward_ingress_chain + if ! ip6tables -D FORWARD -o $thread_if -j $otbr_forward_ingress_chain; then + bashio::log.warning "Failed to delete ingress FORWARD rule" + fi done
Line range hint
54-67: Enhance signal and exit code loggingWhile the exit code handling is correct, consider adding more detailed logging for better diagnostics.
if test "$1" -eq 256 ; then + bashio::log.debug "Received signal-based exit" e=$((128 + $2)) else e="$1" fi if test "$e" -ne 0; then - bashio::log.warning "otbr-agent exited with code $e (by signal $2)." + bashio::log.warning "otbr-agent failed with exit code $e (signal: $2, original code: $1)" echo "$e" > /run/s6-linux-init-container-results/exitcode + bashio::log.info "Initiating container shutdown" exec /run/s6/basedir/bin/halt else - bashio::log.info "otbr-agent exited with code $e (by signal $2)." + bashio::log.info "otbr-agent completed successfully (exit: $e, signal: $2)" fidhcp_server/data/run.sh (6)
Line range hint
9-20: Add input validation and improve variable safetyThe configuration values should be validated before use, and variables should be properly quoted in echo statements.
# Create main config DEFAULT_LEASE=$(bashio::config 'default_lease') +if ! [[ "${DEFAULT_LEASE}" =~ ^[0-9]+$ ]] || [ "${DEFAULT_LEASE}" -lt 60 ]; then + bashio::exit.nok "Invalid default lease time: ${DEFAULT_LEASE}" +fi + DOMAIN=$(bashio::config 'domain') MAX_LEASE=$(bashio::config 'max_lease') +if ! [[ "${MAX_LEASE}" =~ ^[0-9]+$ ]] || [ "${MAX_LEASE}" -lt "${DEFAULT_LEASE}" ]; then + bashio::exit.nok "Invalid max lease time: ${MAX_LEASE}" +fi { - echo "option domain-name \"${DOMAIN}\";" - echo "default-lease-time ${DEFAULT_LEASE};" - echo "max-lease-time ${MAX_LEASE};" + echo "option domain-name \"${DOMAIN:?}\";" + echo "default-lease-time ${DEFAULT_LEASE:?};" + echo "max-lease-time ${MAX_LEASE:?};" echo "authoritative;" } > "${CONFIG}"
Line range hint
22-29: Validate DNS servers and simplify conditionAdd IP address validation and use bashio's built-in functions for cleaner code.
-if [ "$(bashio::config 'dns')" ] +if bashio::config.exists 'dns' then DNS=$(bashio::config 'dns|join(", ")') + # Validate DNS servers + for ip in $(bashio::config 'dns'); do + if ! bashio::var.is_ip_address "$ip"; then + bashio::exit.nok "Invalid DNS server IP: ${ip}" + fi + done { - echo "option domain-name-servers ${DNS};"; + echo "option domain-name-servers ${DNS:?};"; } >> "${CONFIG}" fi
Line range hint
41-63: Add network configuration validationThe network configuration needs validation for IP addresses, ranges, and subnet consistency.
for network in $(bashio::config 'networks|keys'); do BROADCAST=$(bashio::config "networks[${network}].broadcast") GATEWAY=$(bashio::config "networks[${network}].gateway") INTERFACE=$(bashio::config "networks[${network}].interface") NETMASK=$(bashio::config "networks[${network}].netmask") RANGE_START=$(bashio::config "networks[${network}].range_start") RANGE_END=$(bashio::config "networks[${network}].range_end") SUBNET=$(bashio::config "networks[${network}].subnet") + # Validate IP addresses + for ip in "${BROADCAST}" "${GATEWAY}" "${RANGE_START}" "${RANGE_END}" "${SUBNET}"; do + if ! bashio::var.is_ip_address "$ip"; then + bashio::exit.nok "Invalid IP address in network ${network}: ${ip}" + fi + done + + # Validate interface exists + if ! ip link show "${INTERFACE}" >/dev/null 2>&1; then + bashio::exit.nok "Interface ${INTERFACE} does not exist" + fi + + # Validate IP range + if ! bashio::var.is_ip_in_network "${RANGE_START}" "${SUBNET}/${NETMASK}" || \ + ! bashio::var.is_ip_in_network "${RANGE_END}" "${SUBNET}/${NETMASK}"; then + bashio::exit.nok "IP range ${RANGE_START}-${RANGE_END} not in subnet ${SUBNET}/${NETMASK}" + fi + { echo "subnet ${SUBNET} netmask ${NETMASK} {" echo " interface ${INTERFACE};" echo " range ${RANGE_START} ${RANGE_END};" echo " option routers ${GATEWAY};" echo " option broadcast-address ${BROADCAST};" echo "}" } >> "${CONFIG}" done
Line range hint
65-79: Validate host configurations and ensure uniquenessAdd validation for MAC addresses and ensure IP/MAC uniqueness across hosts.
+declare -A used_ips used_macs + for host in $(bashio::config 'hosts|keys'); do IP=$(bashio::config "hosts[${host}].ip") MAC=$(bashio::config "hosts[${host}].mac") NAME=$(bashio::config "hosts[${host}].name") + # Validate MAC address format + if ! [[ "${MAC}" =~ ^([0-9A-Fa-f]{2}:){5}[0-9A-Fa-f]{2}$ ]]; then + bashio::exit.nok "Invalid MAC address format for host ${NAME}: ${MAC}" + fi + + # Check for duplicates + if [[ -n "${used_ips[$IP]:-}" ]]; then + bashio::exit.nok "Duplicate IP address ${IP} for hosts ${used_ips[$IP]} and ${NAME}" + fi + if [[ -n "${used_macs[$MAC]:-}" ]]; then + bashio::exit.nok "Duplicate MAC address ${MAC} for hosts ${used_macs[$MAC]} and ${NAME}" + fi + used_ips[$IP]=$NAME + used_macs[$MAC]=$NAME + { echo "host ${NAME} {" echo " hardware ethernet ${MAC};" echo " fixed-address ${IP};" echo " option host-name \"${NAME}\";" echo "}" } >> "${CONFIG}" done
Line range hint
81-91: Enhance server startup reliabilityAdd configuration validation and improve error handling during startup.
# Create database if ! bashio::fs.file_exists "${LEASES}"; then touch "${LEASES}" fi +# Validate configuration +if ! /usr/sbin/dhcpd -t -cf "${CONFIG}"; then + bashio::exit.nok "Invalid DHCP configuration" +fi + # Start DHCP server bashio::log.info "Starting DHCP server..." -exec /usr/sbin/dhcpd \ - -4 -f -d --no-pid \ - -lf "${LEASES}" \ - -cf "${CONFIG}" \ - < /dev/null +exec /usr/sbin/dhcpd -4 -f -d --no-pid \ + -lf "${LEASES}" -cf "${CONFIG}" < /dev/null 2>&1 \ + || bashio::exit.nok "Failed to start DHCP server"
Line range hint
6-7: Set appropriate file permissionsAdd proper file permissions for security.
CONFIG="/etc/dhcpd.conf" LEASES="/data/dhcpd.lease" + +# Ensure secure file permissions +chmod 644 "${CONFIG}" +chmod 644 "${LEASES}"deconz/rootfs/etc/services.d/deconz/run (2)
Line range hint
23-45: Security Concern: VNC server configuration needs reviewThe VNC server is configured with
-SecurityTypes Noneand--I-KNOW-THIS-IS-INSECURE, which could pose security risks:
- No authentication is required
- If exposed, anyone could access the VNC server
- While there is a warning log, it might be missed by users
Consider implementing these security improvements:
tigervncserver \ -name "Home Assistant - deCONZ" \ -geometry 1920x1080 \ -depth 16 \ -localhost ${LOCAL_ONLY} \ - -SecurityTypes None \ - --I-KNOW-THIS-IS-INSECURE \ + -SecurityTypes VncAuth \ + -PasswordFile /config/vnc_password \ "${DISPLAY}" \ &> /dev/null
Line range hint
58-75: Improve error handling for discovery processThe current implementation launches the discovery process in the background without proper error handling or monitoring.
Consider implementing this more robust approach:
-# Send out discovery information to Home Assistant -./discovery & +# Start discovery process with proper error handling +if ! ./discovery & then + bashio::log.warning "Failed to start discovery process" +fi +DISCOVERY_PID=$! + +# Trap to clean up discovery process +trap 'kill $DISCOVERY_PID 2>/dev/null' EXITThis change will:
- Log discovery process failures
- Store the PID for proper cleanup
- Ensure the discovery process is terminated when the script exits
deconz/rootfs/etc/services.d/deconz/discovery (4)
Line range hint
19-20: Protect against SQL injectionThe SQL query string should be properly escaped to prevent potential SQL injection. While the current usage seems safe, it's better to follow best practices.
Consider using SQLite's parameter binding:
-query='SELECT apikey FROM auth WHERE devicetype="Home Assistant" ORDER BY createdate DESC LIMIT 1' -api_key=$(sqlite3 "${DATA_STORE}" "${query}" .exit) +api_key=$(sqlite3 "${DATA_STORE}" "SELECT apikey FROM auth WHERE devicetype=? ORDER BY createdate DESC LIMIT 1;" "Home Assistant" .exit)
Line range hint
23-28: Improve error handling for API key registrationThe error handling for the API registration could be more robust. The current implementation logs the error but doesn't provide specific error details.
Consider enhancing the error message:
if ! result="$(curl --silent --show-error --request POST -d '{"devicetype": "Home Assistant"}' "http://127.0.0.1:40850/api")"; then - bashio::log.debug "${result}" - bashio::exit.nok "Can't get API key from deCONZ gateway" + bashio::log.error "Failed to register with deCONZ gateway: ${result}" + bashio::exit.nok "Failed to obtain API key. Please check if deCONZ is running and accessible." fi
Line range hint
31-50: Consider making retry parameters configurableThe retry mechanism is well-implemented, but hardcoded values could be made configurable for better flexibility.
Consider defining these as variables at the top of the script:
+# Configuration +readonly MAX_RETRIES=25 +readonly RETRY_INTERVAL=10 + # Try to get the bridge ID/serial, try to avoid using 0000000000000000 -retries=25 +retries="${MAX_RETRIES}" serial="0000000000000000" while [[ "${serial}" = "0000000000000000" ]]; do bashio::log.debug "Waiting for bridge ID..." - sleep 10 + sleep "${RETRY_INTERVAL}"
Line range hint
52-65: Enhance error handling and consider making port configurableThe configuration and discovery section could benefit from more detailed error handling and flexible port configuration.
Consider these improvements:
+# Configuration +readonly DECONZ_PORT=40850 + # Create config payload for Home Assistant config=$(bashio::var.json \ host "$(hostname)" \ - port "^40850" \ + port "^${DECONZ_PORT}" \ api_key "${api_key}" \ serial "${serial}" \ ) # Send discovery info if bashio::discovery "deconz" "${config}" > /dev/null; then bashio::log.info "Successfully send discovery information to Home Assistant." else - bashio::log.error "Discovery message to Home Assistant failed!" + bashio::log.error "Failed to send discovery information to Home Assistant" + bashio::log.debug "Discovery payload: ${config}" + bashio::exit.nok "Discovery failed. Please check Home Assistant logs for more details." fitellstick/data/run.sh (4)
1-4: Consider enhancing error handling setupWhile
set -eis good, consider adding these options for more robust error handling:#!/usr/bin/with-contenv bashio # shellcheck shell=bash -set -e +set -euo pipefail +trap 'bashio::log.error "Unexpected error in ${BASH_SOURCE[0]} at line ${LINENO}"' ERRThis will:
- Exit on undefined variables (
-u)- Prevent masking of pipe failures (
-o pipefail)- Add error location reporting via trap
Line range hint
6-13: Add configuration file safety checksThe script should validate permissions and handle existing configurations safely.
CONFIG="/etc/tellstick.conf" + +# Validate permissions and backup existing config +if [[ -f "${CONFIG}" ]]; then + bashio::log.info "Backing up existing configuration..." + cp "${CONFIG}" "${CONFIG}.backup" +fi + +if ! touch "${CONFIG}" 2>/dev/null; then + bashio::log.fatal "Cannot write to ${CONFIG}. Please check permissions." + exit 1 +fi bashio::log.info "Initialize the tellstick configuration..."
Line range hint
63-69: Improve process management and cleanupThe current implementation lacks proper process management and cleanup mechanisms.
+# Track background processes +declare -a PIDS + +cleanup() { + bashio::log.info "Cleaning up processes..." + for pid in "${PIDS[@]}"; do + kill "$pid" 2>/dev/null || true + done +} + +trap cleanup EXIT + # Expose the unix socket to internal network -socat TCP-LISTEN:50800,reuseaddr,fork UNIX-CONNECT:/tmp/TelldusClient & -socat TCP-LISTEN:50801,reuseaddr,fork UNIX-CONNECT:/tmp/TelldusEvents & +if ! socat TCP-LISTEN:50800,reuseaddr,fork UNIX-CONNECT:/tmp/TelldusClient & PIDS+=($!); then + bashio::log.error "Failed to start TelldusClient socket" + exit 1 +fi + +if ! socat TCP-LISTEN:50801,reuseaddr,fork UNIX-CONNECT:/tmp/TelldusEvents & PIDS+=($!); then + bashio::log.error "Failed to start TelldusEvents socket" + exit 1 +fi # Run telldus-core daemon in the background -/usr/local/sbin/telldusd --nodaemon < /dev/null & +if ! /usr/local/sbin/telldusd --nodaemon < /dev/null & PIDS+=($!); then + bashio::log.error "Failed to start telldusd" + exit 1 +fi
Line range hint
71-85: Address security and robustness concerns in event handlingThe event listener has potential security vulnerabilities and robustness issues.
# Listen for input to tdtool bashio::log.info "Starting event listener..." +declare -r TIMEOUT=10 + while read -r input; do + # Validate JSON format + if ! bashio::jq -e . >/dev/null 2>&1 <<<"${input}"; then + bashio::log.error "Invalid JSON input: ${input}" + continue + } + # parse JSON value funct=$(bashio::jq "${input}" '.function') devid=$(bashio::jq "${input}" '.device // empty') + + # Validate function name (prevent command injection) + if [[ ! "${funct}" =~ ^[a-zA-Z0-9_-]+$ ]]; then + bashio::log.error "Invalid function name: ${funct}" + continue + } + bashio::log.info "Read ${funct} / ${devid}" - if ! msg="$(tdtool "--${funct}" "${devid}")"; then + # Execute with timeout and proper quoting + if ! msg="$(timeout "${TIMEOUT}" tdtool "--${funct}" "${devid}")"; then bashio::log.error "TellStick ${funct} fails -> ${msg}" else bashio::log.info "TellStick ${funct} success -> ${msg}" fi donesilabs-multiprotocol/rootfs/etc/s6-overlay/scripts/universal-silabs-flasher-up (3)
Line range hint
46-57: Consider externalizing firmware version configuration.The firmware versions (4.3.1) are currently hardcoded, which could make updates challenging. Consider moving these to a configuration file or environment variables for easier maintenance.
Example approach:
# config.json or environment SKYCONNECT_FIRMWARE_VERSION="4.3.1" YELLOW_FIRMWARE_VERSION="4.3.1" # Script firmware_version="${SKYCONNECT_FIRMWARE_VERSION}" firmware="NabuCasa_SkyConnect_RCP_v${firmware_version}_rcp-uart-hw-802154_460800.gbl"
65-67: Improve command argument handling.The current command construction could be more robust with proper quoting.
Consider this safer alternative:
-# shellcheck disable=SC2086 -exec universal-silabs-flasher --device ${device} \ - flash ${gpio_reset_flag} --ensure-exact-version --allow-cross-flashing --firmware "/root/${firmware}" +exec universal-silabs-flasher --device "${device}" \ + flash ${gpio_reset_flag:+"${gpio_reset_flag}"} --ensure-exact-version --allow-cross-flashing --firmware "/root/${firmware}"This ensures proper handling of spaces in paths and empty variables.
Line range hint
59-60: Add error handling for baudrate configuration.The baudrate configuration write could fail silently.
Consider adding error handling:
-echo 460800 > /tmp/known-baudrate +if ! echo 460800 > /tmp/known-baudrate; then + bashio::log.warning "Failed to write known baudrate configuration" +fimosquitto/rootfs/etc/cont-init.d/mosquitto.sh (4)
Line range hint
16-28: Add error handling for password generation.While the password generation logic is secure, it lacks error handling for potential
pwgenfailures. This could lead to undefined behavior if password generation fails.Consider adding error checking:
- discovery_password="$(pwgen 64 1)" - service_password="$(pwgen 64 1)" + if ! discovery_password="$(pwgen 64 1)"; then + bashio::exit.nok "Failed to generate discovery password" + fi + if ! service_password="$(pwgen 64 1)"; then + bashio::exit.nok "Failed to generate service password" + fi
Line range hint
45-57: Consider adding username validation and enhancing ACL rules.Two potential improvements for security:
- Add username format validation to prevent injection attacks
- Consider more granular ACL rules based on user roles
Example implementation for username validation:
username=$(bashio::config "logins[${login}].username") +if [[ ! "$username" =~ ^[a-zA-Z0-9_-]+$ ]]; then + bashio::exit.nok "Invalid username format. Use only alphanumeric characters, underscore, and hyphen" +fi
Line range hint
59-73: Enhance SSL security checks.While the SSL configuration is functional, consider adding:
- Verification of certificate file permissions
- Validation of certificate expiration
- Documentation for the cafile fallback behavior
Example implementation:
if bashio::fs.file_exists "${certfile}" \ && bashio::fs.file_exists "${keyfile}"; then + # Check file permissions + if [[ "$(stat -c %a "${keyfile}")" != "600" ]]; then + bashio::log.warning "Insecure permissions on keyfile. Setting to 600" + chmod 600 "${keyfile}" + fi + + # Verify certificate validity + if ! openssl x509 -checkend 86400 -noout -in "${certfile}"; then + bashio::log.warning "SSL certificate will expire within 24 hours" + fi + bashio::log.info "Certificates found: SSL is available" ssl="true" if ! bashio::fs.file_exists "${cafile}"; then + bashio::log.info "No separate CA file provided, using certificate file as CA" cafile="${certfile}" fi
Line range hint
75-89: Add error handling for configuration template rendering.The configuration generation lacks error handling for the tempio template rendering process.
Consider adding validation:
-| tempio \ - -template /usr/share/tempio/mosquitto.gtpl \ - -out /etc/mosquitto/mosquitto.conf +| if ! tempio \ + -template /usr/share/tempio/mosquitto.gtpl \ + -out /etc/mosquitto/mosquitto.conf; then + bashio::exit.nok "Failed to generate Mosquitto configuration" +fi + +# Verify the generated configuration +if ! mosquitto -t -c /etc/mosquitto/mosquitto.conf; then + bashio::exit.nok "Invalid Mosquitto configuration generated" +fiduckdns/rootfs/etc/s6-overlay/s6-rc.d/duckdns/run (4)
Line range hint
19-41: Critical: Improve error handling in certificate renewal function.Several security and reliability concerns need to be addressed:
- The
|| trueafter the dehydrated command masks failures- Missing validation of jq command output
- Potential command injection risk in domain handling
Consider applying these improvements:
function le_renew() { local domain_args=() local domains='' local aliases='' - domains=$(bashio::config 'domains') + if ! domains=$(bashio::config 'domains'); then + bashio::log.error "Failed to get domains configuration" + return 1 + fi # Prepare domain for Let's Encrypt for domain in ${domains}; do - for alias in $(jq --raw-output --exit-status "[.aliases[]|{(.alias):.domain}]|add.\"${domain}\" | select(. != null)" /data/options.json) ; do + local jq_output + if ! jq_output=$(jq --raw-output --exit-status "[.aliases[]|{(.alias):.domain}]|add.\"${domain}\" | select(. != null)" /data/options.json); then + bashio::log.warning "Failed to process aliases for domain: ${domain}" + continue + fi + for alias in ${jq_output}; do aliases="${aliases} ${alias}" done done aliases="$(echo "${aliases}" | tr ' ' '\n' | sort | uniq)" bashio::log.info "Renew certificate for domains: $(echo -n "${domains}") and aliases: $(echo -n "${aliases}")" for domain in $(echo "${domains}" "${aliases}" | tr ' ' '\n' | sort | uniq); do + [[ "${domain}" =~ ^[a-zA-Z0-9.-]+$ ]] || { + bashio::log.error "Invalid domain format: ${domain}" + continue + } domain_args+=("--domain" "${domain}") done - dehydrated --cron --algo "${ALGO}" --hook /root/hooks.sh --challenge dns-01 "${domain_args[@]}" --out "${CERT_DIR}" --config "${WORK_DIR}/config" || true + if ! dehydrated --cron --algo "${ALGO}" --hook /root/hooks.sh --challenge dns-01 "${domain_args[@]}" --out "${CERT_DIR}" --config "${WORK_DIR}/config"; then + bashio::log.error "Certificate renewal failed" + return 1 + fi LE_UPDATE="$(date +%s)" + return 0 }
Line range hint
43-60: Consider adding error handling for directory operations.While the initialization logic is sound, it could benefit from additional error handling.
Consider these improvements:
if bashio::config.true 'lets_encrypt.accept_terms'; then # Init folder structs - mkdir -p "${CERT_DIR}" - mkdir -p "${WORK_DIR}" + if ! mkdir -p "${CERT_DIR}" "${WORK_DIR}"; then + bashio::log.error "Failed to create required directories" + exit 1 + fi # Clean up possible stale lock file if [ -e "${WORK_DIR}/lock" ]; then - rm -f "${WORK_DIR}/lock" + if ! rm -f "${WORK_DIR}/lock"; then + bashio::log.error "Failed to remove stale lock file" + exit 1 + fi bashio::log.warning "Reset dehydrated lock file" fi
Line range hint
63-108: Improve network operation reliability and validation.The DuckDNS update loop could benefit from more robust error handling and validation:
- Curl commands should have comprehensive error handling
- DuckDNS API response validation could be more thorough
- IPv6 address validation could be strengthened
Consider these improvements:
- [[ ${IPV4} != *:/* ]] && ipv4=${IPV4} || ipv4=$(curl -s -m 10 "${IPV4}") - [[ ${IPV6} != *:/* ]] && ipv6=${IPV6} || ipv6=$(curl -s -m 10 "${IPV6}") + if [[ ${IPV4} != *:/* ]]; then + ipv4=${IPV4} + else + if ! ipv4=$(curl -s -m 10 --retry 3 --retry-delay 2 "${IPV4}"); then + bashio::log.error "Failed to fetch IPv4 address" + ipv4="" + fi + fi + + if [[ ${IPV6} != *:/* ]]; then + ipv6=${IPV6} + else + if ! ipv6=$(curl -s -m 10 --retry 3 --retry-delay 2 "${IPV6}"); then + bashio::log.error "Failed to fetch IPv6 address" + ipv6="" + fi + fi # Get IPv6-address from host interface if [[ -n "$IPV6" && ${ipv6} != *:* ]]; then ipv6= bashio::cache.flush_all for addr in $(bashio::network.ipv6_address "$IPV6"); do - # Skip non-global addresses - if [[ ${addr} != fe80:* && ${addr} != fc* && ${addr} != fd* ]]; then + # Validate and select global IPv6 address + if [[ ${addr} =~ ^[0-9a-fA-F:]+$ && ${addr} != fe80:* && ${addr} != fc* && ${addr} != fd* ]]; then ipv6=${addr%/*} break fi done fiAlso, consider implementing a helper function for DuckDNS API calls to reduce code duplication:
update_duckdns() { local ip_type=$1 local ip_value=$2 local response if ! response=$(curl -s -m 10 --retry 3 "https://www.duckdns.org/update?domains=${DOMAINS}&token=${TOKEN}&${ip_type}=${ip_value}&verbose=true"); then bashio::log.error "DuckDNS API call failed" return 1 fi if [ "${response}" = 'KO' ]; then bashio::log.error "DuckDNS update failed: ${response}" return 1 fi if [[ "${response}" == *NOCHANGE* ]]; then bashio::log.debug "${response}" else bashio::log.info "${response}" fi return 0 }
Line range hint
109-114: Optimize certificate renewal check timing.The current implementation checks for certificate renewal on every iteration, which is unnecessary given the 12-hour interval.
Consider implementing a more efficient approach:
+ # Calculate next renewal time only when needed + declare -g NEXT_RENEWAL_CHECK=${NEXT_RENEWAL_CHECK:-0} now="$(date +%s)" - if bashio::config.true 'lets_encrypt.accept_terms' && [ $((now - LE_UPDATE)) -ge 43200 ]; then - le_renew + if [ "${now}" -ge "${NEXT_RENEWAL_CHECK}" ]; then + if bashio::config.true 'lets_encrypt.accept_terms' && [ $((now - LE_UPDATE)) -ge 43200 ]; then + le_renew + fi + # Set next check time to 1 hour from now + NEXT_RENEWAL_CHECK=$((now + 3600)) fimariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-post/run (1)
Line range hint
7-15: Consider improving error handling robustnessWhile the new function provides a good mechanism for controlled error suppression, there are a few potential improvements to consider:
- The command arguments should be quoted to prevent word splitting
- The log level restoration should be protected in case the command modifies it
- Consider adding debug logging for suppressed errors
Here's a suggested improvement:
function execute_without_error_messages() { local current_log_level="${__BASHIO_LOG_LEVELS[${__BASHIO_LOG_LEVEL}]}" bashio::log.level fatal local exit_code=0 - # shellcheck disable=SC2068 - $@ || exit_code=$? - # shellcheck disable=SC2086 - bashio::log.level ${current_log_level} + # Capture output for debug logging + local output + output=$("$@" 2>&1) || exit_code=$? + + # Log suppressed output at debug level + if [ -n "$output" ]; then + bashio::log.debug "Suppressed output: $output" + fi + + # Ensure log level is restored even if command changed it + bashio::log.level "$current_log_level" return ${exit_code} }letsencrypt/rootfs/etc/cont-init.d/file-structure.sh (5)
Line range hint
1-7: Add error handling for directory creation operations.The
mkdircommands should include error handling to ensure the directories are created successfully and to handle potential permission issues.-mkdir -p /data/workdir -mkdir -p /data/letsencrypt +if ! mkdir -p /data/workdir /data/letsencrypt; then + bashio::exit.nok "Failed to create required directories" +fi
Line range hint
8-67: Enhance security and maintainability of credentials handling.Several improvements can be made to this section:
- Credentials are currently exposed in the process list during the echo command
- The large echo statement is difficult to maintain
Consider refactoring to:
- Write credentials individually to avoid process list exposure
- Use a function to handle credential writing
+write_credential() { + local key=$1 + local value=$2 + if [[ -n "$value" ]]; then + echo "${key} = ${value}" >> /data/dnsapikey + fi +} +# Clear existing file +> /data/dnsapikey + +# Write credentials securely +write_credential "dns_desec_token" "$(bashio::config 'dns.desec_token')" +write_credential "dns_digitalocean_token" "$(bashio::config 'dns.digitalocean_token')" # ... repeat for other credentials
Line range hint
69-81: Add validation for mutually exclusive ClouDNS authentication options.The comment indicates that only a single non-empty auth option must be present, but the code doesn't enforce this requirement.
+# Count how many ClouDNS auth options are set +cloudns_auth_count=0 + if bashio::config.exists 'dns.cloudns_auth_id'; then + ((cloudns_auth_count++)) echo -e "dns_cloudns_auth_id = $(bashio::config 'dns.cloudns_auth_id')\n" >> /data/dnsapikey fi if bashio::config.exists 'dns.cloudns_sub_auth_id'; then + ((cloudns_auth_count++)) echo -e "dns_cloudns_sub_auth_id = $(bashio::config 'dns.cloudns_sub_auth_id')\n" >> /data/dnsapikey fi if bashio::config.exists 'dns.cloudns_sub_auth_user'; then + ((cloudns_auth_count++)) echo -e "dns_cloudns_sub_auth_user = $(bashio::config 'dns.cloudns_sub_auth_user')\n" >> /data/dnsapikey fi + +if [[ "$cloudns_auth_count" -gt 1 ]]; then + bashio::exit.nok "Only one ClouDNS authentication option should be configured" +fi
Line range hint
85-89: Add error handling for TransIP key processing.The openssl command lacks error handling and key validation.
if bashio::config.exists 'dns.transip_api_key'; then TRANSIP_API_KEY=$(bashio::config 'dns.transip_api_key') - echo "${TRANSIP_API_KEY}" | openssl rsa -out /data/transip-rsa.key - chmod 600 /data/transip-rsa.key + if ! echo "${TRANSIP_API_KEY}" | openssl rsa -check -noout 2>/dev/null; then + bashio::exit.nok "Invalid TransIP RSA key format" + fi + if ! echo "${TRANSIP_API_KEY}" | openssl rsa -out /data/transip-rsa.key; then + bashio::exit.nok "Failed to process TransIP RSA key" + fi + chmod 600 /data/transip-rsa.key || bashio::exit.nok "Failed to set TransIP key permissions" fi
Line range hint
91-105: Add logging for deprecated option cleanup.Consider adding logging to inform users when deprecated options are removed and suggest alternatives if available.
+bashio::log.info "Cleaning up deprecated configuration options..." if bashio::config.exists 'dns.cloudxns_api_key'; then + bashio::log.warning "Removing deprecated option 'dns.cloudxns_api_key' - CloudXNS service is no longer supported" bashio::addon.option 'dns.cloudxns_api_key' fi # ... repeat for other optionszwave_js/rootfs/etc/cont-init.d/config.sh (3)
Line range hint
52-67: Enhance key validation logic and error messaging.While the security check for example keys is crucial, consider these improvements:
- Use an array for example keys to simplify comparisons
- Include the specific matching key in the error message
Here's a suggested refactoring:
-if [ "${network_key_upper}" == "${DOCS_EXAMPLE_KEY_1}" ] || [ "${network_key_upper}" == "${DOCS_EXAMPLE_KEY_2}" ] || [ "${network_key_upper}" == "${DOCS_EXAMPLE_KEY_3}" ] || [ "${network_key_upper}" == "${DOCS_EXAMPLE_KEY_4}" ] || [ "${network_key_upper}" == "${DOCS_EXAMPLE_KEY_5}" ] || [ "${network_key_upper}" == "${DOCS_EXAMPLE_KEY_6}" ]; then +readonly DOCS_EXAMPLE_KEYS=("${DOCS_EXAMPLE_KEY_1}" "${DOCS_EXAMPLE_KEY_2}" "${DOCS_EXAMPLE_KEY_3}" "${DOCS_EXAMPLE_KEY_4}" "${DOCS_EXAMPLE_KEY_5}" "${DOCS_EXAMPLE_KEY_6}") +if [[ " ${DOCS_EXAMPLE_KEYS[*]} " =~ ${network_key_upper} ]]; then bashio::log.fatal bashio::log.fatal "The add-on detected that the Z-Wave network key used" - bashio::log.fatal "is from the documented example." + bashio::log.fatal "is from the documented example: ${network_key_upper}"
Line range hint
101-117: Consider using a case statement for better maintainability.The soft reset logic could be more maintainable using a case statement.
Here's a suggested refactoring:
-if bashio::config.equals 'soft_reset' 'Automatic'; then - bashio::log.info "Soft-reset set to automatic" - if [ "${host_chassis}" == "vm" ]; then - soft_reset=false - bashio::log.info "Virtual Machine detected, disabling soft-reset" - else - soft_reset=true - bashio::log.info "Virtual Machine not detected, enabling soft-reset" - fi -elif bashio::config.equals 'soft_reset' 'Enabled'; then - soft_reset=true - bashio::log.info "Soft-reset enabled by user" -else - soft_reset=false - bashio::log.info "Soft-reset disabled by user" -fi +case "$(bashio::config 'soft_reset')" in + "Automatic") + bashio::log.info "Soft-reset set to automatic" + if [ "${host_chassis}" == "vm" ]; then + soft_reset=false + bashio::log.info "Virtual Machine detected, disabling soft-reset" + else + soft_reset=true + bashio::log.info "Virtual Machine not detected, enabling soft-reset" + fi + ;; + "Enabled") + soft_reset=true + bashio::log.info "Soft-reset enabled by user" + ;; + *) + soft_reset=false + bashio::log.info "Soft-reset disabled by user" + ;; +esac
Line range hint
118-146: Enhance preset handling with validation and simplified JSON conversion.Consider these improvements:
- Add validation for preset values
- Simplify the JSON array conversion
Here's a suggested refactoring:
+# Define valid presets +readonly VALID_PRESETS=("SAFE_MODE" "NO_CONTROLLER_RECOVERY" "NO_WATCHDOG") +# Validate preset before adding +add_preset() { + local preset="$1" + if [[ " ${VALID_PRESETS[*]} " =~ ${preset} ]]; then + presets_array+=("${preset}") + else + bashio::log.warning "Invalid preset: ${preset}" + fi +} if bashio::config.true 'safe_mode'; then bashio::log.info "Safe mode enabled" bashio::log.warning "WARNING: While in safe mode, the performance of your Z-Wave network will be in a reduced state. This is only meant for debugging purposes." - presets_array+=("SAFE_MODE") + add_preset "SAFE_MODE" fi -# Convert presets array to JSON string and add to config -if [[ ${#presets_array[@]} -eq 0 ]]; then - presets="[]" -else - presets="$(printf '%s\n' "${presets_array[@]}" | jq -R . | jq -s .)" -fi +# Convert presets array to JSON string using a single jq command +presets="$(jq -n --arg v "${presets_array[*]}" '$v | split(" ")')"openthread_border_router/rootfs/etc/s6-overlay/s6-rc.d/otbr-agent/run (1)
Line range hint
141-147: Ensure all variable expansions are properly quoted in theexeccommandIn the
execcommand startingotbr-agent, variables like${thread_if},${device},${baudrate}, and${flow_control}should be enclosed in double quotes to prevent potential issues if they contain spaces.Apply this diff:
-# shellcheck disable=SC2086 exec s6-notifyoncheck -d -s 300 -w 300 -n 0 stdbuf -oL \ "/usr/sbin/otbr-agent" -I ${thread_if} -B "${backbone_if}" \ + "/usr/sbin/otbr-agent" -I "${thread_if}" -B "${backbone_if}" \ --rest-listen-address "${otbr_rest_listen}" \ - -d${otbr_log_level_int} -v -s \ - "spinel+hdlc+uart://${device}?uart-baudrate=${baudrate}${flow_control}" \ + -d"${otbr_log_level_int}" -v -s \ + "spinel+hdlc+uart://${device}?uart-baudrate=${baudrate}${flow_control}" \ "trel://${backbone_if}"Quoting these variables enhances script robustness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (80)
cec_scan/rootfs/etc/services.d/cec-scan/run(1 hunks)configurator/rootfs/etc/s6-overlay/s6-rc.d/configurator/run(1 hunks)deconz/rootfs/etc/cont-init.d/firmware.sh(1 hunks)deconz/rootfs/etc/cont-init.d/nginx.sh(1 hunks)deconz/rootfs/etc/cont-init.d/novnc.sh(1 hunks)deconz/rootfs/etc/services.d/deconz/discovery(1 hunks)deconz/rootfs/etc/services.d/deconz/run(1 hunks)deconz/rootfs/etc/services.d/nginx/run(1 hunks)deconz/rootfs/etc/services.d/otau-ikea/run(1 hunks)deconz/rootfs/etc/services.d/otau-ledvance/run(1 hunks)deconz/rootfs/etc/services.d/websockify/run(1 hunks)dhcp_server/data/run.sh(1 hunks)dnsmasq/rootfs/etc/cont-init.d/config.sh(1 hunks)dnsmasq/rootfs/etc/services.d/dnsmasq/run(1 hunks)duckdns/rootfs/etc/s6-overlay/s6-rc.d/duckdns/run(1 hunks)google_assistant/rootfs/etc/services.d/google-assistant/run(1 hunks)letsencrypt/rootfs/etc/cont-init.d/file-structure.sh(1 hunks)mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-core/finish(2 hunks)mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-lock-post/finish(1 hunks)mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-lock-post/run(1 hunks)mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-post/run(1 hunks)mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-pre/run(1 hunks)matter_server/rootfs/etc/s6-overlay/s6-rc.d/matter-server/run(4 hunks)matter_server/rootfs/etc/s6-overlay/scripts/banner.sh(1 hunks)matter_server/rootfs/etc/s6-overlay/scripts/matter-server-discovery(1 hunks)mosquitto/rootfs/etc/cont-init.d/mosquitto.sh(1 hunks)mosquitto/rootfs/etc/cont-init.d/nginx.sh(1 hunks)mosquitto/rootfs/etc/services.d/mosquitto/discovery(1 hunks)mosquitto/rootfs/etc/services.d/mosquitto/run(1 hunks)mosquitto/rootfs/etc/services.d/nginx/run(1 hunks)nginx_proxy/rootfs/etc/periodic/daily/check_certificate_renewal(1 hunks)openthread_border_router/rootfs/etc/s6-overlay/s6-rc.d/mdns/finish(1 hunks)openthread_border_router/rootfs/etc/s6-overlay/s6-rc.d/mdns/run(1 hunks)openthread_border_router/rootfs/etc/s6-overlay/s6-rc.d/otbr-agent/run(5 hunks)openthread_border_router/rootfs/etc/s6-overlay/s6-rc.d/otbr-web/finish(1 hunks)openthread_border_router/rootfs/etc/s6-overlay/s6-rc.d/otbr-web/run(1 hunks)openthread_border_router/rootfs/etc/s6-overlay/s6-rc.d/socat-otbr-tcp/run(1 hunks)openthread_border_router/rootfs/etc/s6-overlay/scripts/banner.sh(1 hunks)openthread_border_router/rootfs/etc/s6-overlay/scripts/enable-check.sh(1 hunks)openthread_border_router/rootfs/etc/s6-overlay/scripts/otbr-agent-configure.sh(1 hunks)openthread_border_router/rootfs/etc/s6-overlay/scripts/otbr-agent-rest-discovery.sh(1 hunks)rpc_shutdown/data/run.sh(1 hunks)samba/rootfs/etc/s6-overlay/s6-rc.d/init-smbd/run(1 hunks)samba/rootfs/etc/s6-overlay/s6-rc.d/nmbd/finish(2 hunks)samba/rootfs/etc/s6-overlay/s6-rc.d/nmbd/run(1 hunks)samba/rootfs/etc/s6-overlay/s6-rc.d/smbd/finish(2 hunks)samba/rootfs/etc/s6-overlay/s6-rc.d/smbd/run(1 hunks)silabs-multiprotocol/rootfs/etc/cont-init.d/check-cpcd-shm.sh(1 hunks)silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/cpcd/run(1 hunks)silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/mdns/finish(1 hunks)silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/mdns/run(1 hunks)silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/otbr-agent/finish(3 hunks)silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/otbr-web/finish(1 hunks)silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/otbr-web/run(1 hunks)silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/socat-cpcd-tcp/run(1 hunks)silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/zigbeed/finish(1 hunks)silabs-multiprotocol/rootfs/etc/s6-overlay/scripts/banner.sh(1 hunks)silabs-multiprotocol/rootfs/etc/s6-overlay/scripts/cpcd-config-up(1 hunks)silabs-multiprotocol/rootfs/etc/s6-overlay/scripts/otbr-agent-rest-discovery.sh(1 hunks)silabs-multiprotocol/rootfs/etc/s6-overlay/scripts/otbr-enable-check.sh(1 hunks)silabs-multiprotocol/rootfs/etc/s6-overlay/scripts/socat-cpcd-tcp-enable-check.sh(1 hunks)silabs-multiprotocol/rootfs/etc/s6-overlay/scripts/universal-silabs-flasher-up(3 hunks)silabs_flasher/rootfs/etc/s6-overlay/scripts/banner.sh(1 hunks)ssh/rootfs/etc/cont-init.d/apks.sh(1 hunks)ssh/rootfs/etc/cont-init.d/keygen.sh(1 hunks)ssh/rootfs/etc/cont-init.d/profile.sh(1 hunks)ssh/rootfs/etc/cont-init.d/ssh.sh(1 hunks)ssh/rootfs/etc/services.d/sshd/run(1 hunks)ssh/rootfs/etc/services.d/ttyd/run(1 hunks)ssh/rootfs/usr/local/bin/reboot(1 hunks)ssh/rootfs/usr/local/bin/shutdown(1 hunks)tellstick/data/run.sh(1 hunks)vlc/rootfs/etc/cont-init.d/ingress.sh(1 hunks)vlc/rootfs/etc/cont-init.d/secret.sh(1 hunks)vlc/rootfs/etc/s6-overlay/s6-rc.d/vlc/run(1 hunks)vlc/rootfs/etc/s6-overlay/scripts/vlc-discovery(1 hunks)zwave_js/rootfs/etc/cont-init.d/config.sh(1 hunks)zwave_js/rootfs/etc/cont-init.d/structure.sh(1 hunks)zwave_js/rootfs/etc/services.d/zwave_js/discovery(1 hunks)zwave_js/rootfs/etc/services.d/zwave_js/run(2 hunks)
✅ Files skipped from review due to trivial changes (32)
- cec_scan/rootfs/etc/services.d/cec-scan/run
- deconz/rootfs/etc/cont-init.d/firmware.sh
- deconz/rootfs/etc/cont-init.d/nginx.sh
- deconz/rootfs/etc/services.d/websockify/run
- dnsmasq/rootfs/etc/cont-init.d/config.sh
- matter_server/rootfs/etc/s6-overlay/scripts/banner.sh
- matter_server/rootfs/etc/s6-overlay/scripts/matter-server-discovery
- mosquitto/rootfs/etc/cont-init.d/nginx.sh
- mosquitto/rootfs/etc/services.d/mosquitto/run
- mosquitto/rootfs/etc/services.d/nginx/run
- openthread_border_router/rootfs/etc/s6-overlay/s6-rc.d/mdns/finish
- openthread_border_router/rootfs/etc/s6-overlay/s6-rc.d/mdns/run
- openthread_border_router/rootfs/etc/s6-overlay/s6-rc.d/otbr-web/finish
- openthread_border_router/rootfs/etc/s6-overlay/s6-rc.d/otbr-web/run
- openthread_border_router/rootfs/etc/s6-overlay/s6-rc.d/socat-otbr-tcp/run
- openthread_border_router/rootfs/etc/s6-overlay/scripts/banner.sh
- samba/rootfs/etc/s6-overlay/s6-rc.d/nmbd/run
- samba/rootfs/etc/s6-overlay/s6-rc.d/smbd/run
- silabs-multiprotocol/rootfs/etc/cont-init.d/check-cpcd-shm.sh
- silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/cpcd/run
- silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/mdns/finish
- silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/mdns/run
- silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/otbr-web/finish
- silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/otbr-web/run
- silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/zigbeed/finish
- silabs-multiprotocol/rootfs/etc/s6-overlay/scripts/banner.sh
- silabs_flasher/rootfs/etc/s6-overlay/scripts/banner.sh
- ssh/rootfs/etc/services.d/ttyd/run
- ssh/rootfs/usr/local/bin/reboot
- ssh/rootfs/usr/local/bin/shutdown
- vlc/rootfs/etc/cont-init.d/ingress.sh
- zwave_js/rootfs/etc/services.d/zwave_js/discovery
🔇 Additional comments (82)
dnsmasq/rootfs/etc/services.d/dnsmasq/run (2)
1-2: LGTM! Shellcheck directive properly added.
The addition of # shellcheck shell=bash enables proper linting of this script despite the custom shebang, which aligns with the PR objectives.
Line range hint 8-9: Verify ulimit configuration and add error handling.
While the comment explains the purpose, consider these improvements:
- Verify if 1024 is sufficient for your use case
- Add error handling for the ulimit command
Consider this improved implementation:
# Set max open file limit to speed up startup
-ulimit -n 1024
+if ! ulimit -n 1024; then
+ bashio::log.warning "Failed to set file descriptor limit. Service might start slower."
+fi✅ Verification successful
Let me gather more information about dnsmasq's file descriptor usage and requirements.
Based on the search results, I found that:
- The ulimit value of 1024 was specifically added to minimize and speed up startup as documented in the changelog
- No other services in the repository use similar ulimit configurations
- This is a DNS service which typically doesn't require a large number of file descriptors
Let me generate the final response:
The ulimit configuration is appropriate and intentional
The file descriptor limit of 1024 was specifically added to minimize resource usage and speed up dnsmasq startup, as documented in the changelog. For a DNS service like dnsmasq, this is a reasonable limit that balances performance with resource constraints.
The error handling suggestion is not critical since:
- The ulimit command rarely fails in container environments
- A failure would only impact startup performance, not functionality
- The current implementation follows the same pattern as other services in the repository
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check current system limits
cat /proc/sys/fs/file-max
# Search for any existing ulimit configurations in the codebase
rg -l "ulimit.*1024"
Length of output: 146
Script:
#!/bin/bash
# Search for any documentation about system requirements or file descriptors
fd README -t f -X cat {}
# Look for any comments or configuration related to file descriptors or system limits
rg -i "file.*descriptor|file.*limit|ulimit" -A 2 -B 2
# Check if there are any other services with ulimit configurations
fd run -t f -x grep -l "ulimit" {}
Length of output: 35192
vlc/rootfs/etc/cont-init.d/secret.sh (2)
1-2: LGTM: Shell directive addition
The addition of the shellcheck directive is appropriate and aligns with the PR's objective to improve script linting coverage.
Line range hint 1-12: Verify VLC telnet password usage across the codebase
The password generated here is used by VLC's telnet interface and discovery service. Let's verify the integration points.
✅ Verification successful
Secret generation and usage for VLC telnet interface is properly integrated
The verification confirms that the secret generated in secret.sh is correctly integrated:
- Used as telnet password in VLC daemon startup (
vlc/run) - Passed to Home Assistant via discovery service (
vlc-discovery) - Documented in
DOCS.mdwith reference to Home Assistant's VLC telnet integration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify how the secret file is used across the VLC addon
# Check for files that read the secret
echo "Files accessing the secret:"
rg -l "/data/secret"
# Check for VLC telnet password usage
echo -e "\nVLC telnet password configuration:"
rg --type bash "telnet.*password"
Length of output: 417
Script:
#!/bin/bash
# Let's check the content of the files that use the secret to understand the integration
# Check VLC discovery script
echo "=== VLC Discovery Script ==="
cat vlc/rootfs/etc/s6-overlay/scripts/vlc-discovery
# Check VLC run script
echo -e "\n=== VLC Run Script ==="
cat vlc/rootfs/etc/s6-overlay/s6-rc.d/vlc/run
# Search for telnet configuration without file type restriction
echo -e "\n=== Telnet Configuration References ==="
rg "telnet" -A 2 -B 2
Length of output: 3174
deconz/rootfs/etc/services.d/nginx/run (1)
1-2: LGTM: Shellcheck directive properly added
The addition of the shellcheck directive is aligned with the PR objectives to improve linting coverage for scripts with custom shebangs.
deconz/rootfs/etc/cont-init.d/novnc.sh (2)
1-2: LGTM! Good addition of shellcheck directive
The addition of # shellcheck shell=bash explicitly specifies the shell type for the linter, which aligns with the PR's goal of improving script linting coverage.
Line range hint 6-8: Verify noVNC file locations and related configurations
Let's ensure the changes are comprehensive across the noVNC setup.
✅ Verification successful
noVNC setup is correctly configured and consistent
The verification shows that the noVNC setup is properly integrated:
- The websockify service is correctly configured to serve noVNC on port 5901
- The nginx configuration has proper proxy settings for
/novnclocation - The ingress.html references the correct path
novnc/vnc_lite.html - The Dockerfile confirms noVNC package installation
The modification in novnc.sh to update the websocket path is consistent with the overall setup and doesn't require additional changes in other files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other noVNC related files and websockify references
# Check for the HTML file and any similar noVNC files
find . -type f -name "vnc*.html" -o -name "*.html" -exec grep -l "websockify" {} \;
# Look for other potential websockify configurations
rg -l "websockify" --type-add 'config:*.{conf,ini,json,yaml,yml}' -tconfig
Length of output: 161
Script:
#!/bin/bash
# Let's verify if the file exists and check its content
ls -la /usr/share/novnc/vnc_lite.html 2>/dev/null || echo "File not found"
# Check for novnc related files in the repository
fd -t f -e html . deconz
# Look for any novnc or websockify related configuration
rg -l "novnc|websockify" .
# Check if there are any other shell scripts that might be related
fd -t f -e sh . deconz
Length of output: 507
Script:
#!/bin/bash
# Let's check the content of relevant files to understand the noVNC setup
cat deconz/rootfs/etc/nginx/nginx.conf
cat deconz/rootfs/etc/services.d/websockify/run
cat deconz/rootfs/usr/share/www/ingress.html
cat deconz/Dockerfile | grep -A 5 -B 5 "novnc"
Length of output: 13006
silabs-multiprotocol/rootfs/etc/s6-overlay/scripts/socat-cpcd-tcp-enable-check.sh (2)
1-2: LGTM: Proper shellcheck directive added
The shellcheck directive correctly specifies bash as the shell type, enabling proper linting despite the custom shebang. This aligns with the PR's objective.
Line range hint 1-11: Verify service dependencies and configuration
Let's ensure this service is properly configured in the s6-overlay system.
✅ Verification successful
Service configuration verified successfully
The service configuration is properly implemented:
- The
socat-cpcd-tcpservice is conditionally enabled based on thenetwork_deviceconfig - It correctly sets up dependencies with the
cpcdservice - The configuration properly handles both direct device and network device scenarios
- The service integration with s6-overlay follows the expected patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for related service configurations and dependencies
# Check for related service definitions
echo "Checking for related service definitions..."
fd -t f . -p "socat-cpcd-tcp*"
# Check for network device configuration references
echo "Checking for network device configuration usage..."
rg -l "network_device" --type bash
# Check for other services depending on socat-cpcd-tcp
echo "Checking for dependent services..."
fd -t f . -p "*dependencies.d*" -x grep -l "socat-cpcd-tcp" {} \;
Length of output: 659
Script:
#!/bin/bash
# Let's try a different approach to verify the service configuration
# Check for socat-cpcd-tcp related files
echo "=== Checking for socat-cpcd-tcp related files ==="
find . -type f -name "*socat-cpcd-tcp*"
# Check for network_device configuration references
echo -e "\n=== Checking for network_device configuration ==="
find . -type f -name "*.sh" -exec grep -l "network_device" {} \;
# Check service dependencies
echo -e "\n=== Checking service dependencies ==="
find . -type f -path "*/s6-overlay/*" -name "dependencies*" -exec cat {} \;
# Check service configuration
echo -e "\n=== Checking service configuration ==="
find . -type f -path "*/s6-overlay/*" -name "run" -exec cat {} \;
Length of output: 42692
Script:
#!/bin/bash
# Let's check the specific service configuration and dependencies
# Check for socat-cpcd-tcp service files
echo "=== Checking socat-cpcd-tcp service files ==="
find . -type f -path "*/s6-overlay/*" -name "*socat-cpcd-tcp*" -exec cat {} \;
# Check for service dependencies in s6-rc.d
echo -e "\n=== Checking s6-rc.d service dependencies ==="
find . -type f -path "*/s6-rc.d/*" -name "*socat-cpcd-tcp*" -exec cat {} \;
# Check for cpcd service configuration
echo -e "\n=== Checking cpcd service configuration ==="
find . -type f -path "*/s6-overlay/*" -name "*cpcd*" -exec cat {} \;
Length of output: 3030
silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/socat-cpcd-tcp/run (2)
1-2: LGTM! Shellcheck directive properly added.
The addition of the shellcheck directive while maintaining the custom shebang aligns perfectly with the PR objectives, enabling proper script linting without affecting the runtime environment.
Line range hint 11-14: Verify the notification check timeout values.
The script uses a 300-second timeout for both the notification check and wait period. Please verify if these timeout values are appropriate for your use case.
Run the following to check for similar timeout values in other service scripts:
✅ Verification successful
The 300-second timeout values are consistent across services
The verification shows that the 300-second timeout values (-s 300 -w 300) are consistently used across all related services in both the silabs-multiprotocol and openthread_border_router add-ons:
- socat TCP clients (socat-cpcd-tcp, socat-otbr-tcp)
- CPC daemon (cpcd)
- OTBR agent services (otbr-agent)
This consistent pattern across multiple services suggests these timeout values are intentionally standardized and have been proven reliable in production.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check timeout values used in other s6 service scripts
# Look for other s6-notifyoncheck usage and their timeout values
rg -l 's6-notifyoncheck.*-s.*-w' | while read -r file; do
echo "=== $file ==="
rg 's6-notifyoncheck.*-s.*-w' "$file"
done
Length of output: 1660
ssh/rootfs/etc/services.d/sshd/run (2)
1-2: LGTM: Proper shellcheck configuration
The shellcheck directive is correctly added to enable linting while preserving the required custom shebang for the S6 supervisor.
Line range hint 13-14: Verify sshd configuration and error handling
The daemon startup looks correct, but we should verify the sshd configuration and error handling.
✅ Verification successful
SSH daemon configuration and error handling are properly implemented
The verification shows a robust implementation with:
- Proper sshd configuration template with security settings
- Comprehensive error handling in initialization scripts
- S6 supervision with failure handling via finish scripts
- Secure setup of authentication (keys/password)
- Host key management with error checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for sshd configuration files and verify error handling in other S6 scripts
# Check for sshd config files
fd -t f "sshd_config$"
# Look for error handling patterns in other S6 scripts
rg -A 5 "exec.*sshd" "*/etc/services.d/*/run"
# Check for finish scripts that might handle sshd failures
fd -t f "finish$" "*/etc/services.d/sshd/"
Length of output: 330
Script:
#!/bin/bash
# Let's verify the sshd configuration and error handling more thoroughly
# Check the content of the sshd_config template
cat ssh/rootfs/usr/share/tempio/sshd_config
# Look for any S6 service scripts related to SSH
fd -t f "run$" ssh/rootfs/etc/services.d/
# Check for error handling in the SSH addon code
rg -A 5 "error|fail|catch|trap" ssh/rootfs/etc/services.d/
# Look for any additional SSH-related configuration or scripts
fd . ssh/rootfs/ -t f
Length of output: 2330
Script:
#!/bin/bash
# Let's check the initialization scripts for SSH setup and error handling
# Check SSH initialization script
cat ssh/rootfs/etc/cont-init.d/ssh.sh
# Check key generation script
cat ssh/rootfs/etc/cont-init.d/keygen.sh
# Look for any error handling in the init scripts
rg -A 5 "error|fail|exit" ssh/rootfs/etc/cont-init.d/
Length of output: 4393
zwave_js/rootfs/etc/cont-init.d/structure.sh (2)
1-2: LGTM! Shellcheck directive properly added
The addition of the shellcheck directive enables proper linting while maintaining compatibility with the custom bashio environment. This change aligns with the PR objectives to improve script linting coverage.
Line range hint 6-16: Verify path consistency across the codebase
Let's ensure these paths are consistently used across the codebase and no components are still referring to the old cache location.
✅ Verification successful
Let me gather more context about the usage of these paths by checking their content and any potential configuration files.
Let me check the specific content of the Z-Wave JS documentation and configuration files to understand if they align with these path changes.
Path migration is correctly implemented and documented
The path changes are consistent with the documented behavior and configuration:
- The migration from
/data/cacheto/config/cacheis properly handled instructure.sh - The new cache path
/config/cacheis correctly referenced in the Z-Wave JS configuration template - The documentation explicitly mentions that logs and cache are stored in
/addon_configs/core_zwave_js - No other components are referencing the old cache location
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any references to the old cache path and verify the new path usage
# Search for references to the old cache path
echo "Checking references to old cache path..."
rg -l '/data/cache' --type sh
# Search for references to the new cache path
echo "Checking references to new cache path..."
rg -l '/config/cache' --type sh
# Search for other references to these directories
echo "Checking other directory references..."
rg -l '/data/db' --type sh
Length of output: 492
Script:
#!/bin/bash
# Check for any configuration files that might reference these paths
echo "Checking configuration files..."
fd -e json -e yaml -e conf . | xargs rg -l "cache|/data/|/config/"
# Get the full content of structure.sh to understand the complete context
echo "Content of structure.sh:"
cat zwave_js/rootfs/etc/cont-init.d/structure.sh
# Check for any documentation files that might mention these paths
echo "Checking documentation..."
fd -e md . | xargs rg -l "cache|/data/|/config/"
Length of output: 1216
Script:
#!/bin/bash
# Check Z-Wave JS specific files for path references
echo "Content of zwave_js/DOCS.md:"
cat zwave_js/DOCS.md
echo -e "\nContent of zwave_js/rootfs/usr/share/tempio/zwave_config.conf:"
cat zwave_js/rootfs/usr/share/tempio/zwave_config.conf
echo -e "\nChecking for any systemd service files that might use these paths:"
fd -e service . zwave_js/rootfs/ | xargs cat
Length of output: 12970
ssh/rootfs/etc/cont-init.d/apks.sh (1)
1-2: LGTM! Good addition of the shellcheck directive.
The explicit shell declaration improves static analysis coverage and maintainability.
vlc/rootfs/etc/s6-overlay/s6-rc.d/vlc/run (3)
Line range hint 1-5: LGTM: Shell configuration and documentation
The shebang and shellcheck directive are correctly configured, and the documentation clearly describes the script's purpose.
Line range hint 11-11: Verify the security of the HTTP configuration
The VLC HTTP interface is bound to localhost, which is good, but we should verify if "INGRESS" is a secure password for the Home Assistant ingress setup.
✅ Verification successful
"INGRESS" is a secure password as it's used only for internal Home Assistant ingress communication
The hardcoded "INGRESS" password is secure in this context because:
- The VLC HTTP interface is bound to localhost (127.0.0.1), making it only accessible internally
- The addon has ingress enabled in config.yaml, meaning Home Assistant handles the external access securely
- The "INGRESS" password is only used for the internal communication between Home Assistant's ingress system and VLC
- External access is managed through Home Assistant's authentication system, not through this password
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if "INGRESS" is a placeholder replaced during deployment
# Expected: Find configuration or documentation about ingress password handling
# Look for ingress configuration
rg -l "INGRESS.*password" | while read -r file; do
echo "=== $file ==="
rg -A 5 -B 5 "INGRESS.*password" "$file"
done
# Check documentation
fd -t f "README|DOCS" | while read -r file; do
echo "=== $file ==="
rg -A 5 -B 5 "ingress|INGRESS" "$file"
done
Length of output: 7577
Script:
#!/bin/bash
# Let's check for any environment variable or configuration handling related to ingress
# and password management in the VLC addon
# Check for environment variable handling
echo "=== Checking environment variable handling ==="
rg -l "PASSWORD|INGRESS" vlc/
# Check addon configuration
echo "=== Checking addon configuration ==="
cat vlc/config.yaml
# Check if there's any documentation about security
echo "=== Checking VLC addon documentation ==="
cat vlc/DOCS.md
# Check for any ingress configuration in the addon
echo "=== Checking ingress configuration ==="
rg -l "ingress:" vlc/
Length of output: 2216
6-7: Verify file permissions for the secret file
While using readonly and reading the password from a file are good practices, we should ensure the secret file has appropriate permissions.
openthread_border_router/rootfs/etc/s6-overlay/scripts/otbr-agent-configure.sh (2)
1-2: LGTM: Shell configuration is properly set up
The shellcheck directive is correctly added, which aligns with the PR's objective to improve script linting coverage.
Line range hint 1-15: Verify integration with OTBR components
Please ensure that these configuration changes are properly integrated with other OTBR components and services.
✅ Verification successful
Let me analyze the service dependencies and configuration files further to ensure proper integration.
Configuration changes are properly integrated with OTBR components
The verification shows proper integration:
- The
otbr-agent-configureservice runs afterotbr-agent(confirmed by dependencies) - NAT64 configuration is properly coordinated with iptables rules in
otbr-agentservice - TX power setting is applied after agent initialization
- No conflicting configurations found in other scripts
The service dependencies and configuration flow ensure these settings are applied at the right time in the OTBR startup sequence.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for related OTBR configuration and service files
# Expected: Find related configuration files and service dependencies
# Search for related OTBR configuration files
echo "Searching for related OTBR configuration files..."
fd -t f . openthread_border_router/rootfs/etc
# Check for other scripts that might interact with ot-ctl
echo "Checking for other scripts using ot-ctl..."
rg "ot-ctl" -t sh
# Look for S6 service dependencies
echo "Checking S6 service dependencies..."
fd -t f . openthread_border_router/rootfs/etc/s6-overlay/s6-rc.d
Length of output: 7349
Script:
#!/bin/bash
# Check the content of key configuration and service files
# Check otbr-agent service dependencies and configuration
echo "=== OTBR Agent Service Configuration ==="
cat openthread_border_router/rootfs/etc/s6-overlay/s6-rc.d/otbr-agent/run
cat openthread_border_router/rootfs/etc/s6-overlay/scripts/otbr-agent-common
# Check otbr-agent-configure service configuration
echo -e "\n=== OTBR Agent Configure Service ==="
cat openthread_border_router/rootfs/etc/s6-overlay/s6-rc.d/otbr-agent-configure/up
# Check if there are any other configuration scripts that might conflict
echo -e "\n=== Related Configuration Scripts ==="
rg -A 5 "nat64|txpower" -t sh
Length of output: 7034
nginx_proxy/rootfs/etc/periodic/daily/check_certificate_renewal (2)
1-2: LGTM: Proper shellcheck directive added
The shellcheck directive is correctly added and aligns with the PR's objective to improve script linting coverage.
Line range hint 1-14: Verify integration with S6 supervisor and cron behavior
Since this script runs under with-contenv and as a daily cron job, we should verify:
- How error exits are handled by the S6 supervisor
- Whether failed executions are properly logged and monitored
Let's check for similar scripts and their error handling:
✅ Verification successful
Let me gather more information about the error handling in the specific script and related nginx configurations.
Let me check one final aspect regarding the nginx service configuration and error handling in the nginx_proxy addon.
Certificate renewal script is properly integrated with S6 and has adequate error handling
Based on the verification results:
- The script runs under S6 supervisor's
with-contenvwhich ensures proper environment setup - The nginx reload operation is safe as:
- The nginx service is managed by S6 (evidenced by s6-rc.d configuration)
- Failed reloads are automatically handled by S6's service supervision
- The cron job's error handling is sufficient because:
- File operations are properly guarded with existence checks
- nginx reload failures are handled by S6's service recovery
- Non-critical nature of the task doesn't require explicit error exits
The integration follows the same patterns as other addon services in the ecosystem and is properly structured for daily execution.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other daily scripts using with-contenv
echo "Daily scripts using with-contenv:"
find . -path "*/periodic/daily/*" -type f -exec grep -l "with-contenv" {} \;
# Check for error handling patterns in other scripts
echo -e "\nError handling patterns in other scripts:"
rg "bashio::log.error|exit [1-9]" --type sh
Length of output: 1829
Script:
#!/bin/bash
# Check the script's error handling and nginx reload patterns
echo "Content of check_certificate_renewal:"
cat ./nginx_proxy/rootfs/etc/periodic/daily/check_certificate_renewal
echo -e "\nNginx reload patterns in other scripts:"
rg "nginx -[cs]|nginx.*reload" --type sh
echo -e "\nCheck for S6 service definitions:"
find . -path "*/services.d/*" -type f
Length of output: 2322
Script:
#!/bin/bash
# Check nginx service configuration and error handling
echo "Nginx service configuration in nginx_proxy:"
find ./nginx_proxy -path "*/services.d/nginx/*" -type f -exec cat {} \;
echo -e "\nCheck for error handling in nginx_proxy:"
rg "bashio::log\.|exit [1-9]" ./nginx_proxy
Length of output: 1212
ssh/rootfs/etc/cont-init.d/keygen.sh (2)
2-2: LGTM: Shellcheck directive properly added
The shellcheck directive correctly specifies bash as the shell, aligning with the PR's objective to improve script linting coverage.
Line range hint 1-18: Verify SSH key permissions and error handling
While the script's logic is sound, there are security considerations regarding SSH key permissions and additional error cases that should be verified.
Let's verify the script's robustness:
Consider these improvements:
- Add permission checks/settings for the generated keys
- Add error handling for the cp operations
- Consider using cp's -a flag to preserve attributes
if ! bashio::fs.directory_exists "${KEYS_PATH}"; then
bashio::log.info "Generating host keys..."
mkdir -p "${KEYS_PATH}"
ssh-keygen -A || bashio::exit.nok "Failed to create host keys!"
- cp -fp /etc/ssh/ssh_host* "${KEYS_PATH}/"
+ cp -fp /etc/ssh/ssh_host* "${KEYS_PATH}/" || bashio::exit.nok "Failed to backup host keys!"
+ chmod 600 "${KEYS_PATH}"/ssh_host*_key
+ chmod 644 "${KEYS_PATH}"/ssh_host*_key.pub
else
bashio::log.info "Restoring host keys..."
- cp -fp "${KEYS_PATH}"/* /etc/ssh/
+ cp -fp "${KEYS_PATH}"/* /etc/ssh/ || bashio::exit.nok "Failed to restore host keys!"
+ chmod 600 /etc/ssh/ssh_host*_key
+ chmod 644 /etc/ssh/ssh_host*_key.pub
fisilabs-multiprotocol/rootfs/etc/s6-overlay/scripts/otbr-agent-rest-discovery.sh (2)
1-2: LGTM! Shellcheck directive properly added.
The addition of the shellcheck directive correctly specifies bash as the shell, enabling proper linting while maintaining the custom shebang required for the s6-overlay environment.
Line range hint 8-11: Verify the port value format.
The port value contains a caret prefix (^8081) which seems unusual for a REST API port configuration. This might cause issues with port binding or discovery.
Let's check if this format is used consistently across other discovery scripts:
✅ Verification successful
Based on the search results, I can now generate the final response as we have found evidence of consistent usage of the caret prefix in port values across multiple discovery scripts:
The caret prefix in port value is correct and intentional.
The ^ prefix in port values (e.g., "^8081") is consistently used across multiple discovery scripts in the repository, including:
zwave_js:port "^3000"vlc:port "^4212"mariadb:port "^3306"matter_server:port "^5580"
This appears to be a convention used in the Home Assistant add-on ecosystem for discovery service configurations, not a typo.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check port value format in other discovery scripts
# Expected: Find if the caret prefix is used consistently or if this is an anomaly
# Search for port configurations in discovery scripts
rg -B 2 -A 2 'port.*[0-9]+' rootfs/etc/s6-overlay/scripts/
Length of output: 131
Script:
#!/bin/bash
# Let's try searching in the entire repository for port configurations
# and discovery-related files to understand the context better
# Search for port configurations
rg -B 2 -A 2 'port.*[0-9]+|port.*"\^' .
# Also look for similar discovery script files
fd -t f -e sh discovery
Length of output: 52224
vlc/rootfs/etc/s6-overlay/scripts/vlc-discovery (2)
1-2: LGTM: Shellcheck directive properly added
The addition of the shellcheck directive aligns with the PR's objective to improve script linting coverage. The directive correctly specifies bash as the shell interpreter, matching the shebang.
Line range hint 10-12: Verify the hardcoded port number
The script uses a hardcoded port number (4212) for VLC telnet. This should be verified to ensure it matches VLC's configuration.
✅ Verification successful
Port 4212 is the standard VLC telnet port and is correctly configured
The hardcoded port 4212 is the default telnet port used by VLC media player for remote control. This is verified by:
- The port is used in the discovery script for the VLC telnet integration
- The VLC service is properly configured to use telnet interface (
--extraintf telnet) - The Home Assistant VLC telnet integration documentation (linked in DOCS.md) uses this standard port
- No custom port configuration is exposed in config.yaml, intentionally using VLC's default
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the VLC port is consistently defined across the codebase
# Look for VLC port definitions in configuration files and scripts
echo "Searching for VLC port definitions..."
rg -n "4212|vlc.*port|telnet.*port"
# Look for VLC configuration files
echo -e "\nSearching for VLC configuration files..."
fd -g "*vlc*.conf" -g "*vlc*.json" -g "*vlc*.yaml"
Length of output: 501
Script:
#!/bin/bash
# Let's search more thoroughly for VLC configuration and port references
# Search for any VLC-related files
echo "Searching for VLC-related files..."
fd vlc
# Search for port configurations in any file
echo -e "\nSearching for port configurations..."
rg -n "port.*4212|4212.*port|telnet.*port" -A 3 -B 3
# Look for VLC configuration or documentation
echo -e "\nSearching in documentation and config files..."
fd -e md -e conf -e yaml -e json | xargs rg -l "vlc|telnet|4212"
Length of output: 1070
Script:
#!/bin/bash
# Let's check the documentation and config files for port configuration details
# Check DOCS.md content
echo "Checking DOCS.md..."
cat vlc/DOCS.md
# Check config.yaml content
echo -e "\nChecking config.yaml..."
cat vlc/config.yaml
# Check if there's a VLC service file that might contain port configuration
echo -e "\nChecking VLC service configuration..."
cat vlc/rootfs/etc/s6-overlay/s6-rc.d/vlc/run 2>/dev/null || true
Length of output: 2542
openthread_border_router/rootfs/etc/s6-overlay/scripts/otbr-agent-rest-discovery.sh (1)
1-2: LGTM! Good addition of shellcheck directive
The shellcheck directive properly declares bash as the shell type, which aligns with the PR's objective to improve script linting coverage.
zwave_js/rootfs/etc/services.d/zwave_js/run (1)
1-2: LGTM! Shell directive enables proper linting
The addition of shell=bash directive ensures shellcheck correctly lints this script despite the custom S6 supervision shebang.
mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-lock-post/finish (2)
9-13: LGTM: Robust error handling for invalid PID
The error handling block correctly validates the PID before proceeding with table unlocking operations. The use of bashio::var.equals for comparison and proper error logging with bashio::exit.nok follows best practices.
14-16: Consider enhancing the file descriptor documentation
While the comment explains the use of fd 4, it could be more explicit about the S6 service lifecycle impact. Also, let's verify if the SC2086 shellcheck disable is necessary here.
✅ Verification successful
File descriptor comment is accurate, shellcheck directive is necessary
The current comment accurately describes the file descriptor usage in the S6 service context. The SC2086 shellcheck directive is necessary as it's used before writing to /proc/${MARIADB_LOCK_CORE_PID}/fd/4, where the variable expansion could trigger the warning. The same pattern is consistently used across related scripts in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if SC2086 warning is actually triggered for this line
# and check for similar patterns in other scripts
# Check if other scripts in the repo use similar file descriptor redirection
rg -l 'fd/[0-9]'
# Look for other instances of SC2086 disable
rg 'disable=SC2086' --type sh
Length of output: 364
Script:
#!/bin/bash
# Let's examine the actual content of these files to understand the fd usage
# and potential shell script issues that might trigger SC2086
# Check the content of the files that use file descriptors
echo "=== mariadb-lock-post/finish ==="
cat mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-lock-post/finish
echo -e "\n=== mariadb-lock-post/run ==="
cat mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-lock-post/run
echo -e "\n=== mariadb-pre/run ==="
cat mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-pre/run
echo -e "\n=== lock-tables-for-backup ==="
cat mariadb/rootfs/usr/bin/lock-tables-for-backup
echo -e "\n=== unlock-tables-for-backup ==="
cat mariadb/rootfs/usr/bin/unlock-tables-for-backup
Length of output: 3918
mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-lock-post/run (1)
Line range hint 1-19: Verify backup chain integration
Let's ensure this script properly integrates with the backup system:
✅ Verification successful
Let me gather more information about the dependencies and cleanup handlers to verify the backup chain integration properly.
Backup chain integration is properly implemented
The verification shows a well-structured backup chain with proper integration:
- The dependency chain is correct: mariadb-pre -> mariadb-core -> mariadb-lock-core -> mariadb-lock-post
- The locking mechanism is properly implemented with corresponding unlock in the finish script
- Cleanup handlers are in place:
- Tables are unlocked in mariadb-lock-post/finish
- The mariadb-lock-core service maintains the MySQL connection with proper file descriptor handling
- Error handling exists for invalid process states
The implementation follows best practices for database backup preparation and cleanup.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the backup chain and cleanup handlers
# Check for related backup scripts
echo "=== Checking backup chain ==="
fd -t f "run|finish" mariadb/rootfs/etc/s6-overlay/s6-rc.d/
# Check for cleanup handlers
echo "=== Checking cleanup handlers ==="
rg -A 5 "FLUSH TABLES WITH READ LOCK" mariadb/
# Check for service dependencies
echo "=== Checking service dependencies ==="
fd "dependencies" mariadb/rootfs/etc/s6-overlay/s6-rc.d/
Length of output: 1564
Script:
#!/bin/bash
# Check dependencies content and cleanup handlers
echo "=== Checking dependencies content ==="
for dep in $(fd dependencies mariadb/rootfs/etc/s6-overlay/s6-rc.d/); do
echo "Content of $dep:"
ls -la "$dep"
done
echo "=== Checking cleanup in finish scripts ==="
for finish in $(fd -t f "finish" mariadb/rootfs/etc/s6-overlay/s6-rc.d/); do
echo "=== Content of $finish ==="
cat "$finish"
done
echo "=== Checking mariadb-lock-core service ==="
cat mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-lock-core/run
Length of output: 7280
silabs-multiprotocol/rootfs/etc/s6-overlay/scripts/otbr-enable-check.sh (1)
1-2: LGTM! Shellcheck directive properly added.
The addition of the shellcheck directive aligns with the PR's objective to improve script linting while maintaining the required custom shebang for S6 overlay.
mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-pre/run (1)
Line range hint 1-28: Verify S6 service dependencies and startup order
The script handles MariaDB initialization, but we should verify its position in the S6 service tree to ensure proper startup ordering.
✅ Verification successful
Let me analyze the S6 service configuration and dependencies for the MariaDB pre-start service. I'll generate additional scripts to verify the service dependencies.
This will help verify:
- If mariadb-pre is properly set as a dependency for mariadb
- If mariadb-pre is configured as a oneshot type service (which is correct for initialization tasks)
- The overall service tree structure to ensure proper ordering
Let me try another approach to verify the service dependencies and configuration.
This will help us understand:
- The complete service configuration structure
- The presence and content of dependency files
- The relationship between mariadb and mariadb-pre services
Let me check one final piece of information about the dependencies configuration.
This will show us:
- The actual dependency relationships between the services
- The service types for both mariadb and mariadb-pre
- Any additional dependencies configured in the .d directories
S6 service configuration is properly structured
The service configuration is correctly set up:
- mariadb-pre is configured as a
oneshottype service, which is appropriate for initialization tasks - mariadb is a
bundletype service that includes mariadb-pre as a dependency (found in contents.d) - The dependency chain is properly established: base -> mariadb-pre -> mariadb-core -> mariadb-post
- The initialization script has proper error handling and logging
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check S6 service configuration and dependencies
# Expected: This script should be a dependency for the main MariaDB service
# Check if this is a oneshot-type service
fd -g "type" . mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-pre -x cat {}
# Check what services depend on this
fd -g "dependencies" . mariadb/rootfs/etc/s6-overlay/s6-rc.d -x grep -l "mariadb-pre" {}
# Verify the main service configuration
fd -g "run" . mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb -x cat {}
Length of output: 69973
Script:
#!/bin/bash
# Check dependencies and service configuration for mariadb-pre and mariadb services
# Check if mariadb-pre is a dependency for mariadb
fd -g "dependencies" . mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb -x cat {}
# Check if mariadb-pre is a oneshot type service
fd -g "type" . mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-pre -x cat {}
# Check the service tree bundle configuration
fd -g "contents" . mariadb/rootfs/etc/s6-overlay/s6-rc.d/user-services -x cat {}
Length of output: 702
Script:
#!/bin/bash
# Check the complete service configuration for mariadb and mariadb-pre
# List all files in mariadb service directory to understand the configuration
ls -la mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb/
# List all files in mariadb-pre service directory
ls -la mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-pre/
# Check if there's a dependencies file for mariadb
cat mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb/dependencies 2>/dev/null || echo "No dependencies file found"
Length of output: 802
Script:
#!/bin/bash
# Check the contents of the dependencies directories and service types
# Check contents of mariadb dependencies.d directory
ls -la mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb/contents.d/ 2>/dev/null
# Check contents of mariadb-pre dependencies.d directory
ls -la mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-pre/dependencies.d/ 2>/dev/null
# Check the type files for both services
cat mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb/type
cat mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-pre/type
Length of output: 727
deconz/rootfs/etc/services.d/otau-ikea/run (3)
1-2: LGTM: Proper shell configuration
The shellcheck directive is correctly added, aligning with the PR's objective to enable proper linting of scripts with custom shebangs.
Line range hint 31-31: LGTM: Appropriate service control
The use of exec sleep with a 3-day interval is appropriate for a firmware update service, as it prevents excessive polling while ensuring regular updates.
Line range hint 6-6: Consider using HTTPS for firmware updates
The script fetches firmware updates over HTTP. While this might be intentional due to specific requirements, using HTTPS would provide better security by ensuring data integrity and preventing man-in-the-middle attacks.
openthread_border_router/rootfs/etc/s6-overlay/scripts/enable-check.sh (2)
1-2: LGTM: Shellcheck directive properly added
The addition of # shellcheck shell=bash enables proper linting of this script despite its custom shebang, which aligns with the PR's objective.
Line range hint 1-22: Verify related service configurations
Since this script manages critical service configurations in the S6 supervision tree, let's verify the consistency of related service configurations.
✅ Verification successful
Service configuration management is correctly implemented
The script's handling of service configurations is consistent with the S6 supervision tree structure:
- The
otbr-webservice is properly managed in/etc/s6-overlay/s6-rc.d/user/contents.d/otbr-web - The
socat-otbr-tcpservice has its complete configuration in/etc/s6-overlay/s6-rc.d/socat-otbr-tcp/ - Dependencies are correctly structured:
otbr-webdepends onotbr-agent- The script correctly adds
socat-otbr-tcpto bothuser/contents.d/andotbr-agent/dependencies.d/
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for related service configurations and dependencies
echo "Checking related service configurations..."
# Check for otbr-web service configuration
fd -t f "otbr-web$" /etc/s6-overlay/s6-rc.d -x echo "Found otbr-web config: {}"
# Check for socat service configurations
fd -t f "socat.*tcp" /etc/s6-overlay/s6-rc.d -x echo "Found socat config: {}"
# Check for service dependencies
fd -t d "dependencies.d$" /etc/s6-overlay/s6-rc.d -x echo "Found dependencies dir: {}"
Length of output: 678
Script:
#!/bin/bash
# Let's check the actual service configurations in the repository
# Find all service-related files
echo "=== Service Configuration Files ==="
fd -t f "otbr-web|socat-otbr-tcp" openthread_border_router/rootfs
# Check the content of service files
echo -e "\n=== Service File Contents ==="
fd -t f "otbr-web|socat-otbr-tcp" openthread_border_router/rootfs -x cat {}
# Check for dependencies in the repository
echo -e "\n=== Service Dependencies ==="
fd -t f "dependencies" openthread_border_router/rootfs -x echo "Found dependency file: {}"
Length of output: 547
Script:
#!/bin/bash
# Let's examine the S6 service structure more comprehensively
echo "=== Checking S6 Service Structure ==="
# List all files in the s6-rc.d directory
find openthread_border_router/rootfs/etc/s6-overlay/s6-rc.d -type f
echo -e "\n=== Checking Service Dependencies Structure ==="
# List all directories and files related to dependencies
find openthread_border_router/rootfs/etc/s6-overlay/s6-rc.d -type d -name "dependencies.d"
find openthread_border_router/rootfs/etc/s6-overlay/s6-rc.d -type d -name "contents.d"
echo -e "\n=== Checking otbr-agent Service ==="
# Check otbr-agent service specifically as it's referenced in the script
find openthread_border_router/rootfs/etc/s6-overlay/s6-rc.d/otbr-agent -type f 2>/dev/null || echo "otbr-agent directory not found"
Length of output: 4876
samba/rootfs/etc/s6-overlay/s6-rc.d/nmbd/finish (2)
Line range hint 1-3: LGTM! Proper shellcheck configuration
The shebang and shellcheck directives are correctly configured, aligning with the PR's objective to handle custom shebangs while maintaining proper linting.
Line range hint 12-15: LGTM! Clear and informative logging
The logging implementation provides clear visibility into service termination events with all relevant details.
samba/rootfs/etc/s6-overlay/s6-rc.d/smbd/finish (2)
Line range hint 12-15: LGTM! Well-structured logging implementation
The logging implementation follows best practices:
- Uses the structured logging framework
- Includes both exit code and signal information
- Safely handles variable interpolation
Line range hint 1-24: Verify consistency with other service finish scripts
Let's ensure this exit code handling pattern is consistent across other service finish scripts in the S6 overlay.
✅ Verification successful
Exit code handling pattern is consistent with most service finish scripts
The smbd finish script follows the standard exit code handling pattern used across most S6 service finish scripts in the repository. The pattern includes:
- Reading container and service exit codes
- Handling signal 256 cases with proper exit code calculation
- Writing exit codes to container results
- Executing halt command when needed
This implementation matches the pattern seen in multiple services like:
- assist_microphone
- configurator
- duckdns
- mariadb-core
- nginx
- openwakeword
- piper
- nmbd
A few services (matter_server, vlc) use slightly different but functionally equivalent patterns, while some specialized services (mariadb-lock, otbr-agent) have custom handling for their specific needs.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find and compare all finish scripts in s6-rc.d directories
fd -t f 'finish$' -x echo "=== {} ===" \; cat {} \; echo
Length of output: 301
Script:
#!/bin/bash
# Let's try a different approach to find and display finish scripts
find . -type f -name "finish" -path "*/s6-rc.d/*" -exec sh -c 'echo "=== $1 ==="; cat "$1"; echo' sh {} \;
Length of output: 23150
rpc_shutdown/data/run.sh (1)
2-2: Verify shellcheck integration
Let's verify that the shellcheck directive is working as intended with the custom shebang.
✅ Verification successful
Shellcheck directive successfully integrated with custom shebang
The script passes shellcheck validation without any errors, and the search results confirm that the custom shebang #!/usr/bin/with-contenv bashio is consistently used across multiple scripts in the repository. The shellcheck directive addition improves script robustness while maintaining compatibility with the existing shebang pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify shellcheck integration with custom shebang
# Test if shellcheck recognizes the script
shellcheck --version >/dev/null 2>&1 || { echo "shellcheck not available"; exit 0; }
# Check if the script passes shellcheck
shellcheck -x rpc_shutdown/data/run.sh || true
# Verify if other scripts in the repository use the same shebang pattern
rg -l "#!/usr/bin/with-contenv bashio" | while read -r file; do
echo "Found script with custom shebang: $file"
done
Length of output: 14921
google_assistant/rootfs/etc/services.d/google-assistant/run (2)
1-2: LGTM: Good shell linting practice!
The addition of the shellcheck directive explicitly specifies the shell type, which improves linting coverage and aligns with the PR's objective.
Line range hint 26-28: Consider adding error handling for Python script failures
The script directly executes the Python process without handling potential startup failures. This could make troubleshooting harder.
Let's verify if the Python script has proper error handling:
Consider adding error logging:
bashio::log.info "Starting Home Assistant GAssisant SDK..."
+if ! python3 -c "import sys; import google.assistant.library" >/dev/null 2>&1; then
+ bashio::exit.nok "Failed to import required Python packages. Please check the installation."
+fi
exec python3 /usr/bin/hassio_gassistant.py \
"${CRED_JSON}" "${PROJECT_ID}" "${MODEL_ID}" "${FEEDBACK}"configurator/rootfs/etc/s6-overlay/s6-rc.d/configurator/run (3)
1-2: LGTM! Shellcheck directive properly added.
The shellcheck directive is correctly placed and will help ensure proper linting of this script, aligning with the PR's objectives.
Line range hint 4-10: LGTM! Export statements are well organized.
The export statements are properly grouped together at the beginning of the script, following good shell scripting practices.
Line range hint 29-34: LGTM! Configuration setup is secure and well-structured.
The configuration retrieval uses the appropriate bashio helpers and safely handles the SUPERVISOR_TOKEN. The join operation for ignore_pattern is a good practice for handling array values.
Let's verify the SUPERVISOR_TOKEN usage across the codebase:
✅ Verification successful
SUPERVISOR_TOKEN is properly handled across the codebase
The token is consistently handled using secure practices across all its usages:
- In SSH addon: Used with
tempiofor template rendering in controlled environments - In Configurator: Properly passed as an API password via environment variable
- All occurrences use proper quoting and secure variable handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if SUPERVISOR_TOKEN is properly handled elsewhere
rg -l "SUPERVISOR_TOKEN" | while read -r file; do
if [[ "$file" =~ .*\.(sh|bash)$ ]] || grep -q "#!/.*bash" "$file"; then
echo "=== $file ==="
rg -A 2 -B 2 "SUPERVISOR_TOKEN" "$file"
fi
done
Length of output: 2546
silabs-multiprotocol/rootfs/etc/s6-overlay/scripts/cpcd-config-up (2)
2-2: LGTM: Shell directive added for improved linting
The addition of the shellcheck directive aligns with the PR's objective to improve script linting coverage.
Line range hint 1-51: Verify integration with S6 supervision tree
As this script is part of the S6 overlay and generates configuration for the cpcd service, we should verify its integration with related scripts.
✅ Verification successful
S6 integration is properly configured
The script's integration with the S6 supervision tree is correctly implemented:
- The
cpcd-configservice depends onbanneranduniversal-silabs-flasher - The
cpcdservice depends oncpcd-config, ensuring configuration is generated before service start - Both
otbr-agentandzigbeedservices depend oncpcd, maintaining proper startup order - The configuration file is used by the
cpcd-config-upscript to generate the required config before service start
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check related S6 scripts and their interaction with the generated config
# Find related cpcd scripts in the S6 overlay
echo "Related S6 scripts:"
fd -t f "cpcd" "silabs-multiprotocol/rootfs/etc/s6-overlay"
# Check if any scripts read the generated config
echo -e "\nScripts using the generated config:"
rg -l "/usr/local/etc/cpcd.conf" "silabs-multiprotocol/rootfs"
# Check for potential race conditions in service startup
echo -e "\nService dependencies:"
fd "dependencies.d" "silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d"
Length of output: 1553
Script:
#!/bin/bash
# Let's check the service dependencies and startup order
# Check cpcd service dependencies
echo "CPCD service dependencies:"
cat silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/cpcd/dependencies.d/*
# Check cpcd-config service dependencies
echo -e "\nCPCD-config service dependencies:"
cat silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/cpcd-config/dependencies.d/*
# Check services depending on cpcd
echo -e "\nServices depending on CPCD:"
cat silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/*/dependencies.d/cpcd 2>/dev/null
# Check cpcd service run script
echo -e "\nCPCD service run script:"
cat silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/cpcd/run
Length of output: 1192
ssh/rootfs/etc/cont-init.d/profile.sh (2)
1-5: LGTM! Shellcheck directive aligns with PR objectives.
The addition of # shellcheck shell=bash ensures proper linting of this script with the custom shebang, which was a key objective of this PR.
Line range hint 23-27: Similar robustness improvements needed for profile handling.
This section has the same considerations as the history file handling.
Please apply similar improvements as suggested for the .bash_history file handling above.
samba/rootfs/etc/s6-overlay/s6-rc.d/init-smbd/run (1)
Line range hint 1-7: LGTM! Well-structured script header and declarations.
The script follows shell scripting best practices with proper variable declarations and shellcheck configuration.
mosquitto/rootfs/etc/services.d/mosquitto/discovery (2)
1-2: LGTM: Shellcheck directive properly added
The addition of the shellcheck directive enables proper linting while maintaining the custom shebang, which aligns with the PR objectives.
Line range hint 16-28: Verify the ^ prefix usage in JSON configuration
The port and ssl values contain an unusual ^ prefix. This might be intentional, but it's worth verifying.
Additionally, consider enhancing error handling with more context:
# Send discovery info
-if bashio::discovery "mqtt" "${config}" > /dev/null; then
+if bashio::discovery "mqtt" "${config}" > /dev/null 2>&1; then
bashio::log.info "Successfully send discovery information to Home Assistant."
else
- bashio::log.error "Discovery message to Home Assistant failed!"
+ bashio::log.error "Failed to send MQTT discovery to Home Assistant. Please check Home Assistant is running and accessible."
fi✅ Verification successful
The ^ prefix in port and ssl values is intentional and correct
The ^ prefix appears to be a consistent pattern across multiple Home Assistant add-on discovery scripts (zwave_js, vlc, deconz, etc.). It's used in the bashio::var.json helper when specifying port numbers and boolean values, likely as a type hint for the discovery system.
Examples found:
- zwave_js:
port "^3000" - vlc:
port "^4212" - deconz:
port "^40850" - mosquitto:
port "^1883"andssl "^false"
The error handling suggestion remains valid as a good-to-have improvement, but the ^ prefix usage is working as designed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar patterns in other MQTT configurations
rg -l 'port.*\^[0-9]+' --type bash
rg -l 'ssl.*\^(true|false)' --type bash
Length of output: 136
Script:
#!/bin/bash
# Search for similar patterns in MQTT configurations without file type restriction
rg -l 'port.*\^[0-9]+'
rg -l 'ssl.*\^(true|false)'
# Also search for bashio::var.json usage to understand the pattern
rg -l 'bashio::var.json'
Length of output: 1582
Script:
#!/bin/bash
# Let's examine some of these discovery files to understand the pattern
echo "=== zwave_js discovery ==="
cat zwave_js/rootfs/etc/services.d/zwave_js/discovery
echo -e "\n=== vlc discovery ==="
cat vlc/rootfs/etc/s6-overlay/scripts/vlc-discovery
echo -e "\n=== deconz discovery ==="
cat deconz/rootfs/etc/services.d/deconz/discovery
Length of output: 4064
ssh/rootfs/etc/cont-init.d/ssh.sh (1)
Line range hint 17-22: Verify environment file permissions for security
The script writes sensitive SUPERVISOR_TOKEN to an environment file. Ensure this file has restricted permissions.
Add permission setting after file creation:
bashio::var.json \
supervisor_token "${SUPERVISOR_TOKEN}" \
| tempio \
-template /usr/share/tempio/ssh.environment \
-out /data/.ssh/environment
+chmod 600 /data/.ssh/environment || bashio::exit.nok 'Failed to secure environment file'silabs-multiprotocol/rootfs/etc/s6-overlay/s6-rc.d/otbr-agent/finish (1)
40-52: LGTM! Well-structured egress chain cleanup
The implementation includes good practices:
- Uses
-wflag to handle xtables locks - Proper sequence of operations
- Helpful logging
deconz/rootfs/etc/services.d/deconz/run (3)
1-2: LGTM! Shellcheck directive properly added
The addition of # shellcheck shell=bash enables proper linting of this script with the custom shebang, which aligns with the PR objectives.
Line range hint 47-56: LGTM! Well-structured debug configuration
The debug configuration is well-implemented with:
- Proper config value checking
- Sensible default values
- Clear variable naming
Line range hint 14-21: Verify libgcc paths across architectures
The LD_PRELOAD paths are architecture-specific. Let's verify these paths exist in the container.
✅ Verification successful
Let me verify the complete implementation by checking the full content of the file to ensure the paths are used correctly in their respective architecture conditions.
Architecture-specific libgcc paths are correctly implemented
The implementation correctly handles both ARM architectures:
- 32-bit ARM (armhf/armv7):
/lib/arm-linux-gnueabihf/libgcc_s.so.1 - 64-bit ARM (aarch64):
/lib/aarch64-linux-gnu/libgcc_s.so.1
The paths are properly referenced in their respective architecture-specific conditions and the file search confirms their presence in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the libgcc paths exist for both ARM architectures
# Expected: Both paths should exist in their respective architecture containers
echo "Checking for ARM 32-bit library path:"
rg -l "/lib/arm-linux-gnueabihf/libgcc_s.so.1"
echo "Checking for ARM 64-bit library path:"
rg -l "/lib/aarch64-linux-gnu/libgcc_s.so.1"
Length of output: 341
Script:
#!/bin/bash
# Get the complete implementation to verify architecture-specific handling
cat deconz/rootfs/etc/services.d/deconz/run
Length of output: 2428
deconz/rootfs/etc/services.d/deconz/discovery (2)
1-2: LGTM! Good addition of shellcheck directive
The addition of # shellcheck shell=bash directive is aligned with the PR objectives and enables proper linting of this script with its custom shebang.
1-2: Verify shellcheck linting
Let's ensure the script passes shellcheck with the custom shebang.
✅ Verification successful
Shellcheck linting is properly configured and working
The verification confirms:
- The script passes shellcheck validation without any errors
- The workflow is configured to handle custom shebangs through the
additional_filesparameter
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that shellcheck can properly lint this script
# Test: Check if shellcheck can parse and lint the script
shellcheck -x deconz/rootfs/etc/services.d/deconz/discovery || echo "Shellcheck failed to lint the script"
# Test: Verify the shebang pattern is included in the workflow
grep -A 5 "shellcheck" .github/workflows/lint.yml
Length of output: 502
tellstick/data/run.sh (1)
Line range hint 15-61: Add device configuration validation and optimize config retrieval
The device configuration loop could benefit from validation and performance improvements.
Consider applying these improvements:
# devices
+declare -A DEVICE_IDS
for device in $(bashio::config 'devices|keys'); do
DEV_ID=$(bashio::config "devices[${device}].id")
+
+ # Validate device ID uniqueness
+ if [[ -n "${DEVICE_IDS[$DEV_ID]:-}" ]]; then
+ bashio::log.error "Duplicate device ID: ${DEV_ID}"
+ exit 1
+ fi
+ DEVICE_IDS[$DEV_ID]=1
+
+ # Batch retrieve device config to reduce calls
+ device_config="$(bashio::config "devices[${device}]")"
+ DEV_NAME="$(bashio::jq "${device_config}" '.name')"
+ [[ -z "${DEV_NAME}" ]] && bashio::log.error "Device name is required for ID ${DEV_ID}"silabs-multiprotocol/rootfs/etc/s6-overlay/scripts/universal-silabs-flasher-up (1)
Line range hint 1-13: LGTM! Well-structured script setup.
Good practices observed:
- Clear variable declarations upfront
- Proper shellcheck configuration
- Appropriate shebang for s6-overlay environment
mosquitto/rootfs/etc/cont-init.d/mosquitto.sh (1)
1-2: LGTM! Good addition of shellcheck directive.
The addition of the shellcheck directive improves code quality by enabling proper shell script linting.
duckdns/rootfs/etc/s6-overlay/s6-rc.d/duckdns/run (1)
Line range hint 2-13: LGTM! Well-structured initialization with proper error handling.
The script properly initializes variables with fallbacks and uses bashio's configuration management effectively.
matter_server/rootfs/etc/s6-overlay/s6-rc.d/matter-server/run (5)
2-2: LGTM: Shellcheck directive properly added
The shellcheck shell directive is correctly placed and aligns with the PR's objective to improve script linting.
28-32: LGTM: Appropriate shellcheck suppression for pip extras
The SC2102 suppression is correctly applied for the pip install commands. This is necessary because the [server] notation is a valid pip extra and not a pattern range that shellcheck warns about.
70-71: LGTM: Appropriate shellcheck suppression for array assignment
The SC2207 suppression is acceptable here as we're dealing with a simple config value assignment to an array.
74-76: LGTM: Appropriate shellcheck suppression for custom arguments
The SC2207 suppression is correctly applied for handling custom matter server arguments.
87-87: LGTM: Appropriate shellcheck suppression for array assignment
The SC2206 suppression is correctly applied for the matter_server_args array assignment.
mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-post/run (2)
Line range hint 24-24: LGTM: Improved service cleanup handling
The use of execute_without_error_messages for service deletion is appropriate here as it handles expected failures gracefully while ensuring the script continues execution.
Line range hint 89-93: Consider enhancing security measures
Several security improvements could be made to the script:
- The hardcoded IP ranges (
172.30.32.%and172.30.33.%) should ideally be configurable - SQL commands should use prepared statements or proper escaping for user inputs
- Consider using more secure password handling methods
Let's verify if these IP ranges are used consistently across the codebase:
zwave_js/rootfs/etc/cont-init.d/config.sh (3)
1-2: LGTM! Good addition of shellcheck directive.
The addition of the shellcheck directive aligns with the PR's objective to improve script linting coverage.
Line range hint 91-100: LGTM! Well-implemented logging configuration.
The logging setup includes proper fallback behavior and clear messaging.
Line range hint 147-171: LGTM! Well-structured configuration generation.
The use of tempio for template processing and proper handling of all configuration variables is well-implemented.
openthread_border_router/rootfs/etc/s6-overlay/s6-rc.d/otbr-agent/run (3)
2-2: Good practice in specifying the shell for ShellCheck
Adding # shellcheck shell=bash ensures that ShellCheck parses the script correctly as Bash. This enhances code linting and helps prevent potential issues.
7-7: Appropriate use of disabling SC1091 for sourcing scripts
Disabling SC1091 is acceptable here since the script being sourced is at a known absolute path. This prevents unnecessary warnings from ShellCheck.
72-72: 🛠️ Refactor suggestion
Refactor to use proper conditional statements instead of && and ||
Using && and || for control flow can lead to unintended behavior, especially if the first command succeeds but the second fails. It's better to use an if statement to handle success and failure cases explicitly.
Apply this diff to refactor the code:
-# shellcheck disable=SC2015
-mkdir -p /data/thread && ln -sft /var/lib /data/thread || bashio::exit.nok "Could not create directory /var/lib/thread to store Thread data."
+if mkdir -p /data/thread && ln -sft /var/lib /data/thread; then
+ :
+else
+ bashio::exit.nok "Could not create directory /var/lib/thread to store Thread data."
+fiThis refactor ensures that bashio::exit.nok is only called if either mkdir or ln fails.
Likely invalid or redundant comment.
silabs-multiprotocol/rootfs/etc/s6-overlay/scripts/universal-silabs-flasher-up
Show resolved
Hide resolved
silabs-multiprotocol/rootfs/etc/s6-overlay/scripts/universal-silabs-flasher-up
Show resolved
Hide resolved
|
Note that I'm ignoring most of Coderabbit suggestions because it doesn't understand that I'm merely trying to make CI pass with minimum impact to the codebase. Sure, its suggestions are legitimate, but even such an innocuous change as adding double quotes around an argument passed to a command can have negative consequences if the previous code relied (intentionally or accidentally) on word-splitting, for example. Because I'd rather avoid making code changes across dozens of scripts in this repository, I've opted to pepper |
| shutdown | ||
| universal-silabs-flasher-up | ||
| unlock-tables-for-backup | ||
| vlc-discovery |
There was a problem hiding this comment.
It doesn't matter much, but for clarity, should we maybe use the full path for the add-on specific scripts, e.g. vlc/rootfs/etc/s6-overlay/scripts/vlc-discovery? 🤔
I'd still use just file name for the generic s6-init scripts (like run and friends).
There was a problem hiding this comment.
I would have loved to use full paths, but the additional_files input to the action only supports file names and not paths: https://github.com/ludeeus/action-shellcheck/blob/00b27aa7cb85167568cb48a3838b75f4265f2bca/action.yaml#L141-L143
|
@coderabbitai resolve This PR doesn't try to fix shellcheck issues, it tries to enable along with minimal fixes. @coderabbitai pause |
|
@agners: Understood, I will resolve the comments and pause the automatic reviews for this PR. ✅ Actions performedComments resolved. Reviews paused. |
|
@agners I appreciate your assistance so far. Let me know if there is anything else to change/add to this PR. I believe this should be good to go, but there was one CI failure that looks intermittent; i.e. I don't think it's likely that it was caused by the changes in this branch. |
Many scripts in this repository have this shebang:
However, this doesn't match the shebang regex in
action-shellcheckand therefore those files wouldn't get included in the linting check.Supersedes #3803 /cc @agners
This is the only mechanism to include additional scripts in the linting process. I'm not a fan of this approach, since new scripts with unique names added to this repository again won't be linted by default, but it's a start to ensure regressions like #3801 are less likely to happen.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores